[libvirt] [libvirt-go PATCH 0/3] Introduce recent DomainSnapshotXML and DomainSaveImageXML flags
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags. Erik Skultety (3): Introduce DomainSnapshotXMLFlags constant Introduce DomainSaveImageXMLFlags constant Enforce new flags types in DomainSaveImageGetXMLDesc and GetXMLDesc connect.go | 2 +- domain.go | 8 +++++++- domain_compat.h | 8 ++++++++ domain_snapshot.go | 8 +++++++- domain_snapshot_wrapper.h | 2 +- 5 files changed, 24 insertions(+), 4 deletions(-) -- 2.20.1
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- domain_compat.h | 4 ++++ domain_snapshot.go | 6 ++++++ domain_snapshot_wrapper.h | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/domain_compat.h b/domain_compat.h index 9a30e8f..d5a3f78 100644 --- a/domain_compat.h +++ b/domain_compat.h @@ -948,4 +948,8 @@ struct _virDomainInterface { #define VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY (1 << 0) #endif +#ifndef VIR_DOMAIN_SNAPSHOT_XML_SECURE +#define VIR_DOMAIN_SNAPSHOT_XML_SECURE (1 << 0) +#endif + #endif /* LIBVIRT_GO_DOMAIN_COMPAT_H__ */ diff --git a/domain_snapshot.go b/domain_snapshot.go index 65fbbb5..86c7c51 100644 --- a/domain_snapshot.go +++ b/domain_snapshot.go @@ -84,6 +84,12 @@ const ( DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY = DomainSnapshotDeleteFlags(C.VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) ) +type DomainSnapshotXMLFlags int + +const ( + DOMAIN_SNAPSHOT_XML_SECURE = DomainSnapshotXMLFlags(C.VIR_DOMAIN_SNAPSHOT_XML_SECURE) +) + type DomainSnapshot struct { ptr C.virDomainSnapshotPtr } diff --git a/domain_snapshot_wrapper.h b/domain_snapshot_wrapper.h index fcf8036..b2e5cc5 100644 --- a/domain_snapshot_wrapper.h +++ b/domain_snapshot_wrapper.h @@ -28,7 +28,7 @@ #include <libvirt/libvirt.h> #include <libvirt/virterror.h> - +#include "domain_compat.h" int virDomainRevertToSnapshotWrapper(virDomainSnapshotPtr snapshot, -- 2.20.1
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- domain.go | 8 +++++++- domain_compat.h | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/domain.go b/domain.go index 91b8399..9211b6e 100644 --- a/domain.go +++ b/domain.go @@ -817,6 +817,12 @@ const ( MIGRATE_MAX_SPEED_POSTCOPY = DomainMigrateMaxSpeedFlags(C.VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY) ) +type DomainSaveImageXMLFlags int + +const ( + VIR_DOMAIN_SAVE_IMAGE_XML_SECURE = DomainSaveImageXMLFlags(C.VIR_DOMAIN_SAVE_IMAGE_XML_SECURE) +) + type VcpuState int const ( @@ -4677,7 +4683,7 @@ func (d *Domain) ManagedSaveDefineXML(xml string, flags uint32) error { } // See also https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSaveGet... -func (d *Domain) ManagedSaveGetXMLDesc(flags uint32) (string, error) { +func (d *Domain) ManagedSaveGetXMLDesc(flags DomainSaveImageXMLFlags) (string, error) { if C.LIBVIR_VERSION_NUMBER < 3007000 { return "", makeNotImplementedError("virDomainManagedSaveGetXMLDesc") } diff --git a/domain_compat.h b/domain_compat.h index d5a3f78..be6f1c4 100644 --- a/domain_compat.h +++ b/domain_compat.h @@ -952,4 +952,8 @@ struct _virDomainInterface { #define VIR_DOMAIN_SNAPSHOT_XML_SECURE (1 << 0) #endif +#ifndef VIR_DOMAIN_SAVE_IMAGE_XML_SECURE +#define VIR_DOMAIN_SAVE_IMAGE_XML_SECURE (1 << 0) +#endif + #endif /* LIBVIRT_GO_DOMAIN_COMPAT_H__ */ -- 2.20.1
This breaks API compatibility! Recently libvirt introduced 2 new separate flag enums in order to stop recycling the old virDomainXMLFlags since 2/3 flags were discouraged to use. While it's fine for libvirt to introduce such a change since it uses plain 'int' for flags, not so much for the Go bindings which already enforced the now deprecated DomainXMLFlags type. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- connect.go | 2 +- domain_snapshot.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/connect.go b/connect.go index f77d10d..0d5118c 100644 --- a/connect.go +++ b/connect.go @@ -2038,7 +2038,7 @@ func (c *Connect) DomainSaveImageDefineXML(file string, xml string, flags Domain } // See also https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainSaveImageGetXM... -func (c *Connect) DomainSaveImageGetXMLDesc(file string, flags DomainXMLFlags) (string, error) { +func (c *Connect) DomainSaveImageGetXMLDesc(file string, flags DomainSaveImageXMLFlags) (string, error) { cfile := C.CString(file) defer C.free(unsafe.Pointer(cfile)) diff --git a/domain_snapshot.go b/domain_snapshot.go index 86c7c51..282217f 100644 --- a/domain_snapshot.go +++ b/domain_snapshot.go @@ -161,7 +161,7 @@ func (s *DomainSnapshot) HasMetadata(flags uint32) (bool, error) { } // See also https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnaps... -func (s *DomainSnapshot) GetXMLDesc(flags DomainXMLFlags) (string, error) { +func (s *DomainSnapshot) GetXMLDesc(flags DomainSnapshotXMLFlags) (string, error) { var err C.virError result := C.virDomainSnapshotGetXMLDescWrapper(s.ptr, C.uint(flags), &err) if result == nil { -- 2.20.1
On 2/22/19 9:32 AM, Erik Skultety wrote:
This breaks API compatibility! Recently libvirt introduced 2 new separate flag enums in order to stop recycling the old virDomainXMLFlags since 2/3 flags were discouraged to use. While it's fine for libvirt to introduce such a change since it uses plain 'int' for flags, not so much for the Go bindings which already enforced the now deprecated DomainXMLFlags type.
And a quick google search says that Go lacks function overloading (use of variadic functions to provide optional arguments being as much as it is willing to support). Can you declare some sort of meta-type which is a union between the old enum and new enum types, where you can pass in either enum value and still satisfy the function type? Otherwise, I don't know Go well enough to make any suggestions on how to avoid the API break. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/22/19 9:32 AM, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Oh bummer - I didn't realize that some of the other language bindings are more strongly typed than our C APIs. We should probably audit to see if we have any other APIs with foolishly reused enum types that should instead be given their own flag type. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Feb 22, 2019 at 10:39:06AM -0600, Eric Blake wrote:
On 2/22/19 9:32 AM, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Oh bummer - I didn't realize that some of the other language bindings are more strongly typed than our C APIs. We should probably audit to see if we have any other APIs with foolishly reused enum types that should instead be given their own flag type.
Go isn't inherantly more strongly typed. We could have just stuck with uint32 for the Go bindings following libvirt, but I made the concious decision to define Go types for all flags and use them in methods. Generally this isn't a problem as changing from uint32 to a strong type is backcompatible when we've had an unsed flags argument for a method. This situation where we changed the enum for flag for an existing API is thankfully rare, and the benefits of strong typing thus outweighs the occassional API breakage cost. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level. On balance I think that is still the right tradeoff to have stronger type checking as it catches real errors in app code. Due to the widespread use of "vendoring" in the Go community where apps fixate on specific git commit hashes of their dependandancies, the fallout is more limited and will mostly only impact devs at the time they decide to explicitly sync to newer git. So for all three Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/22/19 10:43 AM, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
On balance I think that is still the right tradeoff to have stronger type checking as it catches real errors in app code.
I'm fine with that decision. I will note, however, that this website suggests an interesting approach: http://changelog.ca/log/2015/01/30/golang A variadic function using ...interface{} can accept arbitrary types for the optional parameters, and then open-code its own type-checking to ensure that the user complied with the intended use-cases. If the flags parameter can be accepted through that type of trick, coupled with validation that the caller never passed more than one argument for flags, and that the passed argument is typed solely as one of the two expected enums, then you can maintain API compatiblity, even though the ABI changes to a variadic instead of a strictly-typed flags argument. I don't know if it is worth doing, though.
Due to the widespread use of "vendoring" in the Go community where apps fixate on specific git commit hashes of their dependandancies, the fallout is more limited and will mostly only impact devs at the time they decide to explicitly sync to newer git.
And this is a good argument for not being upset about the API break and therefore not worrying about trying to use variadic functions to give back-compat. (For the record, the nbdkit project specifically documents that its C API/ABI will be kept compatible, but warns that other language bindings API may break, precisely to account for cases like this where there is a mismatch between C semantics and the slightly different idioms of the other languages). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Feb 22, 2019 at 10:54:10AM -0600, Eric Blake wrote:
On 2/22/19 10:43 AM, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
On balance I think that is still the right tradeoff to have stronger type checking as it catches real errors in app code.
I'm fine with that decision. I will note, however, that this website suggests an interesting approach:
http://changelog.ca/log/2015/01/30/golang
A variadic function using ...interface{} can accept arbitrary types for the optional parameters, and then open-code its own type-checking to ensure that the user complied with the intended use-cases. If the flags parameter can be accepted through that type of trick, coupled with validation that the caller never passed more than one argument for flags, and that the passed argument is typed solely as one of the two expected enums, then you can maintain API compatiblity, even though the ABI changes to a variadic instead of a strictly-typed flags argument. I don't know if it is worth doing, though.
Yes interface{} is a clever trick serving a vaguely similar purpose to allowing a 'void*' in C, but with benefit that the method can actually do some real type checking. The downside is that this is now runtime type checking, not static compile time. So I think on balance I'd still prefer the static checking and accept the breakage, as it is a win in the long term with only small amount of short term pain.
Due to the widespread use of "vendoring" in the Go community where apps fixate on specific git commit hashes of their dependandancies, the fallout is more limited and will mostly only impact devs at the time they decide to explicitly sync to newer git.
And this is a good argument for not being upset about the API break and therefore not worrying about trying to use variadic functions to give back-compat.
(For the record, the nbdkit project specifically documents that its C API/ABI will be kept compatible, but warns that other language bindings API may break, precisely to account for cases like this where there is a mismatch between C semantics and the slightly different idioms of the other languages).
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Feb 22, 2019 at 04:43:25PM +0000, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
How would this affect the ABI? Both the old and the new enum have the same int value 0x01, with the only difference that you could have fed a few more enums into the API even though we documented that they were unsupported for that usage.
On balance I think that is still the right tradeoff to have stronger type checking as it catches real errors in app code.
Due to the widespread use of "vendoring" in the Go community where apps fixate on specific git commit hashes of their dependandancies, the fallout is more limited and will mostly only impact devs at the time they decide to explicitly sync to newer git.
So for all three
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Pushed. Thanks, Erik
On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 04:43:25PM +0000, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
How would this affect the ABI? Both the old and the new enum have the same int value 0x01, with the only difference that you could have fed a few more enums into the API even though we documented that they were unsupported for that usage.
Previously an application would have done dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE) With your change pushed, this becomes a compile error demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc because the DomainXMLFlags enum type doesn't match the new DomainSaveImageXMLFlags enum type. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 25, 2019 at 11:14:41AM +0000, Daniel P. Berrangé wrote:
On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 04:43:25PM +0000, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
How would this affect the ABI? Both the old and the new enum have the same int value 0x01, with the only difference that you could have fed a few more enums into the API even though we documented that they were unsupported for that usage.
Previously an application would have done
dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE)
With your change pushed, this becomes a compile error
demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc
because the DomainXMLFlags enum type doesn't match the new DomainSaveImageXMLFlags enum type.
Yes, but if it's a compile time error, it's static type checking, isn't that still API breakage? I'd understand if e.g. it was a public structure and the project we'd expand the structure or added padding where the type is still the same, so compilation is okay, but from binary POV it would not... Thanks for clarifying, Erik
On Mon, Feb 25, 2019 at 12:42:58PM +0100, Erik Skultety wrote:
On Mon, Feb 25, 2019 at 11:14:41AM +0000, Daniel P. Berrangé wrote:
On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 04:43:25PM +0000, Daniel P. Berrangé wrote:
On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote:
Unfortunately, in order to support the new flags, the last patch introduces an API breakage as the convention we use for the bindings is to also enforce types for flags.
Yes, unfortunately this need to break ABI is fallout resulting from my decision to make the Go bindings do stricter enum validation that we have had at the C level.
How would this affect the ABI? Both the old and the new enum have the same int value 0x01, with the only difference that you could have fed a few more enums into the API even though we documented that they were unsupported for that usage.
Previously an application would have done
dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE)
With your change pushed, this becomes a compile error
demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc
because the DomainXMLFlags enum type doesn't match the new DomainSaveImageXMLFlags enum type.
Yes, but if it's a compile time error, it's static type checking, isn't that still API breakage? I'd understand if e.g. it was a public structure and the project we'd expand the structure or added padding where the type is still the same, so compilation is okay, but from binary POV it would not...
Oh, i missed that you wrote 'ABI' rather than 'API'. ABI is probably ok unless function param types result in symbol mangling like C++ does, but I've not checked that. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Erik Skultety