From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53005) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz7Gm-0006K4-Ah for qemu-devel@nongnu.org; Mon, 23 Jun 2014 12:39:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wz7Ge-0004ok-QT for qemu-devel@nongnu.org; Mon, 23 Jun 2014 12:39:00 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:51942 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz7Ge-0004od-I9 for qemu-devel@nongnu.org; Mon, 23 Jun 2014 12:38:52 -0400 Date: Mon, 23 Jun 2014 18:38:50 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140623163850.GA28144@irqsave.net> References: <1402495503-4722-1-git-send-email-kwolf@redhat.com> <1402495503-4722-2-git-send-email-kwolf@redhat.com> <20140612120839.GA24528@irqsave.net> <20140623153045.GH10973@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140623153045.GH10973@noname.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com The Monday 23 Jun 2014 =E0 17:30:45 (+0200), Kevin Wolf wrote : > Am 12.06.2014 um 14:08 hat Beno=EEt Canet geschrieben: > > The Wednesday 11 Jun 2014 =E0 16:04:55 (+0200), Kevin Wolf wrote : > > > The idea of bdrv_fill_options() is to convert every parameter for > > > opening images, in particular the filename and flags, to entries in= the > > > options QDict. > > >=20 > > > This patch starts with moving the filename parsing and driver probi= ng > > > part from bdrv_file_open() to the new function. > > >=20 > > > Signed-off-by: Kevin Wolf > > > --- > > > block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------= ------------ > > > 1 file changed, 73 insertions(+), 43 deletions(-) > > >=20 > > > diff --git a/block.c b/block.c > > > index 17f763d..c5707e8 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -1007,77 +1007,107 @@ free_and_fail: > > > } > > > =20 > > > /* > > > - * Opens a file using a protocol (file, host_device, nbd, ...) > > > - * > > > - * options is an indirect pointer to a QDict of options to pass to= the block > > > - * drivers, or pointer to NULL for an empty set of options. If thi= s function > > > - * takes ownership of the QDict reference, it will set *options to= NULL; > > > - * otherwise, it will contain unused/unrecognized options after th= is function > > > - * returns. Then, the caller is responsible for freeing it. If it = intends to > > > - * reuse the QDict, QINCREF() should be called beforehand. > > > + * Fills in default options for opening images and converts the le= gacy > > > + * filename/flags pair to option QDict entries. > > > */ > > > -static int bdrv_file_open(BlockDriverState *bs, const char *filena= me, > > > - QDict **options, int flags, Error **errp= ) > > > +static int bdrv_fill_options(QDict **options, const char *filename= , > > > + Error **errp) > > > { > > > - BlockDriver *drv; > > > const char *drvname; > > > bool parse_filename =3D false; > > > Error *local_err =3D NULL; > > > - int ret; > > > + BlockDriver *drv; > > > =20 > > > /* Fetch the file name from the options QDict if necessary */ > > > - if (!filename) { > > > - filename =3D qdict_get_try_str(*options, "filename"); > > > - } else if (filename && !qdict_haskey(*options, "filename")) { > > > - qdict_put(*options, "filename", qstring_from_str(filename)= ); > > > - parse_filename =3D true; > > > - } else { > > > - error_setg(errp, "Can't specify 'file' and 'filename' opti= ons at the " > > > - "same time"); > > > - ret =3D -EINVAL; > > > - goto fail; > > > + if (filename) { > > > + if (filename && !qdict_haskey(*options, "filename")) { > >=20 > > The inner filename testing is redundant. >=20 > Will fix. >=20 > > > + qdict_put(*options, "filename", qstring_from_str(filen= ame)); > > > + parse_filename =3D true; > > > + } else { > > > + error_setg(errp, "Can't specify 'file' and 'filename' = options at " > > > + "the same time"); > > > + return -EINVAL; > > > + } > > > } > > > =20 > > > /* Find the right block driver */ > > > + filename =3D qdict_get_try_str(*options, "filename"); > > > drvname =3D qdict_get_try_str(*options, "driver"); > > > - if (drvname) { > > > - drv =3D bdrv_find_format(drvname); > > > - if (!drv) { > > > - error_setg(errp, "Unknown driver '%s'", drvname); > > > - } > > > - qdict_del(*options, "driver"); > > > - } else if (filename) { > > > - drv =3D bdrv_find_protocol(filename, parse_filename); > > > - if (!drv) { > > > - error_setg(errp, "Unknown protocol"); > > > + > > > + if (!drvname) { > > > + if (filename) { > > > + drv =3D bdrv_find_protocol(filename, parse_filename); > > > + if (!drv) { > > > + error_setg(errp, "Unknown protocol"); > > > + return -EINVAL; > > > + } > > > + > > > + drvname =3D drv->format_name; > > > + qdict_put(*options, "driver", qstring_from_str(drvname= )); > > > + } else { > > > + error_setg(errp, "Must specify either driver or file")= ; > >=20 > > Isn't it "Must specify either driver or _filename_ ? >=20 > This is only code motion, but changing it in a separate patch would mak= e > sense, I guess. >=20 > > > + return -EINVAL; > > > } > > > - } else { > > > - error_setg(errp, "Must specify either driver or file"); > > > - drv =3D NULL; > > > } > > > =20 > > > + drv =3D bdrv_find_format(drvname); > > > if (!drv) { > > > - /* errp has been set already */ > > > - ret =3D -ENOENT; > > > - goto fail; > > > + error_setg(errp, "Unknown driver '%s'", drvname); > > > + return -ENOENT; > > > } > > > =20 > > > - /* Parse the filename and open it */ > > > + /* Driver-specific filename parsing */ > > > if (drv->bdrv_parse_filename && parse_filename) { > > > drv->bdrv_parse_filename(filename, *options, &local_err); > > > if (local_err) { > > > error_propagate(errp, local_err); > > > - ret =3D -EINVAL; > > > - goto fail; > > > + return -EINVAL; > > > } > > > =20 > > > if (!drv->bdrv_needs_filename) { > > > qdict_del(*options, "filename"); > > > - } else { > > > - filename =3D qdict_get_str(*options, "filename"); > > > } > > > } > > > =20 > > > + return 0; > > > +} > > > + > > > +/* > > > + * Opens a file using a protocol (file, host_device, nbd, ...) > > > + * > > > + * options is an indirect pointer to a QDict of options to pass to= the block > > > + * drivers, or pointer to NULL for an empty set of options. If thi= s function > > > + * takes ownership of the QDict reference, it will set *options to= NULL; > > > + * otherwise, it will contain unused/unrecognized options after th= is function > > > + * returns. Then, the caller is responsible for freeing it. If it = intends to > > > + * reuse the QDict, QINCREF() should be called beforehand. > > > + */ > > > +static int bdrv_file_open(BlockDriverState *bs, const char *filena= me, > > > + QDict **options, int flags, Error **errp= ) > > > +{ > > > + BlockDriver *drv; > > > + const char *drvname; > > > + Error *local_err =3D NULL; > > > + int ret; > > > + > > > + ret =3D bdrv_fill_options(options, filename, &local_err); > > > + if (local_err) { > > > + error_propagate(errp, local_err); > > > + return ret; > > Why not goto fail for consistency with the rest of the function ? >=20 > Okay. Rather pointless and the function will be gone at the end of the > series, but more consistent indeed. >=20 > > > + } > > > + > > > + filename =3D qdict_get_try_str(*options, "filename"); > > > + drvname =3D qdict_get_str(*options, "driver"); > > > + > > > + drv =3D bdrv_find_format(drvname); > > > + if (!drv) { > > > + error_setg(errp, "Unknown driver '%s'", drvname); > > > + ret =3D -ENOENT; > > > + goto fail; > > > + } > > > + qdict_del(*options, "driver"); > > > + > > > + /* Open the file */ > > > if (!drv->bdrv_file_open) { > >=20 > > Don't we need: > > qdict_del(*options, "filename"); > > Here so bdrv_open don't see an extra filename option ? >=20 > I think it doesn't matter because bdrv_open() would put it back again. > Do you have an example where it makes a difference? No, I don't understand the code that much. I was just thinking outloud betting it would point to something. It's not= :) >=20 > > > ret =3D bdrv_open(&bs, filename, NULL, *options, flags, dr= v, &local_err); > > > *options =3D NULL; >=20 > Kevin