On 3/24/20 3:12 PM, Eric Blake wrote:>> (For non-mingw platforms) this breaks the source API promises rather >> seriously, so if I understand your proposal correctly I don't think >> this is a good idea. It's possibly something we can consider for >> internal plugins, or for the V3 API. > > How does it break API to request that someone link against a particular > library if they want to avoid undefined symbols (and to continue to > allow the to link with undefined symbols if they choose not to compile > against that library)?I guess what I haven't said yet is that the existing proposed patches from Frank for making mingw compile are NOT the right way to resolve the need for -no-undefined on that platform. Instead of creating lots of wrapper functions buried inside #ifdef WINDOWS_COMPAT, we really should be focusing on creating a clean libnbdkit.so/dll library that exposes all of the needed symbols without the need for preprocessor magic, and therefore without API breaks. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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. 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) 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. Rich. ---------------------------------------------------------------------- server/debug.c:nbdkit_vdebug (const char *fs, va_list args) server/debug.c:nbdkit_debug (const char *fs, ...) server/extents.c:nbdkit_extents_new (uint64_t start, uint64_t end) server/extents.c:nbdkit_extents_free (struct nbdkit_extents *exts) server/extents.c:nbdkit_extents_count (const struct nbdkit_extents *exts) server/extents.c:nbdkit_get_extent (const struct nbdkit_extents *exts, size_t i) server/extents.c:nbdkit_add_extent (struct nbdkit_extents *exts, server/log.c:nbdkit_verror (const char *fs, va_list args) server/log.c:nbdkit_error (const char *fs, ...) server/log.c:nbdkit_vfprintf(FILE *f, const char *fmt, va_list args) server/plugins.c:nbdkit_set_error (int err) server/public.c:nbdkit_absolute_path (const char *path) server/public.c:nbdkit_realpath (const char *path) server/public.c:nbdkit_parse_int (const char *what, const char *str, int *rp) server/public.c:nbdkit_parse_int8_t (const char *what, const char *str, int8_t *rp) server/public.c:nbdkit_parse_int16_t (const char *what, const char *str, int16_t *rp) server/public.c:nbdkit_parse_int32_t (const char *what, const char *str, int32_t *rp) server/public.c:nbdkit_parse_int64_t (const char *what, const char *str, int64_t *rp) server/public.c:nbdkit_parse_unsigned (const char *what, const char *str, unsigned *rp) server/public.c:nbdkit_parse_uint8_t (const char *what, const char *str, uint8_t *rp) server/public.c:nbdkit_parse_uint16_t (const char *what, const char *str, uint16_t *rp) server/public.c:nbdkit_parse_uint32_t (const char *what, const char *str, uint32_t *rp) server/public.c:nbdkit_parse_uint64_t (const char *what, const char *str, uint64_t *rp) server/public.c:nbdkit_parse_size (const char *str) server/public.c:nbdkit_parse_bool (const char *str) server/public.c:nbdkit_read_password (const char *value, char **password) server/public.c:nbdkit_nanosleep (unsigned sec, unsigned nsec) server/public.c:nbdkit_export_name (void) server/public.c:nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen) server/quit.c:nbdkit_shutdown (void) -- 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
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