Eric Blake
2022-Sep-01 13:07 UTC
[Libguestfs] [libnbd PATCH] RFC: api: Add nbd_supports_vsock()
On Thu, Sep 01, 2022 at 08:11:17AM +0200, Laszlo Ersek wrote:> On 08/31/22 21:51, Eric Blake wrote: > > Similar to nbd_supports_tls(), it is nice to know from a feature probe > > whether we are likely to have VSOCK support before even trying more > > expensive APIs like nbd_connect_uri("nbd+vsock://..."). > > > > --- > > > > https://bugzilla.redhat.com/show_bug.cgi?id=2069558 documents a case > > where AF_VSOCK is compiled, but vsock still doesn't work because > > vsock_loopback is not loaded. Should we make nbd_supports_vsock() > > return true only when ALL aspects of vsock are known to be working? > > --- > > generator/API.ml | 25 ++++++++++++++++++++++--- > > lib/handle.c | 17 +++++++++++++++++ > > 2 files changed, 39 insertions(+), 3 deletions(-) > > I think the nbd_supports_tls(3) manual is a good example to follow here: > > "Returns true if libnbd was compiled with gnutls which is required to > support TLS encryption, or false if not." > > It says "necessary" but does not say "sufficient", and the distinction > is clear, too. > > So I think the present patch is good, but we should perhaps add a hint > to longdesc about the necessary kernel modules. Rich always says that > error reports and such should recommend actions for the user, and I > think extending longdesc with kernel module hints should cover that. > (Making nbd_supports_vsock() check for the kernel module, but *not* > reporting it human-readably as the failure reason, would be too obscure > IMO.)How about the following wording: Returns true if libnbd was compiled with support for the C<AF_VSOCK> family of sockets, or false if not. Note that on the Linux operating system, this returns true if there is compile-time support, but you may still need runtime support for some aspects of AF_VSOCK usage; for example, use of C<VMADDR_CID_LOCAL> as the server name requires that the I<vsock_loopback> kernel module is loaded.> > So with the longdesc expansion: > > Acked-by: Laszlo Ersek <lersek at redhat.com>Thanks; I'll wait a bit to see if you like my updated wording first. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2022-Sep-01 16:33 UTC
[Libguestfs] [libnbd PATCH] RFC: api: Add nbd_supports_vsock()
On Thu, Sep 01, 2022 at 08:07:33AM -0500, Eric Blake wrote:> On Thu, Sep 01, 2022 at 08:11:17AM +0200, Laszlo Ersek wrote: > > On 08/31/22 21:51, Eric Blake wrote: > > > Similar to nbd_supports_tls(), it is nice to know from a feature probe > > > whether we are likely to have VSOCK support before even trying more > > > expensive APIs like nbd_connect_uri("nbd+vsock://..."). > > > > > > --- > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=2069558 documents a case > > > where AF_VSOCK is compiled, but vsock still doesn't work because > > > vsock_loopback is not loaded. Should we make nbd_supports_vsock() > > > return true only when ALL aspects of vsock are known to be working? > > > --- > > > generator/API.ml | 25 ++++++++++++++++++++++--- > > > lib/handle.c | 17 +++++++++++++++++ > > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > I think the nbd_supports_tls(3) manual is a good example to follow here: > > > > "Returns true if libnbd was compiled with gnutls which is required to > > support TLS encryption, or false if not." > > > > It says "necessary" but does not say "sufficient", and the distinction > > is clear, too. > > > > So I think the present patch is good, but we should perhaps add a hint > > to longdesc about the necessary kernel modules. Rich always says that > > error reports and such should recommend actions for the user, and I > > think extending longdesc with kernel module hints should cover that. > > (Making nbd_supports_vsock() check for the kernel module, but *not* > > reporting it human-readably as the failure reason, would be too obscure > > IMO.) > > How about the following wording: > > Returns true if libnbd was compiled with support for the C<AF_VSOCK> > family of sockets, or false if not. Note that on the Linux > operating system, this returns true if there is compile-time > support, but you may still need runtime support for some aspects > of AF_VSOCK usage; for example, use of C<VMADDR_CID_LOCAL> as the > server name requires that the I<vsock_loopback> kernel module is > loaded.You can use paragraphs (and even subheadings) in the POD here if you want. Anyway, looks good with the enhanced documentation so: Reviewed-by: Richard W.M. Jones <rjones at redhat.com> Rich.> > > > > So with the longdesc expansion: > > > > Acked-by: Laszlo Ersek <lersek at redhat.com> > > Thanks; I'll wait a bit to see if you like my updated wording first. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW