Naphtali Sprei <nsp...@redhat.com> writes: > Added 'access' option to -drive flag > > The new option is: access=[rw|ro|auto] > rw: open the drive's file with Read and Write permission, don't continue if > failed > ro: open the file only with Read permission > auto: open the file with Read and Write permission, if failed, try only Read > permision > > For compatibility reasons, the default is 'auto'. Should be changed later on. > > This option is to replace the 'readonly' options added lately.
Can we take the readonly parameter away? It's undocumented, for whatever that's worth... > Instead of using the field 'readonly' of the BlockDriverState struct for > passing the request, > pass the request in the flags parameter to the function. You went half way from "we have a number of access modes stored in the bits BDRV_O_ACCESS, and you shouldn't assume anything on how they're encoded" to a simple, straightforward "writable" bit. I think the single bit leads to simpler clean code than the access mode, but half way is worse than either of them. Let me illustrate. Access mode: * Test writable: (flags & ~ BDRV_O_ACCESS) == BDRV_O_RDWR * Make writable: flags = (flags & ~ BDRV_O_ACCESS) | BDRV_O_RDWR; * Make read-only: flags = (flags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY; Single bit: * Test writable: flags & BDRV_O_WRITABLE * Make writable: flags |= BDRV_O_WRITABLE; * Make read-only: flags &= ~BDRV_O_WRITABLE; Your code can't quite decide which of the two methods to adopt. You still have BDRV_O_RDONLY and BDRV_O_RDWR. Clean users shouldn't assume anything about how the two are encoded. Consider: ret = drv->bdrv_open(bs, filename, (open_flags & ~BDRV_O_RDWR) | BDRV_O_RDONLY); Clean, but awkward. In another place, you opt for less awkward, but unclean: open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); This assumes that BDRV_O_RDONLY is 0. In my opinion, any benefit in readability you might hope gain by having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you need to keep knowledge of its encoding out of its users.