* Re: [Qemu-devel] [Qemu-block] [PATCHv2 for-2.4] block: Auto-generate node_names for each BDS entry
2015-06-22 23:10 [Qemu-devel] [PATCHv2 for-2.4] block: Auto-generate node_names for each BDS entry Eric Blake
@ 2015-06-23 7:14 ` Alberto Garcia
2015-06-23 9:00 ` [Qemu-devel] " Markus Armbruster
1 sibling, 0 replies; 5+ messages in thread
From: Alberto Garcia @ 2015-06-23 7:14 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, open list:Block layer core
On Tue 23 Jun 2015 01:10:39 AM CEST, Eric Blake <eblake@redhat.com> wrote:
> +#define GEN_NODE_NAME_PREFIX "__qemu##"
> +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> static void bdrv_assign_node_name(BlockDriverState *bs,
> const char *node_name,
> Error **errp)
> {
> + char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> + static uint32_t counter; /* simple counter to guarantee uniqueness */
Any reason why you use uint32_t instead of just unsigned ?
> - if (!id_wellformed(node_name)) {
> + int len;
> + snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> + "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> + len = strlen(gen_node_name);
No need to use strlen() here, snprintf() already returns the number of
characters written.
> + while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> + gen_node_name[len++] = g_random_int_range('A', 'Z');
> + }
I think this doesn't include the 'Z', the upper bound of the interval is
open.
Berto
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCHv2 for-2.4] block: Auto-generate node_names for each BDS entry
2015-06-22 23:10 [Qemu-devel] [PATCHv2 for-2.4] block: Auto-generate node_names for each BDS entry Eric Blake
2015-06-23 7:14 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
@ 2015-06-23 9:00 ` Markus Armbruster
2015-06-23 9:31 ` Paolo Bonzini
1 sibling, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2015-06-23 9:00 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Paolo Bonzini, Jeff Cody, qemu-devel,
open list:Block layer core
Copying Paolo for advice on the more general problem.
Can you explain why we need this in 2.4?
Eric Blake <eblake@redhat.com> writes:
> From: Jeff Cody <jcody@redhat.com>
>
> 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.
This is how we do user-specified IDs everywhere.
There are some exceptions for backward compatibility, where we make up
IDs when the user doesn't. Those made-up IDs can clash with the user's,
so the user needs to either refrain from using IDs that could clash, or
take care to suppress them by specifying his own. For the former, you
need to know all the made-up ID formats. For the latter, you either
have to know exactly when you have to suppress a made-up ID, or you have
to specify IDs always, without fail. That's libvirt's strategy.
Reliable as long as things don't acquire IDs later on, with made-up IDs
that can clash. If we did that, then old libvirt won't suppress the
made-up IDs, because it has no idea it could specify one.
> 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 resolving filenames
> (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> qemu currently needs to deal with.
Let me rephrase the problem statement.
The user has to specify the object's ID to conveniently refer to it in
later commands. If he neglects to specify one, referring to the object
in later commands becomes awkward or impossible, depending on the
command.
Example: command FROB lets you specify the node to frobnicate either by
parameter NODE-NAME or by parameter FILE. Use of NODE-NAME is obvious
and unambiguous, but only possible when the user specified one. Else,
you have to use the old and flawed FILE interface, which less obvious
and sometimes ambiguous: FILE is matched against file names, and file
names need not be unique.
(I lack the time today to find an actual command, so I made one up).
Example: command device_del lets you specified the device to be deleted
by its ID. If the user neglected to specify one, you can't delete it.
My point is: the problem is more general than just block nodes. Doesn't
mean we mustn't solve the special problem unless we solve the general
problem. Does mean we should at least try to solve the general problem,
and if we fail, try our best to solve the special problem in a way that
fits into a general solution.
Have you tried to solve or at least discuss the general problem?
> 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.
I believe this is a good deal more complicated than need be.
The user's IDs (and this includes node names) have to satisfy
id_wellformed(): letters, digits, '-', '.', '_', starting with a letter.
Thus, a made-up ID starting with '_' cannot clash. Instead of an
elaborately decorated (and hard to type) __qemu##00000002METXTRBQ simple
_2 would do.
If you're less laconically minded than I am, pick something in between.
But please spare me strings of randome letters :)
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Message-Id: <9516d2684d419e1bfa2a95f66d2e9a70a4ff7eb7.1403723862.git.jcody@redhat.com>
> [id_wellformed() rejects generated names, which means we can't collide]
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> v2: revive this patch; very little needed changing
>
> I just posted a libvirt patch series that depends on this patch:
> https://www.redhat.com/archives/libvir-list/2015-June/msg01111.html
>
> As the original was posted by Jeff (nearly a year ago!), it counts
> as a patch that was submitted prior to soft freeze :)
>
> [I will be on vacation the next 3 weeks, so I may be slow to respond
> on replies to this message]
>
> block.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index dd4f58d..2532b70 100644
> --- a/block.c
> +++ b/block.c
> @@ -765,16 +765,28 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> return open_flags;
> }
>
> +#define GEN_NODE_NAME_PREFIX "__qemu##"
> +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> static void bdrv_assign_node_name(BlockDriverState *bs,
> const char *node_name,
> Error **errp)
> {
> + char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> + static uint32_t counter; /* simple counter to guarantee uniqueness */
> +
> + /* if node_name is NULL, auto-generate a node name */
> if (!node_name) {
> - return;
> - }
> -
> - /* Check for empty string or invalid characters */
> - if (!id_wellformed(node_name)) {
> + int len;
> + snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> + "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> + len = strlen(gen_node_name);
> + while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> + gen_node_name[len++] = g_random_int_range('A', 'Z');
> + }
> + gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
> + node_name = gen_node_name;
> + } else if (!id_wellformed(node_name)) {
> + /* Check for empty string or invalid characters */
> error_setg(errp, "Invalid node name");
> return;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread