[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Jul 17, 2006 at 01:50:25PM +0100, Daniel P. Berrange wrote:
> 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 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.

Oh one other benefit of this, is if the application wishes to use C-99
they will not have to use malloc at all, instead making use of local
declarations & dynamically sized stack variables:

   virDomainInfo info;
 
   virDomainGetInfo(domain, &info);
 
   virDomainVcpuInfo cpuInfo[info.nrVirtCpu];
   int cpuMapLen = (info.nrVirtCpu + 7) / 8 ;
   char cpuMap[cpuMapLen];
 
   virDomainGetVCpus(domain, cpuInfo, info.nrVirtCpu, cpuMap, cpuMapLen);

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]