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:
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
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/