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
