From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36244) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1wW8-0008L4-Ex for qemu-devel@nongnu.org; Tue, 01 Jul 2014 07:46:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1wW2-0000si-0A for qemu-devel@nongnu.org; Tue, 01 Jul 2014 07:46:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9273) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1wW1-0000rn-Mw for qemu-devel@nongnu.org; Tue, 01 Jul 2014 07:46:25 -0400 Message-ID: <53B29F7D.2000109@redhat.com> Date: Tue, 01 Jul 2014 13:46:05 +0200 From: Max Reitz MIME-Version: 1.0 References: <1403818727-11215-1-git-send-email-mreitz@redhat.com> <1403818727-11215-2-git-send-email-mreitz@redhat.com> <20140630094224.GA4334@noname.str.redhat.com> In-Reply-To: <20140630094224.GA4334@noname.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/2] block: Do not prematurely remove "filename" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , =?ISO-8859-1?Q?Beno=EEt_Canet?= On 30.06.2014 11:42, Kevin Wolf wrote: > Am 26.06.2014 um 23:38 hat Max Reitz geschrieben: >> If "filename" is removed from the options QDict before entering >> bdrv_open_common(), it cannot be stored in the BDS. Therefore, wait >> until it has been copied there and remove it from the options only >> afterwards. >> >> This fixes "filename" in the BDS being empty for block drivers which do >> not need the filename because they have parsed it already (e.g. NBD). >> >> Signed-off-by: Max Reitz > I can't say I like this approach. It looks a bit odd to pass a boolean > variable to bdrv_open(), and in some other function called from there > the cleanup is done that logically really belong to bdrv_fill_options(). > > More importantly, the goal was to get rid of the filename and handle > everything through the options so that we get a uniform state again. > This would involve replacing bs->filename by a new callback function in > BlockDriver that constructs a filename that describes the BDS. This way > we would get useful output not only for "nbd:localhost:10809", but also > for "driver=nbd,host=localhost". Right. This "bug" isn't that severe either, so I guess it's fine to take more time to fix the general problem. Max > In hard cases, the callback might just use "json:{...}" syntax. This > suggests that maybe in the end we'll want to have two different > callbacks, one giving a short human-readable description > ('localhost:10809') and another one giving something that can be used on > the command line ('json:{"driver": "nbd", "host": "localhost", "ipv6": > true}'). > > Kevin