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

[Liu Yuan <namei.unix@gmail.com>]

On Tue, Jul 15, 2014 at 06:14:32AM -0600, Eric Blake wrote:
> 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.
> 

That's awesome, thanks for your pointers, Eric.

Yuan