Re: [Qemu-devel] [PATCH v2] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

On 09/07/18 13:30, Igor Mammedov wrote:
> On Thu, 6 Sep 2018 16:13:52 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 09/06/18 14:51, Igor Mammedov wrote:
>>> If VM has VCPUs plugged sparselly (for example a VM started with
>>> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
>>> only cpu0 and cpu2 are present), QGA will rise a error
>>>   error: internal error: unable to execute QEMU agent command 
>>> 'guest-get-vcpus':
>>>   open("/sys/devices/system/cpu/cpu1/"): No such file or directory
>>> when
>>>   virsh vcpucount FOO --guest
>>> is executed.
>>> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
>>>
>>> Signed-off-by: Igor Mammedov <address@hidden>
>>> ---
>>> v2:
>>>   do not create CPU entry if cpu isn't present
>>>   (Laszlo Ersek <address@hidden>)
>>> ---
>>>  qga/commands-posix.c | 115 
>>> ++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 59 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 37e8a2d..42d30f0 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -2035,61 +2035,56 @@ static long sysconf_exact(int name, const char 
>>> *name_str, Error **errp)
>>>   * Written members remain unmodified on error.
>>>   */
>>>  static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
>>> -                          Error **errp)
>>> +                          char *dirpath, Error **errp)
>>>  {
>>> -    char *dirpath;
>>> +    int fd;
>>> +    int res;
>>>      int dirfd;
>>> +    static const char fn[] = "online";
>>>
>>> -    dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
>>> -                              vcpu->logical_id);
>>>      dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>>>      if (dirfd == -1) {
>>>          error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>> -    } else {
>>> -        static const char fn[] = "online";
>>> -        int fd;
>>> -        int res;
>>> -
>>> -        fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
>>> -        if (fd == -1) {
>>> -            if (errno != ENOENT) {
>>> -                error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, 
>>> fn);
>>> -            } else if (sys2vcpu) {
>>> -                vcpu->online = true;
>>> -                vcpu->can_offline = false;
>>> -            } else if (!vcpu->online) {
>>> -                error_setg(errp, "logical processor #%" PRId64 " can't be "
>>> -                           "offlined", vcpu->logical_id);
>>> -            } /* otherwise pretend successful re-onlining */
>>> -        } else {
>>> -            unsigned char status;
>>> -
>>> -            res = pread(fd, &status, 1, 0);
>>> -            if (res == -1) {
>>> -                error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, 
>>> fn);
>>> -            } else if (res == 0) {
>>> -                error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", 
>>> dirpath,
>>> -                           fn);
>>> -            } else if (sys2vcpu) {
>>> -                vcpu->online = (status != '0');
>>> -                vcpu->can_offline = true;
>>> -            } else if (vcpu->online != (status != '0')) {
>>> -                status = '0' + vcpu->online;
>>> -                if (pwrite(fd, &status, 1, 0) == -1) {
>>> -                    error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", 
>>> dirpath,
>>> -                                     fn);
>>> -                }
>>> -            } /* otherwise pretend successful re-(on|off)-lining */
>>> +        return;
>>> +    }
>>>
>>> -            res = close(fd);
>>> -            g_assert(res == 0);
>>> -        }
>>> +    fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
>>> +    if (fd == -1) {
>>> +        if (errno != ENOENT) {
>>> +            error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
>>> +        } else if (sys2vcpu) {
>>> +            vcpu->online = true;
>>> +            vcpu->can_offline = false;
>>> +        } else if (!vcpu->online) {
>>> +            error_setg(errp, "logical processor #%" PRId64 " can't be "
>>> +                       "offlined", vcpu->logical_id);
>>> +        } /* otherwise pretend successful re-onlining */
>>> +    } else {
>>> +        unsigned char status;
>>> +
>>> +        res = pread(fd, &status, 1, 0);
>>> +        if (res == -1) {
>>> +            error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
>>> +        } else if (res == 0) {
>>> +            error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
>>> +                       fn);
>>> +        } else if (sys2vcpu) {
>>> +            vcpu->online = (status != '0');
>>> +            vcpu->can_offline = true;
>>> +        } else if (vcpu->online != (status != '0')) {
>>> +            status = '0' + vcpu->online;
>>> +            if (pwrite(fd, &status, 1, 0) == -1) {
>>> +                error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
>>> +                                 fn);
>>> +            }
>>> +        } /* otherwise pretend successful re-(on|off)-lining */
>>>
>>> -        res = close(dirfd);
>>> +        res = close(fd);
>>>          g_assert(res == 0);
>>>      }
>>>
>>> -    g_free(dirpath);
>>> +    res = close(dirfd);
>>> +    g_assert(res == 0);
>>>  }
>>>
>>>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>> @@ -2107,17 +2102,21 @@ GuestLogicalProcessorList 
>>> *qmp_guest_get_vcpus(Error **errp)
>>>      while (local_err == NULL && current < sc_max) {
>>>          GuestLogicalProcessor *vcpu;
>>>          GuestLogicalProcessorList *entry;
>>> -
>>> -        vcpu = g_malloc0(sizeof *vcpu);
>>> -        vcpu->logical_id = current++;
>>> -        vcpu->has_can_offline = true; /* lolspeak ftw */
>>> -        transfer_vcpu(vcpu, true, &local_err);
>>> -
>>> -        entry = g_malloc0(sizeof *entry);
>>> -        entry->value = vcpu;
>>> -
>>> -        *link = entry;
>>> -        link = &entry->next;
>>> +        int64_t id = current++;
>>> +        char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 
>>> "/",
>>> +                                     id);
>>> +
>>> +        if (g_file_test(path, G_FILE_TEST_EXISTS)) {
>>> +            vcpu = g_malloc0(sizeof *vcpu);
>>> +            vcpu->logical_id = id;
>>> +            vcpu->has_can_offline = true; /* lolspeak ftw */
>>> +            transfer_vcpu(vcpu, true, path, &local_err);
>>> +            entry = g_malloc0(sizeof *entry);
>>> +            entry->value = vcpu;
>>> +            *link = entry;
>>> +            link = &entry->next;
>>> +        }
>>> +        g_free(path);
>>>      }
>>>
>>>      if (local_err == NULL) {
>>> @@ -2138,7 +2137,11 @@ int64_t 
>>> qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>>>
>>>      processed = 0;
>>>      while (vcpus != NULL) {
>>> -        transfer_vcpu(vcpus->value, false, &local_err);
>>> +        char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 
>>> "/",
>>> +                                     vcpus->value->logical_id);
>>> +
>>> +        transfer_vcpu(vcpus->value, false, path, &local_err);
>>> +        g_free(path);
>>>          if (local_err != NULL) {
>>>              break;
>>>          }
>>>  
>>
>> This looks good. The things that I could raise, if I wanted to annoy you
>> :) , are:
>>
>> - transfer_vcpu() should take "const char *dirpath", not just "char
>>   *dirpath",
>>
>> - there's a small window between g_file_test() and open() -- not sure if
>>   we care, but it catches my eye,
> Even if dirpath is opened cpu removal might still race vs accesses inside 
> that directory.
> I'd say it's not worth effort to make race free,
> just don't use damn thing with real unplug.
> 
>  
>> - the pathname formatting is now duplicated.
> Yep, is wasn't worth adding helper func since it just increases amount of 
> code.
> But I don't really care can add it if I respin patch.
> 
>> How about a helper function like this:
>>
>>> static int open_cpu_dir(int64_t cpu_id, Error **errp)
>>> {
>>>   char *dirpath;
>>>   int rc;
>>>
>>>   dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/", 
>>> cpu_id);
>>>   rc = open(dirpath, O_RDONLY | O_DIRECTORY);
>>>   if (rc == -1) {
>>>     rc = -errno;
>>>     error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>>>   }
>>>   g_free(dirpath);
>>>   return rc;
>>> }
>>
>> Then you can:
>>
>> (1) remove g_file_test() from qmp_guest_get_vcpus(),
>>
>> (2) remove open() from transfer_vcpu(),
>>
>> (3) take a dirfd in transfer_vcpu(), rather than a dirpath,
> I've considered it, but if we don't pass dirpath to transfer_vcpu()
> we loose on path in error messages hence dropped whole idea as excessive.
> 
> 
>> (4) distinguish "success" (non-negative), (-ENOENT), and "other error"
>>     (negative) in qmp_guest_get_vcpus(), and act accordingly (call
>>     transfer_vcpu(), skip, or bail for good; respectively),
>>
>> (5) eliminate the formatting from qmp_guest_set_vcpus().
>>
>> I think the only quirk is that, when skipping in (4), local_err has to
>> be freed with error_free(), and re-set to NULL.
>>
>> What do you think? (If it's too much work for little gain, I'm ready to
>> ACK v2 as well; don't want to waste your time.)
> I doesn't matter to me. Since you happen to be original author,
> I'll deffer to your opinion.

OK, I'm fine with the current patch.

Reviewed-by: Laszlo Ersek <address@hidden>

Thanks
Laszlo