Re: [Libguestfs] [PATCH nbdkit v2 1/3] file: Move file operators to a new common/fileops mini-library.

On 4/9/20 3:36 AM, Richard W.M. Jones wrote: Agreed. Unnecessary. Since all of our .h files will be included by another .c file that has <config.h> as its first include, we don't need .h files to try the include a second time (even if it is idempotent). I'll clean up the existing places where we don't need it, as a separate cleanup patch. Even if your plugin serves the concatenation of several fds as if one single image, fileops may still prove useful; in those situations, you'll have to write your own callbacks that then call into specific fileops helper functions, rather than being able to use the convenience macro that directly sets callbacks to fileops functions. But that can be a later patch; for now, this wording is fine. This is the step that gets dropped when using fileops for concatenated fds. Mention that you must not access fd after this call succeeds. Also, decide if that also applies to error situations (does this function guarantee to call close(fd) even when returning -1, or does it only take over fd on success?). Someday, I hope to add .can_fast_zero support; but for now, this matches what the file plugin can do. Well, don't call them directly if you wrapped a single fd and used the macros above. But for multi-fd clients, calling these after deciding which fd to forward to makes sense. Or maybe we rename these slightly, and the versions we export for multi-fd plugins are type-safe with struct fileops* instead of void* for the first parameter. This does not close fd on failure, but decide if that's the semantics you want when tweaking the documentation above. Do you want to make sure this is safe to call even if init_fileops() failed? If so, I'd recommend that init_fileops set 'fops->fd = -1' before returning failure, and that this check for -1 before trying close. This matches what file already did, but should we cache the file size we read during init_fileops, rather than having to do a second fstat()? Yeah, reading ahead to patch 3, I can see parameterizing things (perhaps an additional bool parameter to init_fileops) to completely skip flushing, in cases where we know that flushing is pointless due to the temporary nature of the plugin. Hmm - this declaration is conditional, and the macros only install the callback when the function is defined. But is that safe enough, or should we always compile the entry point even the macros do not install it as a callback (where the #if moves inside the function body, with a sane fallback for the case where we don't support SEEK_HOLE). And similar. Why the addition? Uninitialized memory... ...good. init_fileops() initialized the memory on success, and we don't leak the memory on failure. Bad - we leak fd if init_fileops() fails (where you fix it, there or here, determines what you document for the contract). The fileops docs probably ought to mention that .can_multi_conn is the responsibility of the plugin, as there is no sane answer that fileops can generalize. Overall, I like how this is heading. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org