On Wed, Jun 19, 2019 at 10:50:14AM +0200, Christophe de Dinechin wrote:
>
>
> > On 18 Jun 2019, at 19:01, Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:
> >
> > On Tue, Jun 18, 2019 at 06:33:01PM +0200, Christophe de Dinechin wrote:
> >>
> >>
> >>> On 18 Jun 2019, at 17:43, Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:
> >>>
> >>> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> >>> ---
> >>> network.go | 80 ++++++++++++++
> >>> network_port.go | 233 ++++++++++++++++++++++++++++++++++++++++
> >>> network_port_compat.h | 67 ++++++++++++
> >>> network_port_wrapper.go | 197 +++++++++++++++++++++++++++++++++
> >>> network_port_wrapper.h | 79 ++++++++++++++
> >>> network_wrapper.go | 73 +++++++++++++
> >>> network_wrapper.h | 23 ++++
> >>> 7 files changed, 752 insertions(+)
> >>> create mode 100644 network_port.go
> >>> create mode 100644 network_port_compat.h
> >>> create mode 100644 network_port_wrapper.go
> >>> create mode 100644 network_port_wrapper.h
> >
> >>> +package libvirt
> >>> +
> >>> +/*
> >>> +#cgo pkg-config: libvirt
> >>> +#include <assert.h>
> >>> +#include "network_port_wrapper.h"
> >>> +
> >>> +virNetworkPtr
> >>> +virNetworkPortGetNetworkWrapper(virNetworkPortPtr port,
> >>> + virErrorPtr err)
> >>> +{
> >>> +#if LIBVIR_VERSION_NUMBER < 5005000
> >>> + assert(0); // Caller should have checked version
> >>
> >> What about
> >>
> >> assert(!”C function not available in this version - Caller should have checked version”);
> >>
> >> ?
> >
> > That's certainly possible, but I'm not sure its worth bothering with
> > since this should never be hit in practice & if it is hit we'll see
> > the offending code in the crash dump easily enough.
>
> By construction, this case happens for a non-libvirt developer.
> The stack trace is useless, nothing in the names there explains
> that it’s a version mismatch. So the non-libvirt Go developer will need
> to install C source code just to be able to read a comment that could
> have been shown in the crash? To me, it looks like like the bother
> is much higher on their side than on yours.
>
> Also, given that the sole purpose of these wrappers is to fail if
> the underlying C function does not exist, would it make sense to
> just not emit the wrapper in that case? That would lead to a link-time
> failure that might be even more friendly, insofar as it catches things earlier.
I'm not sure how familiar you are with Go, so I'll explain the reason why
it is done this way...
example:
func (n *NetworkPort) Delete(flags uint) error {
return nil
}
These functions will always exist from the view of a developers calling
code, as there is no practical way to have conditionally defined Go code
on a per-function basis. The only conditional builds are per-file level
in Go and even that's practically unusable.
So the first thing our Go code does is check the libvirt native library
version number and report an error
eg
func (n *NetworkPort) Delete(flags uint) error {
if C.LIBVIR_VERSION_NUMBER < 5005000 {
return makeNotImplementedError("virNetworkPortDelete")
}
Then, the Go code will call into the C wrapper function we define...
var err C.virError
result := C.virNetworkPortDeleteWrapper(n.ptr, C.uint(flags), &err)
if result == -1 {
return makeError(&err)
}
Since there's no conditional compilation at the Go level the call to
virNetworkPortDeleteWrapper will always be compiled,.
Thus the C wrapper function must always exist despite the fact that it
will not be called when using old libvirt. Hence we have
int
virNetworkPortDeleteWrapper(virNetworkPortPtr port,
unsigned int flags,
virErrorPtr err)
{
#if LIBVIR_VERSION_NUMBER < 5005000
assert(0); // Caller should have checked version
#else
int ret;
ret = virNetworkPortDelete(port, flags);
if (ret < 0) {
virCopyLastError(err);
}
return ret;
#endif
}
The assert is dead code - it will never run - we just have it there as
a sanity check. Any usage by application developers will get stopped
at the Go level and turned into a friendly error message.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
