From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58829) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSZ0M-0003rU-F8 for qemu-devel@nongnu.org; Tue, 25 Mar 2014 17:35:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WSZ0G-0004na-ET for qemu-devel@nongnu.org; Tue, 25 Mar 2014 17:35:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSZ0G-0004n7-5Y for qemu-devel@nongnu.org; Tue, 25 Mar 2014 17:35:24 -0400 Message-ID: <5331F699.3010502@redhat.com> Date: Tue, 25 Mar 2014 15:35:21 -0600 From: Eric Blake MIME-Version: 1.0 References: <1395396763-26081-1-git-send-email-cyliu@suse.com> <1395396763-26081-9-git-send-email-cyliu@suse.com> In-Reply-To: <1395396763-26081-9-git-send-email-cyliu@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UWwo1rXxdW3Lj8lmQUjAAfwWshiDaBSuj" Subject: Re: [Qemu-devel] [PATCH v23 08/32] add convert functions between QEMUOptionParameter to QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunyan Liu , qemu-devel@nongnu.org Cc: stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --UWwo1rXxdW3Lj8lmQUjAAfwWshiDaBSuj Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/21/2014 04:12 AM, Chunyan Liu wrote: > Add two temp convert functions between QEMUOptionParameter to QemuOpts,= s/convert/conversion/ here and in subject > so that next patch can use it. It will simplify later patch for easier > review. And will be finally removed after all backend drivers switch to= > QemuOpts. >=20 > Signed-off-by: Chunyan Liu > --- > +++ b/include/qemu/option.h > @@ -103,6 +103,11 @@ typedef struct QemuOptDesc { > } QemuOptDesc; > =20 > struct QemuOptsList { > + /* FIXME: Temp used for QEMUOptionParamter->QemuOpts conversion to= > + * indicate free memory. Will remove after all drivers switch to Q= emuOpts. > + */ > + bool mallocd; Spelling looks odd; I might have used 'allocated'. But as it gets deleted later, I don't care. > + num_opts =3D count_option_parameters(list); > + opts =3D g_malloc0(sizeof(QemuOptsList) + > + (num_opts + 1) * sizeof(QemuOptDesc)); [1]... > + while (list && list->name) { > + opts->desc[i].name =3D g_strdup(list->name); > + opts->desc[i].help =3D g_strdup(list->help); > + switch (list->type) { > + case OPT_FLAG: > + opts->desc[i].type =3D QEMU_OPT_BOOL; > + opts->desc[i].def_value_str =3D > + g_strdup(list->value.n ? "on" : "off"); This always sets def_value_str... > + break; > + > + case OPT_NUMBER: > + opts->desc[i].type =3D QEMU_OPT_NUMBER; > + if (list->value.n) { > + opts->desc[i].def_value_str =3D > + g_strdup_printf("%" PRIu64, list->value.n); > + } =2E..whereas this only sets def_value_str for non-zero values. But can't= 0 be a valid setting? Is this mismatch in what gets converted going to bite us? Should you be paying attention to list->assigned instead or in addition to just checking for non-zero values? > + break; > + > + case OPT_SIZE: > + opts->desc[i].type =3D QEMU_OPT_SIZE; > + if (list->value.n) { > + opts->desc[i].def_value_str =3D > + g_strdup_printf("%" PRIu64, list->value.n); > + } Same question for 0 values. > + break; > + > + case OPT_STRING: > + opts->desc[i].type =3D QEMU_OPT_STRING; > + opts->desc[i].def_value_str =3D g_strdup(list->value.s); > + break; > + } > + > + i++; > + list++; > + opts->desc[i].name =3D NULL; =2E..[1] This assignment is dead code, because you used malloc0 which guarantees that desc[i].name is already NULL. > +/* convert QemuOpts to QEMUOptionParamter s/Paramter/Parameter/ > + * Note: result QEMUOptionParameter has shorter lifetime than > + * input QemuOpts. > + * FIXME: this function will be removed after all drivers > + * switch to QemuOpts > + */ > +QEMUOptionParameter *opts_to_params(QemuOpts *opts) > +{ > + num_opts =3D count_opts_list(opts->list); > + dest =3D g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter)); > + > + i++; > + desc++; > + dest[i].name =3D NULL; Another dead assignment. > + } > + > + return dest; > +} > + > +void qemu_opts_free(QemuOptsList *list) > +{ > + /* List members point to new malloced space and need to free. > + * FIXME: > + * Introduced for QEMUOptionParamter->QemuOpts conversion. > + * Will remove after all drivers switch to QemuOpts. > + */ > + if (list && list->mallocd) { > + QemuOptDesc *desc =3D list->desc; > + while (desc && desc->name) { > + g_free((char *)desc->name); > + g_free((char *)desc->help); Are these casts still necessary, given patch 4? > + g_free((char *)desc->def_value_str); However, it looks like you are correct that this one is casting away cons= t. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UWwo1rXxdW3Lj8lmQUjAAfwWshiDaBSuj Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTMfaZAAoJEKeha0olJ0NqjmEH/2qRpgf8GQPDu3WUr+5muMlm 0k5HIHsiak8sWEQtGuidIsVZXP1ufNr2y8ex2Y2QVFWqh4Rye+rf4uKyRpoReGi9 7Ac5J7/JFu4YzUKsNPnLQug8GOkC2JoSCFEAwfpiZNSeQC6Er0zQ6FQec5T1POGW I/CavClyeVY2rEt5gh89KQpjtggj7EdP29DF0yRJhbyRnq8WlF6zu7/Po0j1IkWK iJEvNL02SBxO0BKNIqQQPjcKF8oDXPj53CPWDSQdvhJCC2RXrIk2DUMFpKW0pzBc V8SIB2vvsWDm5YXXlFSOgfa+eBx2CXQg17ZeQ7+jcMUHMGTKmSgowD6oHZVEC2A= =pN2b -----END PGP SIGNATURE----- --UWwo1rXxdW3Lj8lmQUjAAfwWshiDaBSuj--