block: untangle open flag manipulation in bdrv_open2
PATCH 2/2 block: untangle open flag manipulation in bdrv_open2
Untangle the open flag manipulation in bdrv_open2 and document why we are clearing the various flags in the different flag combinations.
block.c +33 -13
@@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, consint bdrv_open2 (BlockDriverState *bs,const char *filename,int flags,BlockDriver *drv) { -int ret, open_flags, try_rw; +int ret, open_flags;char tmp_filename[PATH_MAX ];char backing_filename[PATH_MAX ];
@@ -450,19 +450,39 @@ int bdrv_open2(BlockDriverState *bs, conif (flags & (BDRV_O_CACHE_WB |BDRV_O_NOCACHE )) bs->enable_write_cache =1 ; -/* Note: for compatibility, we open disk image files as RDWR, and -RDONLY as fallback */ - try_rw = !bs->read_only || bs->is_temporary; -if (!(flags &BDRV_O_FILE )) - open_flags = (try_rw ?BDRV_O_RDWR :0 ) | - (flags & (BDRV_O_CACHE_MASK |BDRV_O_NATIVE_AIO )); -else - open_flags = flags & ~(BDRV_O_FILE |BDRV_O_SNAPSHOT ); +/* +* Always clear these flags as they are not destined for the drivers. +*/ + open_flags = flags & ~(BDRV_O_FILE |BDRV_O_SNAPSHOT ); + +if (!(flags &BDRV_O_FILE )) { +/* +* For historical reasons we always try to open drive images as +* read-write first and only fall back to read-only later. +* +* This can be overriden using readonly suboption for the +* drive option. +*/ +if (bs->read_only && !bs->is_temporary) { + open_flags &= ~BDRV_O_RDWR ; + }else { + open_flags |=BDRV_O_RDWR ; + } - ret = drv->bdrv_open (bs, filename, open_flags); -if ((ret == -EACCES || ret == -EPERM ) && !(flags &BDRV_O_FILE )) { - ret = drv->bdrv_open (bs, filename, open_flags & ~BDRV_O_RDWR ); - bs->read_only =1 ; +/* +* Currently BDRV_O_CREAT is not supported by any image format, +* but I'm not sure that's reason enough to always clear it for +* the !BDRV_O_FILE case.. +*/ + open_flags &= ~(BDRV_O_CREAT ); + + ret = drv->bdrv_open (bs, filename, open_flags); +if (ret == -EACCES || ret == -EPERM ) { + ret = drv->bdrv_open (bs, filename, open_flags & ~BDRV_O_RDWR ); + bs->read_only =1 ; + } + }else { + ret = drv->bdrv_open (bs, filename, open_flags); }if (ret <0 ) {
Am 11.01.2010 18:52, schrieb Christoph Hellwig:
Quoted 5 lines
Untangle the open flag manipulation in
bdrv_open2and document why we are clearing the various flags in the different flag combinations.Signed-off-by Christoph Hellwig <[email protected]>
Nice cleanup.
block.c int bdrv_open2(BlockDriverState *bs, con · line 477
Quoted 6 lines
block.c +6
-0+/* +* Currently BDRV_O_CREAT is not supported by any image format, +* but I'm not sure that's reason enough to always clear it for +* the !BDRV_O_FILE case.. +*/ + open_flags &= ~(BDRV_O_CREAT );
Is there even a theoretical use case for this flag? When you want to create an image, you use bdrv_create. I'd suggest another patch to remove BDRV_O_CREAT entirely.
Maybe except if we want to support protocols with bdrv_create, so we would go for the block layer functions rather than the POSIX functions. However, I don't think there is a need to do so.
Kevin
Quoted 18 lines · Kevin Wolf
Am 11.01.2010 18:52, schrieb Christoph Hellwig:
Quoted 5 lines
Untangle the open flag manipulation in
bdrv_open2and document why we are clearing the various flags in the different flag combinations.Signed-off-by Christoph Hellwig <[email protected]>Nice cleanup.
Acked-by Kevin Wolf <[email protected]>block.c int bdrv_open2(BlockDriverState *bs, con · line 477
Quoted 6 lines
block.c +6
-0+/* +* Currently BDRV_O_CREAT is not supported by any image format, +* but I'm not sure that's reason enough to always clear it for +* the !BDRV_O_FILE case.. +*/ + open_flags &= ~(BDRV_O_CREAT );
Quoted 3 lines
Is there even a theoretical use case for this flag? When you want to create an image, you use
bdrv_create. I'd suggest another patch to removeBDRV_O_CREATentirely.
Sounds good. Currently the only user of it is qemu-io. I tried to look up the history of it but with all the renamings of the block files I haven't managed to find what it used to be for.
[Qemu-devel] PATCH 3/2] block: kill BDRV_O_CREAT
In reply to Christoph HellwigThe BDRV_O_CREAT option is unused inside qemu and partially duplicates the bdrv_create method. Remove it, and the -C option to qemu-io which isn't used in qemu-iotests anyway.
block.c +0 -7
@@ -469,13 +469,6 @@ int bdrv_open2(BlockDriverState *bs, con open_flags |=BDRV_O_RDWR ; } -/* -* Currently BDRV_O_CREAT is not supported by any image format, -* but I'm not sure that's reason enough to always clear it for -* the !BDRV_O_FILE case.. -*/ - open_flags &= ~(BDRV_O_CREAT ); - ret = drv->bdrv_open (bs, filename, open_flags);if (ret == -EACCES || ret == -EPERM ) { ret = drv->bdrv_open (bs, filename, open_flags & ~BDRV_O_RDWR );
block.h +1 -1
@@ -30,7 +30,7 @@ typedef struct QEMUSnapshotInfo {#define BDRV_O_RDONLY 0x0000#define BDRV_O_RDWR 0x0002#define BDRV_O_ACCESS 0x0003 -#define BDRV_O_CREAT 0x0004/* create an empty file */ +/* 0x0004 was BDRV_O_CREAT */ #define BDRV_O_SNAPSHOT 0x0008/* open the file read only and save writes in a snapshot */ #define BDRV_O_FILE 0x0010/* open as a raw file (do not try to use a disk image format on top of
block/nbd.c +0 -3
@@ -49,9 +49,6 @@ static int nbd_open(BlockDriverState *bssize_t blocksize;int ret; -if ((flags &BDRV_O_CREAT )) -return -EINVAL ; -if (!strstart (filename,"nbd:" , &host))return -EINVAL ;
block/raw-posix.c +1 -5
@@ -205,13 +205,9 @@ out_close:static int raw_open (BlockDriverState *bs,const char *filename,int flags) {BDRVRawState *s = bs->opaque; -int open_flags =0 ; s->type =FTYPE_FILE ; -if (flags &BDRV_O_CREAT ) - open_flags =O_CREAT |O_TRUNC ; - -return raw_open_common (bs, filename, flags, open_flags); +return raw_open_common (bs, filename, flags,0 ); }/* XXX: use host sector size if necessary with:
block/raw-win32.c +3 -7
@@ -76,7 +76,7 @@ static int set_sparse(int fd)static int raw_open (BlockDriverState *bs,const char *filename,int flags) {BDRVRawState *s = bs->opaque; -int access_flags, create_flags; +int access_flags;DWORD overlapped; s->type =FTYPE_FILE ;
@@ -86,11 +86,7 @@ static int raw_open(BlockDriverState *bs }else { access_flags =GENERIC_READ ; } -if (flags &BDRV_O_CREAT ) { - create_flags =CREATE_ALWAYS ; - }else { - create_flags =OPEN_EXISTING ; - } + overlapped =FILE_ATTRIBUTE_NORMAL ;if ((flags &BDRV_O_NOCACHE )) overlapped |=FILE_FLAG_NO_BUFFERING |FILE_FLAG_WRITE_THROUGH ;
@@ -98,7 +94,7 @@ static int raw_open(BlockDriverState *bs overlapped |=FILE_FLAG_WRITE_THROUGH ; s->hfile =CreateFile (filename, access_flags,FILE_SHARE_READ ,NULL , - create_flags, overlapped,NULL ); +OPEN_EXISTING , overlapped,NULL );if (s->hfile ==INVALID_HANDLE_VALUE ) {int err =GetLastError ();
qemu-io.c +3 -12
@@ -1307,7 +1307,6 @@ open_help(void)" 'open -Cn /tmp/data' - creates/opens data file read-write and uncached\n" "\n" " Opens a file for subsequent use by all of the other qemu-io commands.\n" -" -C, -- create new file if it doesn't exist\n" " -r, -- open file read-only\n" " -s, -- use snapshot file\n" " -n, -- disable host cache\n"
@@ -1337,7 +1336,7 @@ open_f(int argc, char **argv)int growable =0 ;int c; -while ((c =getopt (argc, argv,"snCrg" )) !=EOF ) { +while ((c =getopt (argc, argv,"snrg" )) !=EOF ) {switch (c) {case 's' : flags |=BDRV_O_SNAPSHOT ;
@@ -1345,9 +1344,6 @@ open_f(int argc, char **argv)case 'n' : flags |=BDRV_O_NOCACHE ;break ; -case 'C' : - flags |=BDRV_O_CREAT ; -break ;case 'r' : readonly =1 ;break ;
@@ -1396,10 +1392,9 @@ init_check_command(static void usage (const char *name) {printf ( -"Usage: %s [-h] [-V] [-Crsnm] [-c cmd] ... [file]\n" +"Usage: %s [-h] [-V] [-rsnm] [-c cmd] ... [file]\n" "QEMU Disk exerciser\n" "\n" -" -C, --create create new file if it doesn't exist\n" " -c, --cmd command to execute\n" " -r, --read-only export read-only\n" " -s, --snapshot use snapshot file\n"
@@ -1418,13 +1413,12 @@ int main(int argc, char **argv) {int readonly =0 ;int growable =0 ; -const char *sopt ="hVc:Crsnmgk" ; +const char *sopt ="hVc:rsnmgk" ;const struct option lopt[] = { {"help" ,0 ,NULL ,'h' }, {"version" ,0 ,NULL ,'V' }, {"offset" ,1 ,NULL ,'o' }, {"cmd" ,1 ,NULL ,'c' }, - {"create" ,0 ,NULL ,'C' }, {"read-only" ,0 ,NULL ,'r' }, {"snapshot" ,0 ,NULL ,'s' }, {"nocache" ,0 ,NULL ,'n' },
@@ -1450,9 +1444,6 @@ int main(int argc, char **argv)case 'c' :add_user_command (optarg);break ; -case 'C' : - flags |=BDRV_O_CREAT ; -break ;case 'r' : readonly =1 ;break ;