From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40556) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X7Hn4-0002Er-Uf for qemu-devel@nongnu.org; Wed, 16 Jul 2014 01:30:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X7Hmv-0001Es-U6 for qemu-devel@nongnu.org; Wed, 16 Jul 2014 01:30:06 -0400 Received: from mail-pa0-x22a.google.com ([2607:f8b0:400e:c03::22a]:35126) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X7Hmv-0001Ej-Lb for qemu-devel@nongnu.org; Wed, 16 Jul 2014 01:29:57 -0400 Received: by mail-pa0-f42.google.com with SMTP id lf10so638073pab.15 for ; Tue, 15 Jul 2014 22:29:56 -0700 (PDT) Date: Wed, 16 Jul 2014 13:30:00 +0800 From: Liu Yuan Message-ID: <20140716053000.GF15811@ubuntu-trusty> References: <1405406098-30981-1-git-send-email-namei.unix@gmail.com> <1405406098-30981-2-git-send-email-namei.unix@gmail.com> <53C51B28.4030303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53C51B28.4030303@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 1/2] block/quorum: add simple read pattern support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Benoit Canet 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