Libguestfs - Mar 2020 - Re: [PATCH nbdkit 5/9 patch split 2/5] lib: Move code for parsing, passwords and paths into libnbdkit.so.

On 3/26/20 3:13 PM, Richard W.M. Jones wrote:
> ---
Just a reminder in case you want to improve the commit message.
>   lib/Makefile.am               |  27 ++
>   server/Makefile.am            |  24 --
>   server/nbdkit.syms            |  28 +--
>   server/public.c               | 459 +---------------------------------
>   {server => lib}/extents.c     |   4 +-
>   lib/parse.c                   | 341 +++++++++++++++++++++++++
>   lib/password.c                | 152 +++++++++++
>   lib/path.c                    |  97 +++++++
>   {server => lib}/test-public.c |   3 +-
>   .gitignore                    |   2 +-
>   lib/libnbdkit.syms            |  20 ++
>   11 files changed, 645 insertions(+), 512 deletions(-)
This one's still big, but not as daunting. Thanks for doing the split.

> +++ b/server/public.c
> @@ -36,7 +36,6 @@
>   
>   #include <config.h>
>   
> -#include <fcntl.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <stdbool.h>
> @@ -44,6 +43,7 @@
>   #include <inttypes.h>
>   #include <string.h>
>   #include <unistd.h>
> +#include <fcntl.h>
>   #include <limits.h>
Intentional churn?
> diff --git a/server/test-public.c b/lib/test-public.c
> similarity index 99%
> rename from server/test-public.c
> rename to lib/test-public.c
> index fe347d44..d680e7ad 100644
> --- a/server/test-public.c
> +++ b/lib/test-public.c
> @@ -41,7 +41,8 @@
>   #include <string.h>
>   #include <unistd.h>
>   
> -#include "internal.h"
> +#include "nbdkit-plugin.h"
> +#include "nbdkit-filter.h"
>   
I like how this .c file move worked out.

Should the test-public binary still be compiled from direct files under 
lib/, or should it be linked against libnbdkit.so?  If I read the 
Makefile changes correctly, you went with the former.  I'm wondering if 
we want to add some sort of 'assert(is_initialized)' to all of our 
public entry functions to ensure that no one is actually trying to use 
libnbdkit.so without having first gone through nbdkit_private_init; if 
we did that, then test-public should indeed build from direct files 
rather than trying to link against libnbdkit.so and bypass the 
initialization normally done by the nbdkit binary.

> +++ b/lib/libnbdkit.syms
> @@ -38,6 +38,26 @@
>   {
>     # The functions we want plugins and filters to call.
>     global:
> +    nbdkit_absolute_path;
> +    nbdkit_add_extent;
> +    nbdkit_extents_count;
> +    nbdkit_extents_free;
> +    nbdkit_extents_new;
> +    nbdkit_get_extent;
> +    nbdkit_parse_bool;
> +    nbdkit_parse_int16_t;
> +    nbdkit_parse_int32_t;
> +    nbdkit_parse_int64_t;
> +    nbdkit_parse_int8_t;
> +    nbdkit_parse_int;
> +    nbdkit_parse_size;
> +    nbdkit_parse_uint16_t;
> +    nbdkit_parse_uint32_t;
> +    nbdkit_parse_uint64_t;
> +    nbdkit_parse_uint8_t;
> +    nbdkit_parse_unsigned;
> +    nbdkit_read_password;
> +    nbdkit_realpath;
Are we brave enough to list nbdkit_*?  Or is being explicit on each 
symbol still a good idea?

Are we planning on having any nbdkit_debug_* symbols in libnbdkit.so (in 
addition to the ones in the nbdkit server proper), in which case those 
should be exported, too?

Otherwise, LGTM.

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

On Thu, Mar 26, 2020 at 04:35:29PM -0500, Eric Blake
wrote:
> Are we brave enough to list nbdkit_*?  Or is being explicit on each
> symbol still a good idea?
I'd really like to stick with listing symbols explicitly otherwise
it's too easy for an unintended symbol to slip through the net by
accident.  Arguably nbdkit_vfprintf would have been one of those.
> Are we planning on having any nbdkit_debug_* symbols in libnbdkit.so
> (in addition to the ones in the nbdkit server proper), in which case
> those should be exported, too?
nbdkit_debug_* symbols should only be in the server proper.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Thu, Mar 26, 2020 at 04:35:29PM -0500, Eric Blake
wrote:
> Should the test-public binary still be compiled from direct files
> under lib/, or should it be linked against libnbdkit.so?
I will fix this in my copy.
> I'm wondering if we want to add some sort of
> 'assert(is_initialized)' to all of our public entry functions to
> ensure that no one is actually trying to use libnbdkit.so without
> having first gone through nbdkit_private_init;[...]
So firstly I believe this can only be a problem if the server itself
uses __attribute__((constructor)), since plugins and filters are
always dlopened long after main() in the server has run.  Is that
correct?

The only ((constructor)) currently used is in the OCaml plugin.

If the server does use a constructor and calls (eg) nbdkit_debug it
would indeed crash.  However constructors would likely crash in the
pre-change nbdkit because nothing in the server is initialized
properly until main() is running.

Is it worth checking for this still, given it would have an overhead?

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 3/26/20 5:42 PM, Richard W.M. Jones wrote:
> On Thu, Mar 26, 2020 at 04:35:29PM -0500, Eric Blake wrote:
>> Should the test-public binary still be compiled from direct files
>> under lib/, or should it be linked against libnbdkit.so?
> 
> I will fix this in my copy.
> 
>> I'm wondering if we want to add some sort of
>> 'assert(is_initialized)' to all of our public entry functions
to
>> ensure that no one is actually trying to use libnbdkit.so without
>> having first gone through nbdkit_private_init;[...]
> 
> So firstly I believe this can only be a problem if the server itself
> uses __attribute__((constructor)), since plugins and filters are
> always dlopened long after main() in the server has run.  Is that
> correct?
Right. I'm not worried about plugins ever running at a point where they 
would see libnbdkit.so uninitialized, but more about anyone trying to 
compile a standalone binary with its own main() but linked against 
libnbdkit.so.  Maybe it's just a matter of documentation: we state that 
the library exists solely for linking purposes of a plugin, and not for 
standalone binaries (since the only standalone binary that knows how to 
use it is nbdkit itself).
> 
> The only ((constructor)) currently used is in the OCaml plugin.
> 
> If the server does use a constructor and calls (eg) nbdkit_debug it
> would indeed crash.  However constructors would likely crash in the
> pre-change nbdkit because nothing in the server is initialized
> properly until main() is running.
> 
> Is it worth checking for this still, given it would have an overhead?
You've convinced me - there's no need to add checks, because either they
are pointless overhead when the library is used correctly, or the user 
is abusing the library and deserves to crash because they are using it 
incorrectly.

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