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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

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

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