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

[Daniel P. Berrange]

On Wed, Aug 17, 2011 at 09:39:34AM -0600, Eric Blake wrote:
> On 08/12/2011 07:53 AM, Daniel P. Berrange wrote:
> >On Fri, Aug 12, 2011 at 10:17:24PM +0800, Osier Yang wrote:
> >>The new disk latency related members include:
> >>     rd_total_times
> >>     wr_total_times
> >>     flush_operations
> >>     flush_total_times
> >>---
> >>  include/libvirt/libvirt.h.in |    4 ++++
> >>  1 files changed, 4 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> >>index b1bda31..cf6bf60 100644
> >>--- a/include/libvirt/libvirt.h.in
> >>+++ b/include/libvirt/libvirt.h.in
> >>@@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats virDomainBlockStatsStruct;
> >>  struct _virDomainBlockStats {
> >>    long long rd_req; /* number of read requests */
> >>    long long rd_bytes; /* number of read bytes */
> >>+  long long rd_total_times; /* total time spend on cache reads in nano-seconds */
> >>    long long wr_req; /* number of write requests */
> >>    long long wr_bytes; /* number of written bytes */
> >>+  long long wr_total_times; /* total time spend on cache writes in nano-seconds */
> >>+  long long flush_req; /* number of flushed bytes */
> >>+  long long flush_total_times; /* total time spend on cache flushes in nano-seconds */
> >>    long long errs;   /* In Xen this returns the mysterious 'oo_req'. */
> >>  };
> >
> >NACK, it is forbidden to modify any structs in the public header
> >files since that changes the ABI.
> 
> However, what we CAN do, without breaking ABI, is the following:
> 
> In libvirt.h.in:
> 
> add a new struct virDomainBlockStatsStructV1 with layout identical
> to the old virDomainBlockStatsStruct.
> 
> add a new typedef virDomainBlockStatsStructV2 that maps to the
> modified virDomainBlockStatsStruct.
> 
> modify struct virDomainBlockStatsStruct to add new fields, but
> _only_ at the end of the struct - this struct could later grow if we
> need to go to a v3.
> 
> Leave virDomainBlockStatsPtr as a typedef for *virDomainBlockStatsStruct.
> 
> In libvirt.c:
> 
> int
> virDomainBlockStats (virDomainPtr dom, const char *path,
>                      virDomainBlockStatsPtr stats, size_t size)
> {
>     virConnectPtr conn;
>     struct _virDomainBlockStatsStructV1 statsv1 = { -1, -1, -1, -1, -1 };
>     struct _virDomainBlockStatsStructV2 statsv2 = { -1, -1, -1, -1,
> -1, -1 ... };
> 
>     VIR_DOMAIN_DEBUG
> ...
>     if (!path || !stats || size > sizeof(statsv2)) {
>         virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>         goto error;
>     }
>     conn = dom->conn;
> 
>     if (conn->driver->domainBlockStatsv2) {
>         if (conn->driver->domainBlockStatsv2 (dom, path, &statsv2) == -1)
>             goto error;
> 
>         memcpy (stats, &statsv2, size);
>         return 0;
>     }
>     if (conn->driver->domainBlockStats) {
>         if (conn->driver->domainBlockStats (dom, path, &statsv1) == -1)
>             goto error;
> 
>         if (size > sizeof(statsv1)) {
>             memset(((char *)stats)+sizeof(statsv1), -1, size -
> sizeof(statsv1));
>             size = sizeof(statsv1);
>         }
>         memcpy (stats, &statsv1, size);
>         return 0;
>     }
> ...
> 
> 
> In driver.h:
> 
> Update the domainBlockStats callback to explicitly take
> *virDomainBlockStatsStructV1, and add a new domainBlockStats2
> callback that explicitly takes *virDomainBlockStatsStructV2.
> 
> In remote_protocol.x, add a new RPC for domain_block_stats_2 that
> passes the larger struct over the wire; leaving the existing RPC
> tied to the smaller v1 struct.
> 
> In qemu_driver.c, implement the new driver callback as the only way
> to get at the additional fields.
> 
> 
> This proposal is ABI safe (no function signatures changed and no new
> entry points are added - all the decisions on how many bytes to
> populate are based on an existing argument, and all APIs used
> pointers which are constant size rather than copies where the larger
> struct would cause problems).  However, it has the following
> effects, including one slight API ramification:
> 
> 1. all existing clients that were pre-compiled against the older
> libvirt.h should have already been calling
> virDomainBlockStats(...,sizeof(virDomainBlockStatsStruct)), where
> the compilation was done with the struct at the smaller size.  This
> will safely call the older RPC, and skip out on any new fields,
> whether talking to an old or new server.
> 
> 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.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|