[libvirt] [PATCH 1/2] Parallels: add domainGetVcpus().

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 06:06:38PM +0400, Alexander Burluka wrote:
> From: A.Burluka <aburluka at parallels.com>
> 
> OpenStack Nova requires this function
> to start VM instance. Cpumask info is obtained via prlctl utility.
> Unlike KVM, Parallels Cloud Server is unable to set cpu affinity
> mask for every VCpu. Mask is unique for all VCpu. You can set it
> using 'prlctl set <vm_id|vm_name> --cpumask <{n[,n,n1-n2]|all}>'
> command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7'
> would set this mask to yy---yyy.
> ---
>  src/parallels/parallels_driver.c |  169 +++++++++++++++++++++++++++++++++++++-
>  src/parallels/parallels_utils.h  |    1 +
>  2 files changed, 167 insertions(+), 3 deletions(-)
> 
> diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
> index ab59599..f1d5ecc 100644
> --- a/src/parallels/parallels_driver.c
> +++ b/src/parallels/parallels_driver.c
> @@ -108,6 +108,7 @@ parallelsDomObjFreePrivate(void *p)
>      if (!pdom)
>          return;
>  
> +    VIR_FREE(pdom->cpumask);
>      VIR_FREE(pdom->uuid);
>      VIR_FREE(pdom->home);
>      VIR_FREE(p);
> @@ -654,6 +655,9 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj)
>      if (VIR_ALLOC(def) < 0)
>          goto cleanup;
>  
> +    if (VIR_ALLOC(pdom) < 0)
> +        goto cleanup;
> +
>      def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
>      def->id = -1;
>  
> @@ -716,6 +720,17 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj)
>          goto cleanup;
>      }
>  
> +    if (!(tmp = virJSONValueObjectGetString(jobj3, "mask"))) {
> +        /* Absence of this field means that all domains cpus are available */
> +        if (VIR_STRDUP(pdom->cpumask, "all") < 0) {
> +            goto cleanup;
> +        }
> +    } else {
> +        if (VIR_STRDUP(pdom->cpumask, tmp) < 0) {
> +            goto cleanup;
> +        }
> +    }

See my comment later about using 'virBitmap' - this is where you shoudl parse
the string into a bitmap instance.

> +static int
> +parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
> +                        virBitmapPtr *cpumask,
> +                        int hostcpus)
> +{
> +    int ret = -1;
> +    int cpunum = -1;
> +    int prevcpunum = -1;
> +    int offset = 0;
> +    const char *it = privatedomdata->cpumask;
> +    bool isrange = false;
> +    size_t i;
> +    int cpunums[512] = { 0 };
> +    size_t count = 0;
> +
> +    if (STREQ(it, "all")) {
> +        if (!(*cpumask = virBitmapNew(hostcpus)))
> +            goto cleanup;
> +        virBitmapSetAll(*cpumask);
> +    } else {
> +        while (sscanf(it, "%d%n", &cpunum, &offset)) {
> +            char delim = 0;
> +            if (isrange) {
> +                for (i = prevcpunum + 1; i <= cpunum; ++i) {
> +                    cpunums[count++] = i;
> +                }
> +            } else {
> +                cpunums[count++] = cpunum;
> +            }
> +
> +            it += offset;
> +
> +            if (sscanf(it, "%c%n", &delim, &offset) == EOF) {
> +                break;

What is the format that parallels uses for 'cpumask' - ideally
you would just use 'virBitmapParse' for parsing it, not write
another parser

> +static int
> +parallelsDomainGetVcpus(virDomainPtr domain,
> +                   virVcpuInfoPtr info,
> +                   int maxinfo,
> +                   unsigned char *cpumaps,
> +                   int maplen)

Generally parameters should all be aligned with the first
parameter

> diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h
> index 6215553..e88af1c 100644
> --- a/src/parallels/parallels_utils.h
> +++ b/src/parallels/parallels_utils.h
> @@ -54,6 +54,7 @@ struct parallelsDomObj {
>      int id;
>      char *uuid;
>      char *home;
> +    char *cpumask;

A while back we converted all 'char *cpumask' variables to be
'virBitmapPtr cpumask' since it makes the code nicer if we can
actually have things in fully parsed format internally, only
converting to string format when really needed.


Conceptually this looks ok - main issue is using virBitmap
for the internal data structure & changing the parser.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|