Re: [Qemu-devel] [PATCH v3 1/2] block/quorum: add simple read pattern support

[Eric Blake]

On 07/15/2014 12:34 AM, Liu Yuan wrote:
> This patch adds single read pattern to quorum driver and quorum vote is 
> default
> pattern.
> 
> For now we do a quorum vote on all the reads, it is designed for unreliable
> underlying storage such as non-redundant NFS to make sure data integrity at 
> the
> cost of the read performance.
> 

>                              */
> +
> +#define READ_PATTERN_QUORUM 0 /* default */
> +#define READ_PATTERN_FIFO   1
> +    int read_pattern;      /* fifo: read a single child and try first one
> +                            *         first. If error, try next child in an
> +                            *         FIFO order specifed by command line.
> +                            *         Return error if no child read succeeds.
> +                            * quorum: read all the children and do a quorum
> +                            *         vote on reads.
> +                            */

Please swap the order of your two patches, or squash them back into one.
 If you define the qapi enum first, then you don't need to open-code
these #defines; instead, the qapi code will generate a typedef that puts
the C enum QuorumReadPattern into play, so that this declaration becomes
'QuorumReadPattern read_pattern;'...


> @@ -263,6 +292,21 @@ static void quorum_aio_cb(void *opaque, int ret)
>      BDRVQuorumState *s = acb->common.bs->opaque;
>      bool rewrite = false;
>  
> +    if (acb->is_read && s->read_pattern == READ_PATTERN_FIFO) {

...and code like this can compare against the constant
QUORUM_READ_PATTERN_FIFO (which will be generated for you from the .json
file)...


> +    read_pattern = qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN);
> +    if (read_pattern) {
> +        if (strcmp(read_pattern, "fifo") == 0) {
> +            s->read_pattern = READ_PATTERN_FIFO;
> +        } else if (strcmp(read_pattern, "quorum") == 0) {
> +            s->read_pattern = READ_PATTERN_QUORUM;
> +        } else {
> +            error_setg(&local_err,
> +                       "Please set read-pattern as fifo or quorum\n");
> +            ret = -EINVAL;
> +            goto exit;
> +        }

...and instead of open-coding this, you should use parse_enum_option().
Also, by using the generated list here, if you later add a third read
pattern, then this parser code doesn't have to change, but already
automatically covers all valid options encoded in the qapi list.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature