From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W1gwt-00052j-Dk for qemu-devel@nongnu.org; Fri, 10 Jan 2014 13:36:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W1gwn-0007fb-Dy for qemu-devel@nongnu.org; Fri, 10 Jan 2014 13:36:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W1gwn-0007fV-5P for qemu-devel@nongnu.org; Fri, 10 Jan 2014 13:36:45 -0500 Message-ID: <52D03E1A.20709@redhat.com> Date: Fri, 10 Jan 2014 19:38:18 +0100 From: Max Reitz MIME-Version: 1.0 References: <1389210205-10787-1-git-send-email-peter@gridcentric.ca> <20140109105930.GA2862@dhcp-200-207.str.redhat.com> <52D02D96.3010901@redhat.com> <20140110175518.GG4276@dhcp-200-207.str.redhat.com> <52D03651.5010603@redhat.com> <20140110182654.GH4276@dhcp-200-207.str.redhat.com> In-Reply-To: <20140110182654.GH4276@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: fix backing file segfault List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: famz@redhat.com, Peter Feiner , qemu-devel@nongnu.org On 10.01.2014 19:26, Kevin Wolf wrote: > Am 10.01.2014 um 19:05 hat Max Reitz geschrieben: >> On 10.01.2014 18:55, Kevin Wolf wrote: >>> Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-b= y >>> there? >> Yes, feel free to. > Thanks, applied to the block branch. > > Peter, no need for a second version of the patch then. :-) > >>> In the long run, we need to get rid of all this copying anyway. I'm >>> imagining a BlockDriver function that returns a file name to reproduc= e >>> the same setup, and a removal of bs->backing_file and bs->file_name. >>> >>> For some drivers, the returned filename would be a URL or some other >>> string that that particular driver can parse. >>> >>> While doing that, we might also consider a fake protocol that handles >>> filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}', >>> because for some drivers this might be the only thing that comes clos= e >>> to a filename as it is a single string at least... >> Urgh. *g* >> >> I'm not sure if we should force every BDS to have a clearly defining >> file name. If there are options, which completely change the >> behavior of the block driver (I wouldn't consider lazy-refcounts one >> of them since it doesn't change the contents of the block device), >> I'd rather return NULL when asked for a file name. But then again, >> maybe an ugly filename is better than none at all=E2=80=A6 > We need filenames for backing file relationships. For example, when you > take a live snapshot, we need to reference the old image. If you don't > use the filename, but driver-specific options, I believe this fails > today. > > You might also want to set some options for the backing file in images > created with qemu-img. Yes, I hoped we could use the options instead. But if it fails=E2=80=A6 M= aybe=20 it's worth fixing, I don't know. ;-) > The alternative would be to extend qcow2 to have something more complex > than a string to describe backing files. However, this would mean that > qcow2 is the only possible format for live snapshots. Well, the problem would arise only for backing files which can't be=20 sufficiently described through a rather simple filename. If there are=20 exceptions where we are indeed forced to specify some options, qemu=20 would be the only program knowing how to interpret those filenames=20 anyway, therefore, there is no point in trying to be compatible. Max > >> In general, I'd prefer abandoning filenames* (especially protocol >> filenames) altogether. The set of options with which to recreate the >> same BDS is already available. >> >> Max >> >> *Of course, we need filenames for, well, opening files, but I'm >> referring to have an explicit string "filename" in addition to the >> option dicts (nearly) everywhere. > Agreed. The reason why filenames are still passed separately and not > converted to a file.filename QDict entry is the convenience magic that > they enable (at least protocol names) and that file.filename doesn't > have in order to have less special cases with blockdev-add. > > So what you'd need to do is to parse the protocol names in the top-leve= l > function bdrv_open() and convert them into the right QDict entries. > Perhaps this is also a better place for the .bdrv_parse_filename() > calls. And then you could call a new bdrv_file_open() that doesn't take > a separate filename argument any more. > > Kevin