Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Mark Wu <wudxw@linux.vnet.ibm.com>]

Hi guys,

Thanks a lot for all your insightful comments.  I will post a patch to 
resolve the O(n2) problem according to Michael's comments soon and 
rebase the 'success-response' on it.

Mark.

On 09/25/2013 08:34 AM, Luiz Capitulino wrote:
> On Tue, 24 Sep 2013 16:07:29 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
>>>>> +bool qmp_command_has_success_response(const char *name)
>>>>> +{
>>>>> +    QmpCommand *cmd;
>>>>> +
>>>>> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
>>>>> +        if (strcmp(cmd->name, name) == 0) {
>>>>> +            return cmd->options != QCO_NO_SUCCESS_RESP;
>>> cmd->options is a bitmask - it is feasible that we may add more QCO_NO_*
>>> flags in the future, at which point inequality is NOT correct.  Rather,
>>> you want:
>>>
>>> return !(cmd->options & QCO_NO_SUCCESS_RESP);
> Good catch! IIRC I added cmd->options myself and didn't catch this...
>
>>>>> +++ b/qga/commands.c
>>>>> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>>>>>           cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>>>>>           cmd_info->name = g_strdup(*cmd_list);
>>>>>           cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
>>>>> +        cmd_info->success_response =
>>>>> +            qmp_command_has_success_response(cmd_info->name);
>>> This feels wasteful.  Why are we doing an O(n) lookup for BOTH
>>> qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n)
>>> loop over command names?  That's O(n^2) in the number of commands.
>>> Better would be getting a list of QmpCommand* instead of a list of
>>> char*, and looking directly in each object, for O(n) computation of the
>>> results.
>> Agreed, modifying qmp_get_command_list to return a list of QmpCommand
>> would be nicer. Rather than looking directly at the fields though I
>> think we should just fix up qmp_command_is_enabled() and friends to
>> take a QmpCommand arg instead of a char*. We already have
>> qmp_find_command to map char*->QmpCommand to support any cases where
>> we rely on cmd names.
> I agree and I thought the same thing when I reviewed the patch, but
> I didn't mind as Mark is just using what's already there.
>