9:32 a.m.
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
9:32 a.m.
New subject: [libvirt] [libvirt-go PATCH 1/3] Introduce DomainSnapshotXMLFlags constant
Signed-off-by: Erik Skultety <eskultet(a)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
9:32 a.m.
New subject: [libvirt] [libvirt-go PATCH 2/3] Introduce DomainSaveImageXMLFlags constant
Signed-off-by: Erik Skultety <eskultet(a)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#virDomainManagedSave...
-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
9:32 a.m.
New subject: [libvirt] [libvirt-go PATCH 3/3] Enforce new flags types in DomainSaveImageGetXMLDesc and GetXMLDesc
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(a)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#virDomainSaveImageGe...
-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#virDomainSn...
-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
10:43 a.m.
New subject: [libvirt] [libvirt-go PATCH 3/3] Enforce new flags types in DomainSaveImageGetXMLDesc and GetXMLDesc
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
10:39 a.m.
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
10:46 a.m.
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 :|
10:43 a.m.
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(a)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 :|
10:54 a.m.
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
10:59 a.m.
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 :|
5:09 a.m.
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(a)redhat.com>
Pushed.
Thanks,
Erik
5:14 a.m.
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 :|
5:42 a.m.
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
6:01 a.m.
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 :|
2098
days inactive
2101
days old
13 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Erik Skultety