[libvirt] remote generator: Legacy support for hyper to long mappings

[Matthias Bolte]

2011/5/31 Eric Blake <eblake at redhat.com>:
> On 05/30/2011 07:03 AM, Matthias Bolte wrote:
>
>> From 726dae7b4c21d4c17ac19808c06d7fc978b36778 Mon Sep 17 00:00:00 2001
>> From: Matthias Bolte <matthias.bolte at googlemail.com>
>> Date: Mon, 30 May 2011 12:58:57 +0200
>> Subject: [PATCH] remote generator: Legacy support for hyper to long mappings
>>
>> Remove some special case code that took care of mapping hyper to the
>> correct C types.
>
> I like patch 1B better than 1A, so that's what I'm reviewing here.
>
>> +++ b/configure.ac
>> @@ -117,6 +117,7 @@ if test "x$have_cpuid" = xyes; then
>>  fi
>>  AC_MSG_RESULT([$have_cpuid])
>>
>> +AC_CHECK_SIZEOF(long)
>
> AC_CHECK_SIZEOF([long])
>
> so that we follow the autoconf recommended quoting rules.
>
>> +++ b/daemon/remote_generator.pl
>> @@ -174,6 +174,58 @@ while (<PROTOCOL>) {
>>
>>  close(PROTOCOL);
>>
>> +# this dict contains the procedures that are allowed to map [unsigend] hyper
>
> s/unsigend/unsigned/
>
>> +# to [unsigend] long for legacy reasons in their signature and return type.
>> +# this list is fixed. new procedures and public APIs have to map [unsigend]
>> +# hyper to [unsigend] long long
>
> 3 more instances.

Nice copy-n-paste here, isn't it :)

>> +my $long_legacy = {
>> +    DomainGetMaxMemory          => { ret => { memory => 1 } },
>> +    DomainGetInfo               => { ret => { maxMem => 1, memory => 1 } },
>> +    DomainMigrate               => { arg => { flags => 1, resource => 1 } },
>> +    DomainMigrate2              => { arg => { flags => 1, resource => 1 } },
>> +    DomainMigrateBegin3         => { arg => { flags => 1, resource => 1 } },
>
> Dan, DomainMigrate2 and DomainMigrateBegin3 are new APIs as of this
> release (similarly for other Migration v3 commands).  Should these use
> 'long long' rather than 'long' for resource, as well as 'unsigned int'
> for flags, as part of your edict that all new APIs should avoid 'long'?
>  Right _now_ is the time to make this change, before 0.9.2 goes gold.
>
>> +++ b/src/remote/remote_driver.c
>> @@ -87,6 +87,24 @@
>>
>>  #define VIR_FROM_THIS VIR_FROM_REMOTE
>>
>> +#define HYPER_TO_TYPE(_type, _to, _from)                                      \
>
> I'm thinking we should move the definition of HYPER_TO_TYPE inside...
>
>> +    do {                                                                      \
>> +        if ((_from) != (_type)(_from)) {                                      \
>> +            remoteError(VIR_ERR_INTERNAL_ERROR,                               \
>> +                        _("conversion from hyper to %s overflowed"), #_type); \
>> +            goto done;                                                        \
>> +        }                                                                     \
>> +        (_to) = (_from);                                                      \
>> +    } while (0)
>> +
>> +#if SIZEOF_LONG < 8
>
> ...this #if conditional, so that gcc won't complain about the macro
> HYPER_TO_TYPE being unused on 64 bit platforms.  That's a one-liner
> change, but copied into two files.
>
>> +# define HYPER_TO_LONG(_to, _from) HYPER_TO_TYPE(long, _to, _from)
>> +# define HYPER_TO_ULONG(_to, _from) HYPER_TO_TYPE(unsigned long, _to, _from)
>> +#else
>> +# define HYPER_TO_LONG(_to, _from) (_to) = (_from)
>> +# define HYPER_TO_ULONG(_to, _from) (_to) = (_from)
>> +#endif
>> +
>
> ACK with the nits fixed.  I think we can push this in now whether or not
> we have Dan's answer on whether migration v3 calls should avoid 'long',
> because that would be an independent cleanup.

As Dan explained we will stick to long in migration v3 calls.

I addressed your comments and pushed 1B.

Matthias