From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45793) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFdfp-0002qM-O5 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 00:56:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFdfo-0001Kh-Gx for qemu-devel@nongnu.org; Tue, 18 Feb 2014 00:56:53 -0500 Received: from mail-vc0-x230.google.com ([2607:f8b0:400c:c03::230]:61143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFdfo-0001KZ-An for qemu-devel@nongnu.org; Tue, 18 Feb 2014 00:56:52 -0500 Received: by mail-vc0-f176.google.com with SMTP id la4so12367000vcb.7 for ; Mon, 17 Feb 2014 21:56:51 -0800 (PST) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <53025B9B.5090405@redhat.com> References: <1392186806-10418-1-git-send-email-cyliu@suse.com> <1392186806-10418-18-git-send-email-cyliu@suse.com> <53025B9B.5090405@redhat.com> Date: Tue, 18 Feb 2014 13:56:51 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=001a1133eec2466de004f2a7f048 Subject: Re: [Qemu-devel] [PATCH v20 17/26] rbd.c: replace QEMUOptionParameter with QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Dong Xu Wang , qemu-devel@nongnu.org, stefanha@redhat.com --001a1133eec2466de004f2a7f048 Content-Type: text/plain; charset=UTF-8 2014-02-18 2:57 GMT+08:00 Eric Blake : > On 02/11/2014 11:33 PM, Chunyan Liu wrote: > > rbd.c: replace QEMUOptionParameter with QemuOpts > > > > Signed-off-by: Dong Xu Wang > > Signed-off-by: Chunyan Liu > > --- > > block/rbd.c | 63 > +++++++++++++++++++++++++++++------------------------------ > > 1 files changed, 31 insertions(+), 32 deletions(-) > > > > > + objsize = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0); > > + if (objsize) { > > + if ((objsize - 1) & objsize) { /* not a power of 2? */ > > + error_report("obj size needs to be power of 2"); > > + return -EINVAL; > > + } > > + if (objsize < 4096) { > > + error_report("obj size too small"); > > + return -EINVAL; > > } > > > + { > > + .name = BLOCK_OPT_CLUSTER_SIZE, > > + .type = QEMU_OPT_SIZE, > > + .help = "RBD object size", > > + .def_value_str = stringify(0), > > Do we really want to list a default of 0, given that the code behaves > the same as if 0 had been specified or if the argument had been omitted? > Passing 0 doesn't really mean using a cluster size of 0, which makes > listing it as an explicit default would look odd. > > Inherited from old patches. Not necessary in fact. Will remove that. > > - .create_options = qemu_rbd_create_options, > > + .create_opts = qemu_rbd_create_opts, > > Hmm, I've noticed that your series is inconsistent on whether it uses '= > foo_opts' or '= &foo_opts' in these initializers (for example, 15/26 > used & even though nothing else did, 16/26 uses & but all other > initializers were also using &, and in this 17/26 nothing is using &). > Not that it matters to the compiler, but there's a lot to be said for > consistency. > > Sorry, will correct that. > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --001a1133eec2466de004f2a7f048 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-02-18 2:57 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 02/11/= 2014 11:33 PM, Chunyan Liu wrote:
> rbd.c: replace QEMUOptionParameter with QemuOpts
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu= @suse.com>
> ---
> =C2=A0block/rbd.c | =C2=A0 63 +++++++++++++++++++++++++++++-----------= -------------------
> =C2=A01 files changed, 31 insertions(+), 32 deletions(-)
>

> + =C2=A0 =C2=A0objsize =3D qemu_opt_get_size_del= (opts, BLOCK_OPT_CLUSTER_SIZE, 0);
> + =C2=A0 =C2=A0if (objsize) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((objsize - 1) & objsize) { =C2=A0= =C2=A0/* not a power of 2? */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("obj size= needs to be power of 2");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (objsize < 4096) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_report("obj size= too small");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.name =3D BLOCK_OPT_CLUSTER= _SIZE,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type =3D QEMU_OPT_SIZE, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.help =3D "RBD object = size",
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.def_value_str =3D stringif= y(0),

Do we really want to list a default of 0, given that the code behaves=
the same as if 0 had been specified or if the argument had been omitted? =C2=A0Passing 0 doesn't really mean using a cluster size of 0, which ma= kes
listing it as an explicit default would look odd.

Inherited from old patches. Not= necessary in fact. Will remove that.
=C2=A0
> - =C2=A0 =C2=A0.create_options =C2=A0 =C2=A0 =3D qemu_rbd_create_optio= ns,
> + =C2=A0 =C2=A0.create_opts =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D qemu_rbd_cr= eate_opts,

Hmm, I've noticed that your series is inconsistent on whether it = uses '=3D
foo_opts' or '=3D &foo_opts' in these initializers (for exa= mple, 15/26
used & even though nothing else did, 16/26 uses & but all other
initializers were also using &, and in this 17/26 nothing is using &= ;).
Not that it matters to the compiler, but there's a lot to be said for consistency.


Sorry, will correct that.
=C2=A0
=
--
Eric Blake =C2=A0 eblake redhat com =C2=A0 =C2=A0+1-919-301-3266
Libvirt virtualization library http://libvirt.org


--001a1133eec2466de004f2a7f048--