On Tue, Jul 11, 2006 at 10:50:53AM -0400, Daniel Veillard wrote:

...

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

This interesting thread is temporarilu suspended because Michel has left for some (long) vacation until August 15th. I think the last proposal is a good one and deserves to be implemented and tested. I am confident that Michel will be willing to update its patch accordingly when he comes back. Is it OK for you ?. Side question: is there a defined "sign-off" procedure to follow when submitting patches ? Cordialement. Jean-Paul Daniel Veillard <veillard@redhat.com> Envoyé par : libvir-list-bounces@redhat.com 17/07/2006 15:08 Veuillez répondre à veillard Pour : "Daniel P. Berrange" <berrange@redhat.com> cc : libvir-list@redhat.com, michel.ponceau@bull.net Objet : 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: 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

Right this is confusing, I wanted to avoid another allocation in the general case but this made things even more confusing.

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);

Agreed, this is the most logical approach

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.

Agree. We should keep an example of use (like above completed) in the source tree to help developpers. Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list