Re: [Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export

On 8/27/20 2:57 AM, Richard W.M. Jones wrote: Yes, we have a documentation hole, because that's not the use case I had in mind. Looking at patch 8 for how the exportname filter uses it may make more sense. My intent is that 'nbdkit_add_default_export(exports)' behaves as short-hand for 'nbdkit_add_export(exports, default_export(), NULL)' but with less boilerplate and more convenience (it better handles when default_export() returns NULL, and lets nbdkit cache things so a later call to default_export can reuse the cache). The intended usage is in language bindings, which can do: glue_default_export (readonly, is_tls) { if (real_default_export) return real_default_export(readonly, is_tls) else return ""; } glue_list_exports (readonly, is_tls, exports) { if (real_list_exports) return real_list_exports (readonly, is_tls, exports); else return nbdkit_add_default_export (exports); } rather than this (with glue_default_export unchanged): glue_list_exports (readonly, is_tls, exports) { if (real_list_exports) return real_list_exports (readonly, is_tls, exports); else { // unsafe: if glue_default_export() gives NULL, .list_exports fails // return nbdkit_add_export (exports, glue_default_export(readonly, // is_tls), NULL); const char *def = glue_default_export(readonly, is_tls); if (def) return nbdkit_add_export (exports, def, NULL); return 0; } } Awkward. As I designed things, there are four different client paths for triggering our calls: client calls NBD_OPT_GO("a") - neither .list_exports nor .default_export is needed client calls NBD_OPT_GO("") - .list_exports is not needed, but .default_export is client calls NBD_OPT_LIST, then NBD_OPT_GO("") - both .list_exports and .default_export are needed; and we prefer to cache things so that the plugin's .default_export is called exactly once (if glue_list_exports calls glue_default_export, then we've lost that caching effect; but letting glue_list_exports use nbdkit_add_default_export() solves it) client calls NBD_OPT_LIST, then NBD_OPT_GO("a") - .list_exports is needed. Whether .default_export is called or not depends on the plugin; if the plugin is happy with listing all things explicitly then .default_export can be skipped; if the plugin omits .list_exports then calling .default_export provides a saner list than [""] My worry is that .list_exports has some overhead - it can be expensive to collect a list of exports and store them in memory on every connection, particularly when many clients do NOT care about the list. Yes, we could state that the nbdkit driver _always_ calls .list_exports after .preconnect for each new client, at which point we don't need a separate .default_export (but instead have a way during .list_exports to mark _which_ export to use as default, if any). But it will be more common to have a client that needs .default_export than a client that needs .list_exports, so allowing .default_export to be easy and fast, and declaring that plugins cannot rely on .list_exports being called, is worthwhile. So I definitely need to respin the doc portion of the patch. In my usage, I never mixed add_export and add_default_export in the same control path (it was one or the other), but I also didn't see a problem with a plugin that wants to include its default name in addition to other names. Would bike-shedding the name help, maybe nbdkit_use_default_export, to make it obvious that this is instructing nbdkit to reuse .default_export? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org