[libvirt] [PATCH] blockjob: avoid compiler uncertainty in info sizing
We have a policy of avoiding enum types in structs in our public API, because it is possible for a client to choose compiler options that can change the in-memory ABI of that struct based on whether the enum value occupies an int or a minimal size. But we missed this for virDomainBlockJobInfo. We got lucky on little-endian machines - if the enum fits minimal size (a char), we still end up padding to the next long before the next field; but on big-endian, a client interpreting the enum as a char would always see 0 when the server supplies contents as an int. While at it, I noticed that the web page lacked documentation: http://libvirt.org/html/libvirt-libvirt.html#virDomainBlockJobType not only for the recently added active commit, but also for all the other job types. * include/libvirt/libvirt.h.in (virDomainBlockJobInfo): Enforce particular sizing. (virDomainBlockJobType): Document recent addition. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 127de11..dc88c40 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2501,19 +2501,26 @@ int virDomainUpdateDeviceFlags(virDomainPtr domain, /** * virDomainBlockJobType: * - * VIR_DOMAIN_BLOCK_JOB_TYPE_PULL: Block Pull (virDomainBlockPull, or - * virDomainBlockRebase without flags), job ends on completion - * VIR_DOMAIN_BLOCK_JOB_TYPE_COPY: Block Copy (virDomainBlockRebase with - * flags), job exists as long as mirroring is active - * VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT: Block Commit (virDomainBlockCommit), - * job ends on completion + * Describes various possible block jobs. */ typedef enum { - VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, + VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Placeholder */ + VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, + /* Block Pull (virDomainBlockPull, or virDomainBlockRebase without + * flags), job ends on completion */ + VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2, + /* Block Copy (virDomainBlockRebase with flags), job exists as + * long as mirroring is active */ + VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3, + /* Block Commit (virDomainBlockCommit without flags), job ends on + * completion */ + VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, + /* Active Block Commit (virDomainBlockCommit with flags), job + * exists as long as sync is active */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST @@ -2537,7 +2544,7 @@ typedef unsigned long long virDomainBlockJobCursor; typedef struct _virDomainBlockJobInfo virDomainBlockJobInfo; struct _virDomainBlockJobInfo { - virDomainBlockJobType type; + int type; /* virDomainBlockJobType */ unsigned long bandwidth; /* * The following fields provide an indication of block job progress. @cur -- 1.9.3
On 06/14/14 14:50, Eric Blake wrote:
We have a policy of avoiding enum types in structs in our public API, because it is possible for a client to choose compiler options that can change the in-memory ABI of that struct based on whether the enum value occupies an int or a minimal size. But we missed this for virDomainBlockJobInfo. We got lucky on little-endian machines - if the enum fits minimal size (a char), we still end up padding to the next long before the next field; but on big-endian, a client interpreting the enum as a char would always see 0 when the server supplies contents as an int.
While at it, I noticed that the web page lacked documentation: http://libvirt.org/html/libvirt-libvirt.html#virDomainBlockJobType not only for the recently added active commit, but also for all the other job types.
* include/libvirt/libvirt.h.in (virDomainBlockJobInfo): Enforce particular sizing. (virDomainBlockJobType): Document recent addition.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 127de11..dc88c40 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2501,19 +2501,26 @@ int virDomainUpdateDeviceFlags(virDomainPtr domain, /** * virDomainBlockJobType: * - * VIR_DOMAIN_BLOCK_JOB_TYPE_PULL: Block Pull (virDomainBlockPull, or - * virDomainBlockRebase without flags), job ends on completion - * VIR_DOMAIN_BLOCK_JOB_TYPE_COPY: Block Copy (virDomainBlockRebase with - * flags), job exists as long as mirroring is active - * VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT: Block Commit (virDomainBlockCommit), - * job ends on completion + * Describes various possible block jobs. */ typedef enum { - VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, + VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Placeholder */ + VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, + /* Block Pull (virDomainBlockPull, or virDomainBlockRebase without + * flags), job ends on completion */ + VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2, + /* Block Copy (virDomainBlockRebase with flags), job exists as + * long as mirroring is active */ + VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3, + /* Block Commit (virDomainBlockCommit without flags), job ends on + * completion */ + VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, + /* Active Block Commit (virDomainBlockCommit with flags), job + * exists as long as sync is active */
#ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
ACK to the above hunk,
@@ -2537,7 +2544,7 @@ typedef unsigned long long virDomainBlockJobCursor;
typedef struct _virDomainBlockJobInfo virDomainBlockJobInfo; struct _virDomainBlockJobInfo { - virDomainBlockJobType type; + int type; /* virDomainBlockJobType */ unsigned long bandwidth; /* * The following fields provide an indication of block job progress. @cur
I don't object to this one, but I'd like to see a second opinion. Peter
On Sat, Jun 14, 2014 at 06:50:47AM -0600, Eric Blake wrote:
We have a policy of avoiding enum types in structs in our public API, because it is possible for a client to choose compiler options that can change the in-memory ABI of that struct based on whether the enum value occupies an int or a minimal size. But we missed this for virDomainBlockJobInfo. We got lucky on little-endian machines - if the enum fits minimal size (a char), we still end up padding to the next long before the next field; but on big-endian, a client interpreting the enum as a char would always see 0 when the server supplies contents as an int.
Hmm, nasty.
@@ -2537,7 +2544,7 @@ typedef unsigned long long virDomainBlockJobCursor;
typedef struct _virDomainBlockJobInfo virDomainBlockJobInfo; struct _virDomainBlockJobInfo { - virDomainBlockJobType type; + int type; /* virDomainBlockJobType */ unsigned long bandwidth; /* * The following fields provide an indication of block job progress. @cur
I think we can get away with this change from an ABI POV since we're not changing little-endian hosts, and big-endian is screwed already without it. Regards, 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 :|
On 06/16/2014 01:54 AM, Peter Krempa wrote:
On 06/14/14 14:50, Eric Blake wrote:
We have a policy of avoiding enum types in structs in our public API, because it is possible for a client to choose compiler options that can change the in-memory ABI of that struct based on whether the enum value occupies an int or a minimal size. But we missed this for virDomainBlockJobInfo. We got lucky on little-endian machines - if the enum fits minimal size (a char), we still end up padding to the next long before the next field; but on big-endian, a client interpreting the enum as a char would always see 0 when the server supplies contents as an int.
While at it, I noticed that the web page lacked documentation: http://libvirt.org/html/libvirt-libvirt.html#virDomainBlockJobType not only for the recently added active commit, but also for all the other job types.
"While at it" is often an indication that a patch does too much at once.
VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, + /* Active Block Commit (virDomainBlockCommit with flags), job + * exists as long as sync is active */
#ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
ACK to the above hunk,
So I'll split this into 2, and apply the doc patches separately from...
@@ -2537,7 +2544,7 @@ typedef unsigned long long virDomainBlockJobCursor;
typedef struct _virDomainBlockJobInfo virDomainBlockJobInfo; struct _virDomainBlockJobInfo { - virDomainBlockJobType type; + int type; /* virDomainBlockJobType */ unsigned long bandwidth; /* * The following fields provide an indication of block job progress. @cur
I don't object to this one, but I'd like to see a second opinion.
...the ABI change. But as Dan has agreed that we are not breaking ABI on little-endian, and fixing a real bug on big-endian, I'll go ahead and push both patches shortly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa