Re: [PATCH not-for-merge 2/5] qom: Make "info qom-tree" show children sorted

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 5/18/20 12:19 AM, Markus Armbruster wrote:
>> "info qom-tree" prints children in unstable order.  This is a pain
>> when diffing output for different versions to find change.  Print it
>> sorted.
>
> Yes, this does seem reasonable to include even without the rest of the
> series.

Noted.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   qom/qom-hmp-cmds.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
>> index 4a61ee1b8c..cf0af8f6b5 100644
>> --- a/qom/qom-hmp-cmds.c
>> +++ b/qom/qom-hmp-cmds.c
>> @@ -78,6 +78,35 @@ static int print_qom_composition_child(Object *obj, void 
>> *opaque)
>>       return 0;
>>   }
>>   +static int qom_composition_compare(const void *a, const void *b,
>> void *ignore)
>> +{
>> +    Object *obja = (void *)a, *objb = (void *)b;
>
> Casting away const...
>
>> +    const char *namea, *nameb;
>> +
>> +    if (obja == object_get_root()) {
>> +        namea = g_strdup("");
>> +    } else {
>> +        namea = object_get_canonical_path_component(obja);
>
> ...should we instead improve object_get_canonical_path_component to
> work with 'const Object *'?

Go right ahead :)

I need to sit on my hands to have a chance getting my task queue back
under control.

>> +    }
>> +
>> +    if (objb == object_get_root()) {
>> +        nameb = g_strdup("");
>> +    } else {
>> +        nameb = object_get_canonical_path_component(objb);
>> +    }
>> +
>> +
>> +    return strcmp(namea, nameb);
>
> Why the two blank lines?  This leaks namea and/or nameb if either
> object is the object root.  Should you instead use g_strcmp0 here,
> with namea/b set to NULL instead of g_strdup("") above?

My not-for-merge proves prudent ;)

>> @@ -105,7 +134,16 @@ static void print_qom_composition(Monitor *mon, Object 
>> *obj, int indent)
>>       monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
>>                      object_get_typename(obj));
>>       g_free(name);
>> -    object_child_foreach(obj, print_qom_composition_child, &s);
>> +
>> +    GQueue children;
>> +    Object *child;
>
> Mid-function declarations - I assume you'd clean this up if we want
> this for real?

Yes.  I prioritized diff over maintainability, because not-for-merge.

>> +    g_queue_init(&children);
>> +    object_child_foreach(obj, insert_qom_composition_child, &children);
>> +    while ((child = g_queue_pop_head(&children))) {
>> +        print_qom_composition(mon, child, indent + 2);
>> +    }
>> +    (void)s;
>> +    (void)print_qom_composition_child;
>
> Also, this looks like leftover debugger aids?

Shut up the compiler so I don't have to remove code.  Shorter diff,
not-for-merge.

>>   }
>>     void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
>>

Thanks!