Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[John Snow <jsnow@redhat.com>]

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 */
>>> [...]