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