| Message ID | 0e01e250855f841e56481fe690859bd65e667c4f.1444691409.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
Jeff Cody <jcody@redhat.com> writes: > Multiple sub-systems in QEMU may find it useful to generate IDs > for objects that a user may reference via QMP or HMP. This patch > presents a standardized way to do it, so that automatic ID generation > follows the same rules. > > This patch enforces the following rules when generating an ID: > > 1.) Guarantee no collisions with a user-specified ID > 2.) Identify the sub-system the ID belongs to > 3.) Guarantee of uniqueness > 4.) Spoiling predictability, to avoid creating an assumption > of object ordering and parsing (i.e., we don't want users to think > they can guess the next ID based on prior behavior). > > The scheme for this is as follows (no spaces): > > # subsys D RR > Reserved char --| | | | > Subsystem String ----| | | > Unique number (64-bit) --| | > Two-digit random number ---| > > For example, a generated node-name for the block sub-system may look > like this: > > #block076 > > The caller of id_generate() is responsible for freeing the generated > node name string with g_free(). > > Reviewed-by: John Snow <jsnow@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > include/qemu-common.h | 8 ++++++++ > util/id.c | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/include/qemu-common.h b/include/qemu-common.h > index 0bd212b..2f74540 100644 > --- a/include/qemu-common.h > +++ b/include/qemu-common.h > @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, > #define STR_OR_NULL(str) ((str) ? (str) : "null") > > /* id.c */ > + > +typedef enum IdSubSystems { > + ID_QDEV, ID_QDEV is not used in this series. Do you intend to use it in a followup-series? Can we reasonably expect that series will be accepted? You could sidestep these questions by making id_generate() take a string argument ;) > + ID_BLOCK, > + ID_MAX /* last element, used as array size */ > +} IdSubSystems; > + > +char *id_generate(IdSubSystems id); > bool id_wellformed(const char *id); > > /* path.c */ [...]
On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > > > Multiple sub-systems in QEMU may find it useful to generate IDs > > for objects that a user may reference via QMP or HMP. This patch > > presents a standardized way to do it, so that automatic ID generation > > follows the same rules. > > > > This patch enforces the following rules when generating an ID: > > > > 1.) Guarantee no collisions with a user-specified ID > > 2.) Identify the sub-system the ID belongs to > > 3.) Guarantee of uniqueness > > 4.) Spoiling predictability, to avoid creating an assumption > > of object ordering and parsing (i.e., we don't want users to think > > they can guess the next ID based on prior behavior). > > > > The scheme for this is as follows (no spaces): > > > > # subsys D RR > > Reserved char --| | | | > > Subsystem String ----| | | > > Unique number (64-bit) --| | > > Two-digit random number ---| > > > > For example, a generated node-name for the block sub-system may look > > like this: > > > > #block076 > > > > The caller of id_generate() is responsible for freeing the generated > > node name string with g_free(). > > > > Reviewed-by: John Snow <jsnow@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > include/qemu-common.h | 8 ++++++++ > > util/id.c | 37 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/include/qemu-common.h b/include/qemu-common.h > > index 0bd212b..2f74540 100644 > > --- a/include/qemu-common.h > > +++ b/include/qemu-common.h > > @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, > > #define STR_OR_NULL(str) ((str) ? (str) : "null") > > > > /* id.c */ > > + > > +typedef enum IdSubSystems { > > + ID_QDEV, > > ID_QDEV is not used in this series. Do you intend to use it in a > followup-series? Can we reasonably expect that series will be accepted? > John Arbuckle has a patch on list that uses it. I haven't reviewed it, however - but I guess it depends ultimately on whether qdev will allow autogeneration for its IDs or not. > You could sidestep these questions by making id_generate() take a string > argument ;) > I'd rather avoid having each system specifying a string inline in their code. It is cleaner to have the strings defined in a central location, I think (not to mention, easier to reference). > > + ID_BLOCK, > > + ID_MAX /* last element, used as array size */ > > +} IdSubSystems; > > + > > +char *id_generate(IdSubSystems id); > > bool id_wellformed(const char *id); > > > > /* path.c */ > [...]
Jeff Cody <jcody@redhat.com> writes: > On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote: >> Jeff Cody <jcody@redhat.com> writes: >> >> > Multiple sub-systems in QEMU may find it useful to generate IDs >> > for objects that a user may reference via QMP or HMP. This patch >> > presents a standardized way to do it, so that automatic ID generation >> > follows the same rules. >> > >> > This patch enforces the following rules when generating an ID: >> > >> > 1.) Guarantee no collisions with a user-specified ID >> > 2.) Identify the sub-system the ID belongs to >> > 3.) Guarantee of uniqueness >> > 4.) Spoiling predictability, to avoid creating an assumption >> > of object ordering and parsing (i.e., we don't want users to think >> > they can guess the next ID based on prior behavior). >> > >> > The scheme for this is as follows (no spaces): >> > >> > # subsys D RR >> > Reserved char --| | | | >> > Subsystem String ----| | | >> > Unique number (64-bit) --| | >> > Two-digit random number ---| >> > >> > For example, a generated node-name for the block sub-system may look >> > like this: >> > >> > #block076 >> > >> > The caller of id_generate() is responsible for freeing the generated >> > node name string with g_free(). >> > >> > Reviewed-by: John Snow <jsnow@redhat.com> >> > Reviewed-by: Eric Blake <eblake@redhat.com> >> > Reviewed-by: Alberto Garcia <berto@igalia.com> >> > Signed-off-by: Jeff Cody <jcody@redhat.com> >> > --- >> > include/qemu-common.h | 8 ++++++++ >> > util/id.c | 37 +++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 45 insertions(+) >> > >> > diff --git a/include/qemu-common.h b/include/qemu-common.h >> > index 0bd212b..2f74540 100644 >> > --- a/include/qemu-common.h >> > +++ b/include/qemu-common.h >> > @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, >> > #define STR_OR_NULL(str) ((str) ? (str) : "null") >> > >> > /* id.c */ >> > + >> > +typedef enum IdSubSystems { >> > + ID_QDEV, >> >> ID_QDEV is not used in this series. Do you intend to use it in a >> followup-series? Can we reasonably expect that series will be accepted? >> > > John Arbuckle has a patch on list that uses it. I haven't reviewed > it, however - but I guess it depends ultimately on whether qdev will > allow autogeneration for its IDs or not. Then that patch should add ID_QDEV. >> You could sidestep these questions by making id_generate() take a string >> argument ;) >> > > I'd rather avoid having each system specifying a string inline in > their code. It is cleaner to have the strings defined in a central > location, I think (not to mention, easier to reference). Covered by your artistic license :) >> > + ID_BLOCK, >> > + ID_MAX /* last element, used as array size */ >> > +} IdSubSystems; >> > + >> > +char *id_generate(IdSubSystems id); >> > bool id_wellformed(const char *id); >> > >> > /* path.c */ >> [...]
On 10/13/2015 11:26 AM, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > >> On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote: >>> Jeff Cody <jcody@redhat.com> writes: >>> >>>> Multiple sub-systems in QEMU may find it useful to generate IDs >>>> for objects that a user may reference via QMP or HMP. This patch >>>> presents a standardized way to do it, so that automatic ID generation >>>> follows the same rules. >>>> >>>> This patch enforces the following rules when generating an ID: >>>> >>>> 1.) Guarantee no collisions with a user-specified ID >>>> 2.) Identify the sub-system the ID belongs to >>>> 3.) Guarantee of uniqueness >>>> 4.) Spoiling predictability, to avoid creating an assumption >>>> of object ordering and parsing (i.e., we don't want users to think >>>> they can guess the next ID based on prior behavior). >>>> >>>> The scheme for this is as follows (no spaces): >>>> >>>> # subsys D RR >>>> Reserved char --| | | | >>>> Subsystem String ----| | | >>>> Unique number (64-bit) --| | >>>> Two-digit random number ---| >>>> >>>> For example, a generated node-name for the block sub-system may look >>>> like this: >>>> >>>> #block076 >>>> >>>> The caller of id_generate() is responsible for freeing the generated >>>> node name string with g_free(). >>>> >>>> Reviewed-by: John Snow <jsnow@redhat.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>>> Signed-off-by: Jeff Cody <jcody@redhat.com> >>>> --- >>>> include/qemu-common.h | 8 ++++++++ >>>> util/id.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 45 insertions(+) >>>> >>>> diff --git a/include/qemu-common.h b/include/qemu-common.h >>>> index 0bd212b..2f74540 100644 >>>> --- a/include/qemu-common.h >>>> +++ b/include/qemu-common.h >>>> @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, >>>> #define STR_OR_NULL(str) ((str) ? (str) : "null") >>>> >>>> /* id.c */ >>>> + >>>> +typedef enum IdSubSystems { >>>> + ID_QDEV, >>> >>> ID_QDEV is not used in this series. Do you intend to use it in a >>> followup-series? Can we reasonably expect that series will be accepted? >>> >> >> John Arbuckle has a patch on list that uses it. I haven't reviewed >> it, however - but I guess it depends ultimately on whether qdev will >> allow autogeneration for its IDs or not. > > Then that patch should add ID_QDEV. > >>> You could sidestep these questions by making id_generate() take a string >>> argument ;) >>> >> >> I'd rather avoid having each system specifying a string inline in >> their code. It is cleaner to have the strings defined in a central >> location, I think (not to mention, easier to reference). > > Covered by your artistic license :) > I think our engineering license would have us do it the way Jeff already is -- If we want a central unique ID generator utility, it would be best to disallow different areas of code from specifying their own IDs ... it's bound to lead to collisions and heartbreak someday. Easiest to just keep an enum and if you decide you need to use this facility, add your name to the Registry Of Accepted Subcomponents and keep moving. >>>> + ID_BLOCK, >>>> + ID_MAX /* last element, used as array size */ >>>> +} IdSubSystems; >>>> + >>>> +char *id_generate(IdSubSystems id); >>>> bool id_wellformed(const char *id); >>>> >>>> /* path.c */ >>> [...]
On Oct 13, 2015, at 11:26 AM, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > >> On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote: >>> Jeff Cody <jcody@redhat.com> writes: >>> >>>> Multiple sub-systems in QEMU may find it useful to generate IDs >>>> for objects that a user may reference via QMP or HMP. This patch >>>> presents a standardized way to do it, so that automatic ID generation >>>> follows the same rules. >>>> >>>> This patch enforces the following rules when generating an ID: >>>> >>>> 1.) Guarantee no collisions with a user-specified ID >>>> 2.) Identify the sub-system the ID belongs to >>>> 3.) Guarantee of uniqueness >>>> 4.) Spoiling predictability, to avoid creating an assumption >>>> of object ordering and parsing (i.e., we don't want users to think >>>> they can guess the next ID based on prior behavior). >>>> >>>> The scheme for this is as follows (no spaces): >>>> >>>> # subsys D RR >>>> Reserved char --| | | | >>>> Subsystem String ----| | | >>>> Unique number (64-bit) --| | >>>> Two-digit random number ---| >>>> >>>> For example, a generated node-name for the block sub-system may look >>>> like this: >>>> >>>> #block076 >>>> >>>> The caller of id_generate() is responsible for freeing the generated >>>> node name string with g_free(). >>>> >>>> Reviewed-by: John Snow <jsnow@redhat.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>>> Signed-off-by: Jeff Cody <jcody@redhat.com> >>>> --- >>>> include/qemu-common.h | 8 ++++++++ >>>> util/id.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 45 insertions(+) >>>> >>>> diff --git a/include/qemu-common.h b/include/qemu-common.h >>>> index 0bd212b..2f74540 100644 >>>> --- a/include/qemu-common.h >>>> +++ b/include/qemu-common.h >>>> @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, >>>> #define STR_OR_NULL(str) ((str) ? (str) : "null") >>>> >>>> /* id.c */ >>>> + >>>> +typedef enum IdSubSystems { >>>> + ID_QDEV, >>> >>> ID_QDEV is not used in this series. Do you intend to use it in a >>> followup-series? Can we reasonably expect that series will be accepted? >>> >> >> John Arbuckle has a patch on list that uses it. I haven't reviewed >> it, however - but I guess it depends ultimately on whether qdev will >> allow autogeneration for its IDs or not. > > Then that patch should add ID_QDEV. > >>> You could sidestep these questions by making id_generate() take a string >>> argument ;) >>> >> >> I'd rather avoid having each system specifying a string inline in >> their code. It is cleaner to have the strings defined in a central >> location, I think (not to mention, easier to reference). I can see the benefit of using a string. The id_generate() function could use va_args like printf() uses to allow almost any kind of string argument. An empty string argument could mean to default to ID_MAX. But I also think using an enumeration is good enough, so either way is good.
diff --git a/include/qemu-common.h b/include/qemu-common.h index 0bd212b..2f74540 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end, #define STR_OR_NULL(str) ((str) ? (str) : "null") /* id.c */ + +typedef enum IdSubSystems { + ID_QDEV, + ID_BLOCK, + ID_MAX /* last element, used as array size */ +} IdSubSystems; + +char *id_generate(IdSubSystems id); bool id_wellformed(const char *id); /* path.c */ diff --git a/util/id.c b/util/id.c index 09b22fb..bcc64d8 100644 --- a/util/id.c +++ b/util/id.c @@ -26,3 +26,40 @@ bool id_wellformed(const char *id) } return true; } + +#define ID_SPECIAL_CHAR '#' + +static const char *const id_subsys_str[] = { + [ID_QDEV] = "qdev", + [ID_BLOCK] = "block", +}; + +/* + * Generates an ID of the form PREFIX SUBSYSTEM NUMBER + * where: + * + * - PREFIX is the reserved character '#' + * - SUBSYSTEM identifies the subsystem creating the ID + * - NUMBER is a decimal number unique within SUBSYSTEM. + * + * Example: "#block146" + * + * Note that these IDs do not satisfy id_wellformed(). + * + * The caller is responsible for freeing the returned string with g_free() + */ +char *id_generate(IdSubSystems id) +{ + static uint64_t id_counters[ID_MAX]; + uint32_t rnd; + + assert(id < ID_MAX); + assert(id_subsys_str[id]); + + rnd = g_random_int_range(0, 100); + + return g_strdup_printf("%c%s%" PRIu64 "%02" PRId32, ID_SPECIAL_CHAR, + id_subsys_str[id], + id_counters[id]++, + rnd); +}
Multiple sub-systems in QEMU may find it useful to generate IDs for objects that a user may reference via QMP or HMP. This patch presents a standardized way to do it, so that automatic ID generation follows the same rules. This patch enforces the following rules when generating an ID: 1.) Guarantee no collisions with a user-specified ID 2.) Identify the sub-system the ID belongs to 3.) Guarantee of uniqueness 4.) Spoiling predictability, to avoid creating an assumption of object ordering and parsing (i.e., we don't want users to think they can guess the next ID based on prior behavior). The scheme for this is as follows (no spaces): # subsys D RR Reserved char --| | | | Subsystem String ----| | | Unique number (64-bit) --| | Two-digit random number ---| For example, a generated node-name for the block sub-system may look like this: #block076 The caller of id_generate() is responsible for freeing the generated node name string with g_free(). Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- include/qemu-common.h | 8 ++++++++ util/id.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)