[Libvir] Re: Proposal : add 3 functions to Libvirt API, for virtual CPUs

[Daniel P. Berrange]

On Tue, Jul 11, 2006 at 10:50:53AM -0400, Daniel Veillard wrote:
> On Tue, Jul 11, 2006 at 04:14:12PM +0200, michel.ponceau at bull.net wrote:
> 
>    Hum, I could see compilers righteously complaining about
>      unsigned char cpumap[];
> in a structure. Maybe we should default to 256 / 8 but allow at the API level
> to grow over that value. What we could do is define a default of 256 available
> in the structure and allow an extra parameter which could point to a larger
> array like the following:
>    - restore VIR_MAX_CPUS as VIR_STD_MAX_CPUS 256
>    - virDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo,
>                        char *lcpumap, int lmaplen);
> 
>    lcpumap and lmaplen being extra arguments for very large arrays
>    for most use case, we will fit in 256 processors, those will be respectively
>    NULL and 0, but assuming we have more than 256 processors:
>      + maxinfo > VIR_STD_MAX_CPUS
>      + lcpumap points to a array of bytes, they are interpreted as an
>        array of cpumap of ((maxinfo + 7) div 8) bytes each. So 
>        if lmaplen != ((maxinfo + 7) div 8) * maxinfo then there is an
>        error.
>    in that case the cpumap structures of info are not filled on return.
> 
>   We still have a relatively simple API for the common case, and for special
> cases we have an extension capability with relatively clear definitions. it's
> a bitstrange but I think that should cover most case as best as possible

I dont particularly like this as an API because I think it will be error
prone for application developers. Most app developers will only ever have
a handful of CPUs in their test machines, so they'll never the alternate
codepath for > 256 cpu case.  Likewise I don't like the idea of a virVcpuInfo
struct which has a variable size because it will totally confuse people who
haven't read the API docs very carefully, again leading to obscure bugs.

The root problem is that we have two conflicting goals here 

  1. Want to have virVcpuInfo be a fixed size struct
  2. We want a cpumap of arbitrary size

The obvious solution to this problem is to *remove*  the cpumap data from
the virVcpuInfo structure completely, and always pass in a separately 
malloc'd array of the correct size. So I'd suggest:

   typedef struct _virVcpuInfo virVcpuInfo;
   struct _virVcpuInfo {
     unsigned int number;        /* virtual CPU number */
     int state;                  /* value from virVcpuState */
     unsigned long long cpuTime; /* CPU time used, in nanoseconds */
     int cpu;                    /* real CPU number, or -1 if offline */
   }

  virDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo,
                    char *cpumap, int maplen);


The client applications calling this API already have to malloc() the memory
region for the 'info' parameter of a correct size, so having to also malloc
the cpumap parameter is no extra trouble.

  virDomainInfo info;
  virDomainVpuInfoPtr cpuInfo;
  int cpuMapLen;
  char *cpuMap;

  virDomainGetInfo(domain, &info);

  cpuInfo = malloc(sizeof(virDomainVcpuInfo)*info.nrVirtCpu);
  cpuMapLen = (info.nrVirtCpu + 7) / 8 ;
  cpuMap = malloc(cpuMapLen);

  virDomainGetVCpus(domain, cpuInfo, info.nrVirtCpu, cpuMap, cpuMapLen);

  ... do stuff with the data ...

  free(cpuInfo);
  free(cpuMap);


So you can see there is minimal extra work to always pass in cpuMap as 
a separate parameter. If an application didn't care about the cpuMap
data they could simply pass in NULL.

Regards,
Dan,
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|