Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Eric Blake <eblake@redhat.com>]

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

On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d4b6bdc..1848df8 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -40,6 +40,7 @@ extern char **environ;
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
>  #include <net/if.h>
> +#include <inttypes.h>
>  
>  #ifdef FIFREEZE
>  #define CONFIG_FSFREEZE
> @@ -1178,7 +1179,57 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  
>  void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> +#if defined(__linux__)
> +    const GuestLogicalProcessorList *entry;
> +    Error *local_err = NULL;
> +
> +    for (entry = vcpus; local_err == NULL && entry != NULL;
> +         entry = entry->next) {
> +        const GuestLogicalProcessor *vcpu;
> +
> +        vcpu = entry->value;
> +        if (vcpu->logical_id == 0) {
> +            if (!vcpu->online) {
> +                error_setg(&local_err,
> +                           "unable to offline logical processor #0");
> +            }

This is not quite accurate; my understanding is that there are setups
where cpu0 can be offlined.  More accurate would be to reject attempts
to offline ANY cpu where the cpu/cpuNN/online file does not exist (which
will catch the fact that cpu0 must always be on for the hardware you are
testing with).

> +        } else {
> +            char *buf;
> +            FILE *f;
> +
> +            buf = g_strdup_printf("/sys/devices/system/cpu/cpu%"PRId64
> +                                  "/online", vcpu->logical_id);
> +            f = fopen(buf, "r+");
> +            if (f == NULL) {
> +                error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r+\")",
> +                                 buf);

Again, if the file doesn't exist, but the user asked for online, then
this should not be an error.

> +            } else {
> +                unsigned online;
> +
> +                if (fscanf(f, "%u", &online) != 1) {
> +                    error_setg(&local_err, "failed to read or parse \"%s\"",
> +                               buf);

Does doing a scan of the file's existing contents buy us any safety?
Why not just blindly write into the file, instead of first reading it?

> +                } else if ((online != 0) != vcpu->online) {
> +                    errno = 0;
> +                    rewind(f);
> +                    if (errno != 0 ||
> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {

Do you really want to be printing NUL or \1?  I though the kernel
interface expected the literal character '0' or '1' (in ascii, \x30 or
\x31).

Why even bother with stdio buffering?  Wouldn't it be simpler to
open()/write() instead of fopen()/fprintf()?

> +                        error_setg_errno(&local_err, errno,
> +                                         "rewind()/fprintf() on \"%s\"", buf);
> +                    }
> +                }
> +
> +                if (fclose(f) == EOF && local_err == NULL) {
> +                    error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
> +                }
> +            }
> +            g_free(buf);
> +        }
> +    }
> +    error_propagate(errp, local_err);
> +#else
>      error_set(errp, QERR_UNSUPPORTED);
> +#endif
>  }
>  
>  /* register init/cleanup routines for stateful command groups */
> 

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