Re: [Qemu-devel] [PATCH v2] Add interface to traverse the qmp command list by QmpCommand

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

On 10/08/2013 12:23 AM, Mark Wu wrote:
> In the original code, qmp_get_command_list is used to construct
> a list of all commands' name. To get the information of all qga
> commands, it traverses the name list and search the command info
> with its name.  So it can cause O(n^2) in the number of commands.
> 
> This patch adds an interface to traverse the qmp command list by
> QmpCommand to replace qmp_get_command_list. It can decrease the
> complexity from O(n^2) to O(n).
> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> ---
> Changes:
> v2:
> 	1. Keep the signature of qmp_command_is_enabled (per Eric and Michael)
>     2. Remove the unnecessary pointer castings (per Eric)
> 
>  include/qapi/qmp/dispatch.h |  5 ++--
>  qapi/qmp-registry.c         | 27 +++---------------
>  qga/commands.c              | 38 ++++++++++---------------
>  qga/main.c                  | 68 +++++++++++++++++----------------------------
>  4 files changed, 48 insertions(+), 90 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/qga/main.c
> @@ -347,48 +347,34 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
>  }
>  
>  /* disable commands that aren't safe for fsfreeze */
> -static void ga_disable_non_whitelisted(void)
> +static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
>  {
> -    char **list_head, **list;
>      bool whitelisted;
>      int i;
>  
> -    list_head = list = qmp_get_command_list();
> -    while (*list != NULL) {
> -        whitelisted = false;
> -        i = 0;
> -        while (ga_freeze_whitelist[i] != NULL) {
> -            if (strcmp(*list, ga_freeze_whitelist[i]) == 0) {
> -                whitelisted = true;
> -            }
> -            i++;
> -        }
> -        if (!whitelisted) {
> -            g_debug("disabling command: %s", *list);
> -            qmp_disable_command(*list);
> +    whitelisted = false;
> +    i = 0;
> +    while (ga_freeze_whitelist[i] != NULL) {
> +        if (strcmp(cmd->name, ga_freeze_whitelist[i]) == 0) {

This (and several other instances) accesses cmd->name directly where we
were formally using the string from the returned list.  Should these
spots be modified to go through an accessor method instead, similar to
qmp_command_is_enabled, so that QmpCommand can be treated as an opaque
type from this file?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]