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

[zhanghailiang]

On 2014/12/22 5:17, Michael Roth wrote:
> 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 <address@hidden>
> > ---
> >   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

Ha, yes. ;)

> > +        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.

OK.

> > +        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

OK, will fix that. Thanks.

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