Re: [Qemu-devel] [PATCH RFC for-2.3 3/6] qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting zhanghailiang (2014-12-06 00:59:16)
> We can get guest's memory block information by using command
> "guest-get-memory-blocks", the returned value contains a list of memory block
> info, such as phys_index, online state, can-offline info.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8917dca..d3f7d4f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2005,6 +2005,60 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>      g_assert(res == 0);
>  }
> 
> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> +{
> +    GuestMemoryBlockList *head, **link;
> +    Error *local_err = NULL;
> +    struct dirent *de;
> +    DIR *dp;
> +
> +    head = NULL;
> +    link = &head;
> +
> +    dp = opendir("/sys/devices/system/memory/");
> +    if (!dp) {
> +        error_setg_errno(errp, errno, "Can't open directory"
> +                         "\"/sys/devices/system/memory/\"\n");
> +        return NULL;
> +    }
> +
> +    /* Note: the phys_index of memory block may be discontinuous,
> +     * this is because a memblk is the unit of the Sparse Memory design, which
> +     * allows discontinuous memory ranges (ex. NUMA), so here we should
> +     * traverse the memory block directory.
> +     */
> +    while ((de = readdir(dp)) != NULL) {
> +        GuestMemoryBlock *mem_blk;
> +        GuestMemoryBlockList *entry;
> +
> +        if ((strncmp(de->d_name, "memory", 6) != 0) ||
> +            !(de->d_type & DT_DIR)) {
> +            continue;
> +        }
> +
> +        mem_blk = g_malloc0(sizeof *mem_blk);
> +        /* The d_name is "memoryXXX",  phys_index is block id, same as XXX */
> +        mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
> +        mem_blk->has_can_offline = true; /* lolspeak ftw */

My initial thought as well :P

> +        transfer_memory_block(mem_blk, true, &local_err);
> +
> +        entry = g_malloc0(sizeof *entry);
> +        entry->value = mem_blk;
> +
> +        *link = entry;
> +        link = &entry->next;
> +    }
> +
> +    if (local_err == NULL) {
> +        /* there's no guest with zero memroy blocks */

typo

> +        g_assert(head != NULL);

As commented on in earlier patches, guest errors shouldnt crash the guest agent
if it can be avoided.

> +        return head;
> +    }
> +
> +    qapi_free_GuestMemoryBlockList(head);
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -2040,6 +2094,12 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return -1;
>  }
> 
> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
>  #endif
> 
>  #if !defined(CONFIG_FSFREEZE)
> @@ -2126,12 +2186,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> -GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> -{
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -

Looks like some unecessary code movement made it's way into the patch. Please
squash this change into original patch

>  int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>                                      Error **errp)
>  {
> -- 
> 1.7.12.4