From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1csq3T-0001up-6w for qemu-devel@nongnu.org; Tue, 28 Mar 2017 08:16:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1csq3P-00047s-VE for qemu-devel@nongnu.org; Tue, 28 Mar 2017 08:16:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49076) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1csq3P-00047o-Mp for qemu-devel@nongnu.org; Tue, 28 Mar 2017 08:16:51 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A76A43D965 for ; Tue, 28 Mar 2017 12:16:50 +0000 (UTC) Date: Tue, 28 Mar 2017 08:16:49 -0400 From: Jeff Cody Message-ID: <20170328121649.GI22342@localhost.localdomain> References: <1490621195-2228-1-git-send-email-armbru@redhat.com> <1490621195-2228-3-git-send-email-armbru@redhat.com> <87wpbaefzo.fsf@dusky.pond.sub.org> <8737dybl7v.fsf@dusky.pond.sub.org> <20170327213347.GF22342@localhost.localdomain> <87r31h6dki.fsf@dusky.pond.sub.org> <20170328115606.GC1135@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170328115606.GC1135@localhost.localdomain> Subject: Re: [Qemu-devel] [PATCH RFC v3 for-2.9 02/11] rbd: Fix to cleanly reject -drive without pool or image List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, jdurgin@redhat.com, qemu-devel@nongnu.org, Max Reitz On Tue, Mar 28, 2017 at 07:56:06AM -0400, Jeff Cody wrote: > On Tue, Mar 28, 2017 at 09:54:53AM +0200, Markus Armbruster wrote: > > Jeff Cody writes: > > > > > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote: > > >> Markus Armbruster writes: > > >> > > >> > Max Reitz writes: > > >> > > > >> >> On 27.03.2017 18:10, Max Reitz wrote: > > >> >>> On 27.03.2017 15:26, Markus Armbruster wrote: > > >> >>>> qemu_rbd_open() neglects to check pool and image are present. > > >> >>>> Reproducer: > > >> >>>> > > >> >>>> $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p > > >> >>>> Segmentation fault (core dumped) > > >> >>>> $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i > > >> >>>> qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool (null) > > >> >>>> > > >> >>>> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename() > > >> >>>> always sets both pool and image. > > >> >>>> > > >> >>>> Doesn't affect -blockdev, because pool and image are mandatory in the > > >> >>>> QAPI schema. > > >> >>>> > > >> >>>> Fix by adding the missing checks. > > >> >>>> > > >> >>>> Signed-off-by: Markus Armbruster > > >> >>>> Reviewed-by: Eric Blake > > >> >>>> --- > > >> >>>> block/rbd.c | 10 +++++++--- > > >> >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > > >> >>> > > >> >>> Reviewed-by: Max Reitz > > >> >> > > >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too > > >> >> doesn't check whether those options are given but I guess they're just > > >> >> as mandatory. > > >> > > > >> > Looks like it. I'll try to stick a fix into v4. > > >> > > >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going > > >> through qemu_rbd_parse_filename()? > > > > > > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call > > > qemu_rbd_parse_filename(). And in qemu_rbd_parse_filename(), it will > > > complain if pool is not provided (and that is what causes the abort, not the > > > missing image parameter). So I think we are safe, but a nicer error message > > > for a missing 'image' parameter might be nice anyway. > > > > I now see the "we are safe" part, but not the "want a nicer error > > message for a missing 'image'" part. How can 'image' be missing? > > > > qemu_rbd_parse_filename() parses a pseudo-filename of the form > > > > rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...] > > > > It fails if > > > > * the pseudo-filename doesn't start with "rbd:" > > > > * doesn't contain '/' ("Pool name is required") > > > > * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or > > too long (until the next commit) > > > > * a KEY=VALUE doesn't contain '=' > > > > If it succeeds, "pool" and "image" are both set in @options. > > > > Can you give me a reproducer for the error message you'd like me to > > improve? > > > Sure: qemu-img info rbd:mypool/:mon_host=192.168.15.180 > > 'image' is technically present in the options, but it is an empty string. > > My thought was that it'd be nice to throw a similar error message for an > empty string for 'image'. As it is, the rbd library catches it, so it isn't > catastrophic, but a nicer message would be helpful. > > (My comment here assumes an empty string is not an acceptable image name for > rbd) I'm not concerned enough by this to want a v5. I wasn't clear about that. -Jeff