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(-)
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.