Libguestfs - Jun 2019 - Re: [PATCH libnbd] api: nbd_get_version, nbd_supports_uri and nbd_get_package_name.

nbd_get_version returns the library version as a string.

nbd_supports_uri returns whether or not the library was compiled with
NBD URI support (ie. with libxml2).

nbd_get_package_name is fairly useless as it always returns the string
"libnbd", however it replaces a function that was written for the
Python bindings.

These take a handle parameter but don't need to use it.  Changing the
generator to support a whole new class of API calls which don't need a
handle is a massive pain.
---
 generator/generator | 51 +++++++++++++++++++++++++++++++++++++++++----
 lib/handle.c        | 22 +++++++++++++++++++
 python/handle.c     | 22 -------------------
 3 files changed, 69 insertions(+), 26 deletions(-)

diff --git a/generator/generator b/generator/generator
index 54dc6cc..5e052df 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1823,6 +1823,49 @@ can be used for debugging or troubleshooting, but you
should not
 rely on the state of connections since it may change in future
 versions.";
   };
+
+  "get_package_name", {
+    default_call with
+    args = []; ret = RConstString; is_locked = false; may_set_error = false;
+    shortdesc = "return the name of the library";
+    longdesc = "\
+Returns the name of the library, always C<\"libnbd\"> unless
+the library was modified with another name at compile time.";
+  };
+
+  "get_version", {
+    default_call with
+    args = []; ret = RConstString; is_locked = false; may_set_error = false;
+    shortdesc = "return a descriptive string for the state of the
connection";
+    longdesc = "\
+Return the version of libnbd.  This is returned as a string
+in the form C<\"major.minor.release\"> where each of major,
minor
+and release is a small positive integer.  For example
C<\"1.0.3\">.
+
+The major number is C<0> for the early experimental versions of
+libnbd where we still had an unstable API, or C<1> for the versions
+of libnbd with a long-term stable API and ABI.
+
+The minor number is even (C<0>, C<2>, etc) for stable releases,
+and odd (C<1>, C<3>, etc) for development versions.  Note that
+new APIs added in a development version remain experimental
+and subject to change in that branch until they appear in a stable
+release.
+
+The release number is increments for each release along a particular
+branch.";
+  };
+
+  "supports_uri", {
+    default_call with
+    args = []; ret = RBool; is_locked = false; may_set_error = false;
+    shortdesc = "return true if libnbd was compiled with support for NBD
URIs";
+    longdesc = "\
+Returns true if libnbd was compiled with libxml2 which is required
+to support NBD URIs, or false if not.  See C<nbd_connect_uri> and
+C<nbd_aio_connect_uri>.";
+  };
+
 ]
 
 (* Constants, flags, etc. *)
@@ -3007,7 +3050,7 @@ get_handle (PyObject *obj)
     fun name ->
       pr "extern PyObject *nbd_internal_py_%s (PyObject *self, PyObject
*args);\n"
          name;
-  ) ([ "create"; "close"; "get_package_name";
"get_package_version";
+  ) ([ "create"; "close";
        "alloc_aio_buffer"; "aio_buffer_from_bytearray";
        "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
 
@@ -3035,7 +3078,7 @@ let generate_python_libnbdmod_c ()      fun name ->
       pr "  { (char *) \"%s\", nbd_internal_py_%s, METH_VARARGS,
NULL },\n"
          name name;
-  ) ([ "create"; "close"; "get_package_name";
"get_package_version";
+  ) ([ "create"; "close";
        "alloc_aio_buffer"; "aio_buffer_from_bytearray";
        "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
   pr "  { NULL, NULL, 0, NULL }\n";
@@ -3607,8 +3650,8 @@ class NBD (object):
 
   (* For nbdsh. *)
   pr "\
-package_name = libnbdmod.get_package_name ()
-__version__ = libnbdmod.get_package_version ()
+package_name = NBD().get_package_name()
+__version__ = NBD().get_version()
 
 if __name__ == \"__main__\":
     import argparse
diff --git a/lib/handle.c b/lib/handle.c
index a42b8dc..cc311ba 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -214,3 +214,25 @@ nbd_add_close_callback (struct nbd_handle *h,
nbd_close_callback cb, void *data)
   pthread_mutex_unlock (&h->lock);
   return ret;
 }
+
+const char *
+nbd_unlocked_get_package_name (struct nbd_handle *h)
+{
+  return PACKAGE_NAME;
+}
+
+const char *
+nbd_unlocked_get_version (struct nbd_handle *h)
+{
+  return PACKAGE_VERSION;
+}
+
+int
+nbd_unlocked_supports_uri (struct nbd_handle *h)
+{
+#ifdef HAVE_LIBXML2
+  return 1;
+#else
+  return 0;
+#endif
+}
diff --git a/python/handle.c b/python/handle.c
index 7b5e139..7cff41a 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -89,28 +89,6 @@ free_aio_buffer (PyObject *capsule)
   free (buf);
 }
 
-PyObject *
-nbd_internal_py_get_package_name (PyObject *self, PyObject *args)
-{
-  static char name[] = PACKAGE_NAME;
-
-  if (!PyArg_ParseTuple (args, (char *)
":nbd_internal_py_get_package_name"))
-    return NULL;
-
-  return PyUnicode_FromStringAndSize (name, strlen (name));
-}
-
-PyObject *
-nbd_internal_py_get_package_version (PyObject *self, PyObject *args)
-{
-  static char version[] = PACKAGE_VERSION;
-
-  if (!PyArg_ParseTuple (args, (char *)
":nbd_internal_py_get_package_version"))
-    return NULL;
-
-  return PyUnicode_FromStringAndSize (version, strlen (version));
-}
-
 /* Allocate a persistent buffer used for nbd_aio_pread and
  * nbd_aio_pwrite.
  */
-- 
2.21.0

On Mon, Jun 03, 2019 at 10:59:22AM +0100, Richard W.M. Jones
wrote:
> +The release number is increments for each release along a particular
> +branch.";
"is incremented" - fixed in my local copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 6/3/19 4:59 AM, Richard W.M. Jones wrote:
> nbd_get_version returns the library version as a string.
> 
> nbd_supports_uri returns whether or not the library was compiled with
> NBD URI support (ie. with libxml2).
> 
> nbd_get_package_name is fairly useless as it always returns the string
> "libnbd", however it replaces a function that was written for the
> Python bindings.
> 
> These take a handle parameter but don't need to use it.  Changing the
> generator to support a whole new class of API calls which don't need a
> handle is a massive pain.
Should we permit and/or document that these functions may pass NULL (at
least in C bindings) for the nbd_handle?  (It's harder in other
languages, where you would treat it as more of a static method rather
than an instance method - hence I agree with your comment that
refactoring the generator to support that is harder)

Looks good to me.

I agree that supports_uri should be a runtime question. Having the
version as a runtime answer is sane enough (even if being a string
requires a bit of parsing to interpret the version). There's still the
question if additionally exposing the version as a compile-time constant
can also make it easier to conditionally use other API (such as
nbd_supports_uri) based on version-number checks for whether it is known
to be present (not as nice as a configure-check for the actual function,
in case it was backported across version numbers, but having
compile-time constants allows skipping the development of the
configure-time check).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Jun 03, 2019 at 10:12:05AM -0500, Eric Blake
wrote:
> On 6/3/19 4:59 AM, Richard W.M. Jones wrote:
> > nbd_get_version returns the library version as a string.
> > 
> > nbd_supports_uri returns whether or not the library was compiled with
> > NBD URI support (ie. with libxml2).
> > 
> > nbd_get_package_name is fairly useless as it always returns the string
> > "libnbd", however it replaces a function that was written
for the
> > Python bindings.
> > 
> > These take a handle parameter but don't need to use it.  Changing
the
> > generator to support a whole new class of API calls which don't
need a
> > handle is a massive pain.
> 
> Should we permit and/or document that these functions may pass NULL (at
> least in C bindings) for the nbd_handle?  (It's harder in other
> languages, where you would treat it as more of a static method rather
> than an instance method - hence I agree with your comment that
> refactoring the generator to support that is harder)
We'd have to be careful in case we made some change to the generator
which broke that contract.  It would at the very least require a test.
It certainly wouldn't work right now in the two non-C languages we've
implemented so far (Python would hopefully fail the runtime type
check, OCaml wouldn't let you compile the code in the first place).
> Looks good to me.
> 
> I agree that supports_uri should be a runtime question. Having the
> version as a runtime answer is sane enough (even if being a string
> requires a bit of parsing to interpret the version). There's still the
> question if additionally exposing the version as a compile-time constant
> can also make it easier to conditionally use other API (such as
> nbd_supports_uri) based on version-number checks for whether it is known
> to be present (not as nice as a configure-check for the actual function,
> in case it was backported across version numbers, but having
> compile-time constants allows skipping the development of the
> configure-time check).
In libguestfs we automatically generate defines like

  #define GUESTFS_HAVE_<function_name> 1

for all functions which are present in the API.  Easy enough to do
with a generator.

I'd also like to get symbol versioning working at some point.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top