Re: [Qemu-devel] [PATCH] Added 'access' option to -drive flag

[Markus Armbruster]

Naphtali Sprei <address@hidden> 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.