From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wl0ag-0007cT-UH for qemu-devel@nongnu.org; Thu, 15 May 2014 14:41:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wl0aa-0004xu-PE for qemu-devel@nongnu.org; Thu, 15 May 2014 14:41:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12027) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wl0aa-0004xn-Ho for qemu-devel@nongnu.org; Thu, 15 May 2014 14:41:08 -0400 Date: Thu, 15 May 2014 14:41:03 -0400 From: Jeff Cody Message-ID: <20140515184103.GM8452@localhost.localdomain> References: <5374E445.9070609@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5374E445.9070609@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, benoit.canet@irqsave.net, pkrempa@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Thu, May 15, 2014 at 09:59:01AM -0600, Eric Blake wrote: > On 05/14/2014 09:20 PM, Jeff Cody wrote: > > Currently, node_name is only filled in when done so explicitly by the > > user. If no node_name is specified, then the node name field is not > > populated. > > > > If node_names are automatically generated when not specified, that means > > that all block job operations can be done by reference to the unique > > node_name field. This eliminates ambiguity in filename pathing > > (relative filenames, or file descriptors, symlinks, mounts, etc..) that > > qemu currently needs to deal with. > > > > If a node name is specified, then it will not be automatically > > generated for that BDS entry. > > > > If it is automatically generated, it will be prefaced with "__qemu##", > > followed by 8 characters of a unique number, followed by 8 random > > ASCII characters in the range of 'A-Z'. Some sample generated node-name > > strings: > > __qemu##00000000IAIYNXXR > > __qemu##00000002METXTRBQ > > __qemu##00000001FMBORDWG > > > > The prefix is to aid in identifying it as a qemu-generated name, the > > numeric portion is to guarantee uniqueness in a given qemu session, and > > the random characters are to further avoid any accidental collisions > > with user-specified node-names. > > > > Signed-off-by: Jeff Cody > > --- > > block.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > This patch feels incomplete. We probably also ought to modify > qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by > unconditional (as an output-only struct, changing from optional to > mandatory is a safe change for back-compat API considerations); which > implies further modifying code to get rid of has_node_name arguments in > all places that generate BlockDeviceInfo details. I think it should be a separate patch (but still in this series). Strictly speaking, this patch doesn't force the argument to be mandatory, the value just happens to always be populated now. > See also my thoughts on patch 5/5 - can this patch be rebased to be LAST > in the series, rather than first, so that it serves as a reliable > witness that everything else related to block operations on node-names > is complete? > How about this becomes the penultimate patch, and the patch to make BlockDeviceInfo's node-name field to be unconditional becomes the last patch? The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending. So let me revise my above statement - how about this patch stays at the beginning, which makes development / testing easier, with the patch that modifies BlockDeviceInfo as the final (not including tests) patch?