[libvirt] remote generator: Legacy support for hyper to long mappings
This can be considered as a v2 for hyper annotation patch [1]. Dan argued that hyper-to-long mapping is considered as legacy and hyper should be mapped to long long for all new APIs. Therefore, he voted against the longlong annotation, because this should be the default anyway. I removed the longlong annotations from the original patch. This results in version-A of the attached patch 0001. But this optional annotation makes the parser more complex. One of the reasons for annotating things in the .x is to have single place to define a new procedure. But as the long/longlong annotation only affect exiting procedures and will not be added to new ones it's much simpler to store this information in the generator itself. This information it is fixed and will not change anymore. This is implemented in version-B of the attached patch 0001. I favor version-B, what's your opinion on this? Attached patch 0002 extends the apibuild script to allow the usage of long for the same fixed set as in the remote generator only. It reports an error when new functions or structs use long instead of longlong. [1] https://www.redhat.com/archives/libvir-list/2011-May/msg01434.html Matthias
On 05/30/2011 07:03 AM, Matthias Bolte wrote:
From 726dae7b4c21d4c17ac19808c06d7fc978b36778 Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte@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.
+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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, May 31, 2011 at 11:00:04AM -0600, Eric Blake wrote:
On 05/30/2011 07:03 AM, Matthias Bolte wrote:
From 726dae7b4c21d4c17ac19808c06d7fc978b36778 Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte@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.
+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.
The migration API situation is complex, because the public APIs virDomainMigrate / MigrateToURI, call into different internal methods based on migration protocol. Having the v3 migration protocol use different size types to the v1 and v2 migration protocols would be creating pain for ourselves IMHO. 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 :|
2011/5/31 Eric Blake <eblake@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@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
On 05/30/2011 07:03 AM, Matthias Bolte wrote:
From 3bb05f7543f86e3b47772f0fabecbda4f167b3bc Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte@googlemail.com> Date: Mon, 30 May 2011 14:36:41 +0200 Subject: [PATCH] apibuild: Restrict long usage to existing functions and struct
New APIs have to use long long instead of long.
Also make apibuild errors fatal. --- docs/Makefile.am | 2 +- docs/apibuild.py | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletions(-)
Now reviewing patch 2, assuming that you go with 1B.
diff --git a/docs/Makefile.am b/docs/Makefile.am index 59ae685..a98f08d 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -179,7 +179,7 @@ $(python_generated_files): $(srcdir)/apibuild.py \ $(srcdir)/../include/libvirt/*.h \ $(srcdir)/../src/libvirt.c \ $(srcdir)/../src/util/virterror.c - -srcdir=$(srcdir) $(srcdir)/apibuild.py + srcdir=$(srcdir) $(srcdir)/apibuild.py
While you're touching this line, how about we also modify it to use $(AM_V_GEN)
+++ b/docs/apibuild.py @@ -1480,6 +1480,77 @@ class CParser: self.signature = signature return token
+ # this dict contains the functions that are allowed to use [unsigend] + # long for legacy reasons in their signature and return type. this list is + # fixed. new procedures and public APIs have to use [unsigend] long long
s/unsigend/unsigned/ twice (copy and paste from the last patch? :)
+ long_legacy_functions = \ + { "virGetVersion" : (False, ("libVer", "typeVer")), + "virConnectGetLibVersion" : (False, ("libVer")), + "virConnectGetVersion" : (False, ("hvVer")), + "virDomainGetMaxMemory" : (True, ()), + "virDomainMigrate" : (False, ("flags", "bandwidth")), + "virDomainMigrate2" : (False, ("flags", "bandwidth")), + "virDomainMigrateBegin3" : (False, ("flags", "bandwidth")),
Same query to danpb about whether new migration v3 APIs should avoid 'long', and same independent patch to fix that issue.
+ + # this dict contains the structs that are allowed to use [unsigend] + # long for legacy reasons. this list is fixed. new structs have to use + # [unsigend] long long
s/unsigend/unsigned/ twice ACK with the spelling nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/5/31 Eric Blake <eblake@redhat.com>:
On 05/30/2011 07:03 AM, Matthias Bolte wrote:
From 3bb05f7543f86e3b47772f0fabecbda4f167b3bc Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte@googlemail.com> Date: Mon, 30 May 2011 14:36:41 +0200 Subject: [PATCH] apibuild: Restrict long usage to existing functions and struct
New APIs have to use long long instead of long.
Also make apibuild errors fatal. --- docs/Makefile.am | 2 +- docs/apibuild.py | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletions(-)
Now reviewing patch 2, assuming that you go with 1B.
diff --git a/docs/Makefile.am b/docs/Makefile.am index 59ae685..a98f08d 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -179,7 +179,7 @@ $(python_generated_files): $(srcdir)/apibuild.py \ $(srcdir)/../include/libvirt/*.h \ $(srcdir)/../src/libvirt.c \ $(srcdir)/../src/util/virterror.c - -srcdir=$(srcdir) $(srcdir)/apibuild.py + srcdir=$(srcdir) $(srcdir)/apibuild.py
While you're touching this line, how about we also modify it to use $(AM_V_GEN)
+++ b/docs/apibuild.py @@ -1480,6 +1480,77 @@ class CParser: self.signature = signature return token
+ # this dict contains the functions that are allowed to use [unsigend] + # long for legacy reasons in their signature and return type. this list is + # fixed. new procedures and public APIs have to use [unsigend] long long
s/unsigend/unsigned/ twice (copy and paste from the last patch? :)
Yes, fixed.
+ long_legacy_functions = \ + { "virGetVersion" : (False, ("libVer", "typeVer")), + "virConnectGetLibVersion" : (False, ("libVer")), + "virConnectGetVersion" : (False, ("hvVer")), + "virDomainGetMaxMemory" : (True, ()), + "virDomainMigrate" : (False, ("flags", "bandwidth")), + "virDomainMigrate2" : (False, ("flags", "bandwidth")), + "virDomainMigrateBegin3" : (False, ("flags", "bandwidth")),
Same query to danpb about whether new migration v3 APIs should avoid 'long', and same independent patch to fix that issue.
+ + # this dict contains the structs that are allowed to use [unsigend] + # long for legacy reasons. this list is fixed. new structs have to use + # [unsigend] long long
s/unsigend/unsigned/ twice
ACK with the spelling nits fixed.
Thanks, pushed. Matthias
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte