From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNEZw-0005Rc-W2 for qemu-devel@nongnu.org; Tue, 11 Mar 2014 00:46:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNEZs-0002c1-Hn for qemu-devel@nongnu.org; Tue, 11 Mar 2014 00:46:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNEZs-0002br-8n for qemu-devel@nongnu.org; Tue, 11 Mar 2014 00:46:08 -0400 Message-ID: <531E950B.5010402@redhat.com> Date: Mon, 10 Mar 2014 22:46:03 -0600 From: Eric Blake MIME-Version: 1.0 References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-7-git-send-email-cyliu@suse.com> In-Reply-To: <1394436721-21812-7-git-send-email-cyliu@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QjaRsrjA3qsw0IWgEKlFmurIluiWsrMxl" Subject: Re: [Qemu-devel] [PATCH v22 06/25] 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: kwolf@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --QjaRsrjA3qsw0IWgEKlFmurIluiWsrMxl Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/10/2014 01:31 AM, Chunyan Liu wrote: > Add two temp convert functions between QEMUOptionParameter to QemuOpts,= so that > next patch can use it. It will simplify next patch for easier review. >=20 > Signed-off-by: Chunyan Liu > --- > include/qemu/option.h | 2 + > util/qemu-option.c | 105 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > 2 files changed, 107 insertions(+) >=20 > + > +/* convert QEMUOptionParameter to QemuOpts */ > +QemuOptsList *params_to_opts(QEMUOptionParameter *list) > +{ > + QemuOptsList *opts =3D NULL; > + size_t num_opts, i =3D 0; > + > + if (!list) { > + return NULL; > + } > + > + num_opts =3D count_option_parameters(list); > + opts =3D g_malloc0(sizeof(QemuOptsList) + > + (num_opts + 1) * sizeof(QemuOptDesc)); > + QTAILQ_INIT(&opts->head); > + opts->desc[i].name =3D NULL; Dead assignment to NULL, thanks to the g_malloc0 above. > + > + 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 list->value.n ? "on" : "of= f"; Ouch. The pointer used here points to constant memory... > + 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..while the pointer used here points to heap memory. Yet I see nothin= g in the struct that you use to track whether to free the string or not, which means this is more likely a memory leak, but also a potential for a crash during an errant call to g_free(). You absolutely must manage the memory correctly, if you are going to conditionally heap-allocate def_value_str. For the sake of conversions between the two types, may I suggest an idea:= in 'struct QemuOpt', add a char[24] buffer (that's large enough to hold the maximum stringized uint value). Then, instead of malloc'ing memory with g_strdup_printf, you instead format integers in place. You've already malloc'd the size for the QemuOpt, and now the string representation also fits within that space without needing a secondary malloc. Whether or not you end up keeping the stringizing buffer permanently part of QemuOpt, or delete it after you delete QEMUOptionParameter, remains to be seen at the end of the series. > + case OPT_STRING: > + opts->desc[i].type =3D QEMU_OPT_STRING; > + opts->desc[i].def_value_str =3D g_strdup(list->value.s); Here, just declare that your temporary QemuOptsList result from the function has a shorter lifetime than the input QemuOptionParameter, and set the def_value_str pointer to the original list->value.s instead of duplicating it. Then, you are back to the situation where freeing the temporary QemuOptsList doesn't leak and doesn't risk double frees. > +QEMUOptionParameter *opts_to_params(QemuOpts *opts) > +{ > + QEMUOptionParameter *dest =3D NULL; > + QemuOptDesc *desc; > + size_t num_opts, i =3D 0; > + const char *tmp; > + > + if (!opts || !opts->list || !opts->list->desc) { > + return NULL; > + } > + > + num_opts =3D count_opts_list(opts->list); > + dest =3D g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter)); > + dest[i].name =3D NULL; > + > + desc =3D opts->list->desc; > + while (desc && desc->name) { > + dest[i].name =3D g_strdup(desc->name); > + dest[i].help =3D g_strdup(desc->help); I didn't research QEMUOptionParameter close enough to know if you will be causing any memory leaks on the reverse conversion - but since the end goal of the series is to delete QEMUOptionParameter, this method will eventually disappear, so I guess I could live with leaks here (although it would be nice to document it in the commit message, if you do indeed leak). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --QjaRsrjA3qsw0IWgEKlFmurIluiWsrMxl 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/ iQEcBAEBCAAGBQJTHpULAAoJEKeha0olJ0NqEUMIAKflkITKwhjiK1+qbSRiZ48o af17sPFyP0/j8wEmh4Ux/MnEAjQsmV8ILP+jStBj/Ttjw7e0ndVXLynoXqQcXrjE 8AbhV4YtF8A3lxsyN/C5m/Y3M6rwzx7qXO1KIWdEa22KJQJxsnsalJUboNGVVR3y 3Ilsghrqd834bOm84e/nVnx6hultiuvycQrGHE85do0ugwOLB2RQptIIUW+Kat7g b7n2k5fq9ekBTzqyE5pqaPdqN/IED96jahks3zOf2VBtV/hB/3XPRoGuQiDRfbpM KelQ7qmtC36R8N2Bz1Zwkm8qbCzkNewl0NvbibRoCPpP/OS/fNKdPJWUvgDc88s= =xtAk -----END PGP SIGNATURE----- --QjaRsrjA3qsw0IWgEKlFmurIluiWsrMxl--