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

Re: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats



On 08/17/2011 10:10 AM, Daniel P. Berrange wrote:
2. recompiling an existing client against newer libvirt.h with no
other changes will automatically pick up the larger struct.  The
sizeof() argument will change.  Newer servers will recognize the
larger struct and automatically call the newer RPC, getting all the
new fields. However, older servers will now reject the larger sizeof
as too large - this is a subtle limitation, if clients are
recompiled without being aware of the difference it will cause!

That last point is a significant problem. While we have technically
preserved "ABI" compatibility, we have not preserved actual real
world application API usage compatibility.

I don't really like the idea of having this kind of special case
functionality in just one of our APIs, where our current policy
is always to add new APIs. While this is a no doubt a very neat
trick in terms of ABI compatibility, I think we should go for
clarity&  simplicity.

OK, both DV and you have convinced me that API compatibility is important, and that we must not get in the situation of point 2 - that is, a solution is viable only if any application that uses the type virDomainBlockStatsPtr is always referring to the v1 struct size, and the only way to get the larger struct is to use a new struct name.

But I'm unclear from your answer whether you are opposed to the overall idea of reusing virDomainBlockStats() with the size argument as the key on which struct was passed in (in which case, we need to rethink everything about my proposal - and rebasing to a newer .so is the only way to take advantage of the new fields), or just the idea of an API break by how I first described the changes (in which case I am confident that we can tweak libvirt.h.in to guarantee a naming choice that preserves API compatibility across recompilation, while most of my proposal stands as-is where it remains possible to backport the new fields to older libvirtd installations since no .so entry points are changed). DV also seemed to be in favor of reusing the existing function call with a new struct, as long as there is no API break for existing code. And personally, I see value in being able to add a new struct without requiring a rebase to a new ABI.


I just audited libvirt.c, and found that the following libvirt interfaces that take a size parameter, and could be extensible by use of the size field:
virDomainBlockStats
virDomainInterfaceStats

The following libvirt interfaces take a flags parameter, and could be extensible by setting a flag bit that says a larger struct is in use:
virDomainGetControlInfo
virNodeGetCPUStats
virNodeGetMemoryStats
virDomainMemoryStats
virDomainGetBlockInfo
virDomainGetBlockJobInfo

Meanwhile, the following APIs which take a pointer to a struct are non-extensible (neither size nor flags as an escape hatch to indicate explicit use of a larger struct), extending any of these would require a new API function:
virDomainGetInfo
virNodeGetInfo
virDomainGetVcpus
virDomainGetSecurityLabel
virDomainGetSecurityModel
virStoragePoolGetInfo
virStorageVolGetInfo
virDomainGetJobInfo



As a final question for this mail, if we decide that reusing the existing ABI with a new struct size is not desirable, then would you rather that the new virDomainBlockStats2 API (or whatever we name the new function) would also take a (non-extensible) struct, or would it be better to rewrite it in terms of 'virTypedParameterPtr params, int *nparams' so that all future extensibility is covered by adding name-type-value tuples rather than dealing with future struct sizing? I'd rather go through the pain of an ABI addition at most once, rather than once per time that we identify new fields to add.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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