From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Y6h-0002Bn-JI for qemu-devel@nongnu.org; Mon, 30 Jun 2014 05:42:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1Y6W-0003y1-Iy for qemu-devel@nongnu.org; Mon, 30 Jun 2014 05:42:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59330) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Y6W-0003xR-Ai for qemu-devel@nongnu.org; Mon, 30 Jun 2014 05:42:28 -0400 Date: Mon, 30 Jun 2014 11:42:24 +0200 From: Kevin Wolf Message-ID: <20140630094224.GA4334@noname.str.redhat.com> References: <1403818727-11215-1-git-send-email-mreitz@redhat.com> <1403818727-11215-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1403818727-11215-2-git-send-email-mreitz@redhat.com> 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: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , =?iso-8859-1?Q?Beno=EEt?= Canet 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". 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