From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZX9VU-0006dh-Hr for qemu-devel@nongnu.org; Wed, 02 Sep 2015 10:59:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZX9VQ-0006Kj-Gw for qemu-devel@nongnu.org; Wed, 02 Sep 2015 10:59:24 -0400 References: <1439939415-18180-1-git-send-email-mreitz@redhat.com> <1439939415-18180-4-git-send-email-mreitz@redhat.com> <55E4C062.4010008@redhat.com> From: Max Reitz Message-ID: <55E70EBF.3020808@redhat.com> Date: Wed, 2 Sep 2015 16:59:11 +0200 MIME-Version: 1.0 In-Reply-To: <55E4C062.4010008@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BIC39l1HBvXTFfDVKeQe5N8rp62jimpHS" Subject: Re: [Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --BIC39l1HBvXTFfDVKeQe5N8rp62jimpHS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 31.08.2015 23:00, Eric Blake wrote: > On 08/18/2015 05:10 PM, Max Reitz wrote: >> Split the part which actually refreshes the BlockDriverState.filename >> field off of bdrv_refresh_filename() into a more generic function >> bdrv_filename(), which first calls bdrv_refresh_filename() and then >> stores a qemu-usable filename into the given buffer instead of >> BlockDriverState.filename. >> >> Since bdrv_refresh_filename() therefore no longer refreshes that field= , >> some calls to that function have to be replaced by calls to >> bdrv_filename() "manually" refreshing the BDS filename field (this is >> only temporary). >> >> Additionally, a wrapper function bdrv_filename_alloc() is added which >> allocates a buffer of size PATH_MAX, call bdrv_filename() on that buff= er >> and returns it, since needing a temporary buffer for the filename is a= >> rather common pattern. >> >> Signed-off-by: Max Reitz >> --- >> block.c | 39 ++++++++++++++++++++++++++++++++------- >> block/blkverify.c | 3 ++- >> block/quorum.c | 2 +- >> include/block/block.h | 2 ++ >> 4 files changed, 37 insertions(+), 9 deletions(-) >> >=20 >> +/* First refreshes exact_filename and full_open_options by calling >> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is cop= ied into >> + * the target buffer. Otherwise, full_open_options is converted to a = JSON >> + * object, prefixed with "json:" (for use through the JSON pseudo pro= tocol) and >> + * put there. >> + * >> + * If sz > 0, the string put into the buffer will always be null-term= inated. >> + * >> + * Returns @dest. >> + */ >> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz) >> +{ >=20 > How does one tell if 'sz' was large enough, vs. too short and therefore= > the snprintf() truncated the resulting string? Would the code be any > simpler if this always returned a freshly g_malloc'd string of the righ= t > length, rather than making the caller have to pre-allocate and guess > whether the allocation was big enough? >=20 >> + bdrv_refresh_filename(bs); >> + >> + if (sz > INT_MAX) { >> + sz =3D INT_MAX; >> + } >> =20 >> if (bs->exact_filename[0]) { >> - pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filenam= e); >> + pstrcpy(dest, sz, bs->exact_filename); >> } else if (bs->full_open_options) { >> QString *json =3D qobject_to_json(QOBJECT(bs->full_open_optio= ns)); >> - snprintf(bs->filename, sizeof(bs->filename), "json:%s", >> - qstring_get_str(json)); >> + snprintf(dest, sz, "json:%s", qstring_get_str(json)); >> QDECREF(json); >> } >> + >> + return dest; >=20 > In other words, I think it's very dangerous to use snprintf() without > checking whether the result fit. There are a couple of places in qemu which copy BDS.filename into a pre-existing buffer, so I'd rather not just drop bdrv_filename() as it is= =2E I guess I'll just make it so that calling bdrv_filename() with a NULL dest will result in the buffer being allocated. Note however that there are a couple of places in qemu which rely on filenames not exceeding PATH_MAX (by using PATH_MAX sized buffers for holding them). Maybe we'll eventually get around to fix them, but right now it's a limitation not introduced by this series. Max --BIC39l1HBvXTFfDVKeQe5N8rp62jimpHS Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJV5w6/AAoJEDuxQgLoOKytnWMIAIGxn4UvCWeGOVzl5xqL5sly tV2zQb2czo94FsBbHwN4Q2Peax722utUM5ThQaVOfYTLNv/8EbOMW18Stf9YQsU/ nXIOHyTWICL0H7wIYEHCSxlIT45lsKOr9U0x+SvxFNpweuM7F+pD3+44SAu7NrRN guf1n7hlCnaYcZWP8eKGxR11V3DV5sMWb3fJ6KIxeK9L1umJ9NiC4C/bXLxDXHlJ S430z2YSsvGW2LO5AMdbs700ve3WibE/jqdUy8doA/GdKlpyPms3DqJU22bnsfEe TGTIQxtyXneBuHOV2vFERXr8z3pbVBtwMvdJSzweyrRM9Pm8Q14dbXU9acq/zKw= =s732 -----END PGP SIGNATURE----- --BIC39l1HBvXTFfDVKeQe5N8rp62jimpHS--