Re: [libvirt] [go PATCH] Add support for virNetworkPort object and APIs
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(a)redhat.com> 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(a)redhat.com>
wrote:
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
>>> ---
>>> 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 :|