On 3/25/20 7:16 AM, Richard W.M. Jones wrote:> I think I understand now that libnbdkit.so won't break the ABI for > existing plugins. Does it require that plugins for newer nbdkit use > -lnbdkit (which would be a source API break) or would it still be > possible to compile without this? I guess as long as plugins do not > start using -no-undefined then it would still work, so it wouldn't be > a source API break.Yes, that's what I'm thinking - it's an API break if you have to add in -lnbdkit; but you only have to add that if you want to use -no-undefined. Plugins compiled without -lnbdkit (either because they predate the library, or because they specifically did not care about -no-undefined) should still be viable.> > I had a look into how we might implement libnbdkit.so. Some functions > are obviously self-contained (eg. nbdkit_parse_*, nbdkit_realpath, > nbdkit_debug, nbdkit_error, nbdkit_*extents). > > Unfortunately some functions depend themselves on internals > of the server: > > * nbdkit_nanosleep, nbdkit_export_name, nbdkit_peer_name call > threadlocal_get_conn > * nbdkit_set_error calls threadlocal_set_error > * nbdkit_shutdown must set the quit global (or call a server function)Yeah, there's some awkward dependencies to figure out. It's obvious the library has to export public nbdkit_* interfaces for the sake of plugins, but can it also export one additional symbol _nbdkit_init() for internal use? Then we can have the nbdkit binary pass whatever additional hooks are needed for proper isolation of the public interface (a callback pointer to get at threadlocal_get_conn, threadlocal_set_error, the address of the quit global, ...), and leave the symbol undocumented (plus the fact that the leading _ will discourage plugins from trying to abuse it).> > I guess we can deal with the first ones by moving threadlocal.c into > the same library, although it's a bit awkward. The quit flag is still > more awkward because you have to move a lot of quit pipe handling code > into the library which has knock-on effects all over.The other extreme is to have the entire nbdkit engine in libnbdkit.so, plus the addition of a single internal-only callback _nbdkit_main(), then the nbdkit binary becomes a bare-bones: int main (int argc, char *argv[]) { return _nbdkit_main (argc, argv); } at which point you don't have to decide which functionality lives where. After all, you already have a linker script that limits what the main binary exports; which really becomes what libnbdkit.so has to export. If we are worried about plugins trying to abuse that additional entry point, you can have _nbdkit_main set a flag on first execution so that subsequent attempts to call it fail immediately; but I'm not too worried about it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Mar 25, 2020 at 07:46:37AM -0500, Eric Blake wrote:> On 3/25/20 7:16 AM, Richard W.M. Jones wrote: > >I think I understand now that libnbdkit.so won't break the ABI for > >existing plugins. Does it require that plugins for newer nbdkit use > >-lnbdkit (which would be a source API break) or would it still be > >possible to compile without this? I guess as long as plugins do not > >start using -no-undefined then it would still work, so it wouldn't be > >a source API break. > > Yes, that's what I'm thinking - it's an API break if you have to add > in -lnbdkit; but you only have to add that if you want to use > -no-undefined. Plugins compiled without -lnbdkit (either because > they predate the library, or because they specifically did not care > about -no-undefined) should still be viable. > > > > >I had a look into how we might implement libnbdkit.so. Some functions > >are obviously self-contained (eg. nbdkit_parse_*, nbdkit_realpath, > >nbdkit_debug, nbdkit_error, nbdkit_*extents). > > > >Unfortunately some functions depend themselves on internals > >of the server: > > > > * nbdkit_nanosleep, nbdkit_export_name, nbdkit_peer_name call > > threadlocal_get_conn > > * nbdkit_set_error calls threadlocal_set_error > > * nbdkit_shutdown must set the quit global (or call a server function) > > Yeah, there's some awkward dependencies to figure out. It's obvious > the library has to export public nbdkit_* interfaces for the sake of > plugins, but can it also export one additional symbol _nbdkit_init() > for internal use? Then we can have the nbdkit binary pass whatever > additional hooks are needed for proper isolation of the public > interface (a callback pointer to get at threadlocal_get_conn, > threadlocal_set_error, the address of the quit global, ...), and > leave the symbol undocumented (plus the fact that the leading _ will > discourage plugins from trying to abuse it).Yes this is a good point. Also I suppose this interface between nbdkit <-> libnbdkit.so is *not* ABI so we can change this at will? It would mean that nbdkit & libnbdkit.so must always be shipped together, but that doesn't seem to be a problem. Rich.> > > >I guess we can deal with the first ones by moving threadlocal.c into > >the same library, although it's a bit awkward. The quit flag is still > >more awkward because you have to move a lot of quit pipe handling code > >into the library which has knock-on effects all over. > > The other extreme is to have the entire nbdkit engine in > libnbdkit.so, plus the addition of a single internal-only callback > _nbdkit_main(), then the nbdkit binary becomes a bare-bones: > int main (int argc, char *argv[]) { > return _nbdkit_main (argc, argv); > } > > at which point you don't have to decide which functionality lives > where. After all, you already have a linker script that limits what > the main binary exports; which really becomes what libnbdkit.so has > to export. If we are worried about plugins trying to abuse that > additional entry point, you can have _nbdkit_main set a flag on > first execution so that subsequent attempts to call it fail > immediately; but I'm not too worried about it. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 3/25/20 8:01 AM, Richard W.M. Jones wrote:>>> Unfortunately some functions depend themselves on internals >>> of the server: >>> >>> * nbdkit_nanosleep, nbdkit_export_name, nbdkit_peer_name call >>> threadlocal_get_conn >>> * nbdkit_set_error calls threadlocal_set_error >>> * nbdkit_shutdown must set the quit global (or call a server function) >> >> Yeah, there's some awkward dependencies to figure out. It's obvious >> the library has to export public nbdkit_* interfaces for the sake of >> plugins, but can it also export one additional symbol _nbdkit_init() >> for internal use? Then we can have the nbdkit binary pass whatever >> additional hooks are needed for proper isolation of the public >> interface (a callback pointer to get at threadlocal_get_conn, >> threadlocal_set_error, the address of the quit global, ...), and >> leave the symbol undocumented (plus the fact that the leading _ will >> discourage plugins from trying to abuse it). > > Yes this is a good point. > > Also I suppose this interface between nbdkit <-> libnbdkit.so is > *not* ABI so we can change this at will? It would mean that > nbdkit & libnbdkit.so must always be shipped together, but that > doesn't seem to be a problem.Yes, I can live with that. Similar to how we declare that while plugins are API/ABI-compatible across versions, filters are not, and to run a filter, it must come from the same sources as the nbdkit binary. So I don't see it as a problem to change our undocumented _nbdkit_internal() entry point at will. Of course, it is still wise to make our internal interface robust enough to make it easy to detect if a user has ever accidentally mismatched versions, such as declaring that the first parameter is a version number regardless of how other parameters change over time. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org