Libguestfs - Apr 2020 - Re: [PATCH nbdkit v2 2/3] iso: Implement this plugin using fileops (read-only).

On 4/9/20 3:36 AM, Richard W.M. Jones wrote:
> The plugin should now support pre-fetch and extents, although most ISO
> images will be non-sparse so extents probably isn't that useful.
True, but it can't hurt ;)
> ---
>   plugins/iso/Makefile.am |  4 +-
>   plugins/iso/iso.c       | 99 +++++++++++++++++++----------------------
>   2 files changed, 48 insertions(+), 55 deletions(-)
> 
> @@ -193,25 +195,43 @@ iso_get_ready (void)
>   static void *
>   iso_open (int readonly)
>   {
> -  return NBDKIT_HANDLE_NOT_NEEDED;
> +  struct fileops *fops;
> +  int fd;
> +
> +  fops = malloc (sizeof *fops);
> +  if (fops == NULL) {
> +    nbdkit_error ("malloc: %m");
> +    return NULL;
> +  }
> +
> +  /* Copy the fd because fileops needs to have its own file descriptor. */
> +  fd = dup (iso_fd);
We use system(), but only during .get_ready().  I don't see any places 
where we fork a child after the first .open, so not setting FD_CLOEXEC 
here won't leak into the next client's connection (if we ever add later 
fork()s, we'd have to use fcntl(F_DUPFD_CLOEXEC) instead); but maybe a 
comment to that effect wouldn't hurt (since we already audited which fds 
need CLOEXEC atomically set, seeing the comment will speed up any re-audit).

You are changing from one fd shared among all clients to one fd per 
client, but I don't see that as being a severe problem (the file plugin 
was one fd per client).
>   static struct nbdkit_plugin plugin = {
>     .name              = "iso",
>     .longname          = "nbdkit iso plugin",
> @@ -259,11 +250,11 @@ static struct nbdkit_plugin plugin = {
>     .magic_config_key  = "dir",
>     .get_ready         = iso_get_ready,
>     .open              = iso_open,
> -  .get_size          = iso_get_size,
> +  .close             = iso_close,
>     .can_multi_conn    = iso_can_multi_conn,
Part of me wondered if .can_multi_conn be set for all read-only clients, 
but without making it mandatory on read-write clients.  But I'm fine 
with keeping it as something the plugin must do itself.

Looks good.

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

On 4/9/20 4:27 PM, Eric Blake wrote:
> On 4/9/20 3:36 AM, Richard W.M. Jones wrote:
>> The plugin should now support pre-fetch and extents, although most ISO
>> images will be non-sparse so extents probably isn't that useful.
> 
> True, but it can't hurt ;)
> 
>> ---
>>   plugins/iso/Makefile.am |  4 +-
>>   plugins/iso/iso.c       | 99
+++++++++++++++++++----------------------
>>   2 files changed, 48 insertions(+), 55 deletions(-)
>>
> 
Oops, you forgot to set
#define NBDKIT_API_VERSION 2

Maybe in patch 1 you should add this to fileops.h:
#if NBDKIT_API_VERSION - 0 != 2
# error This header requires API version 2
#endif

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

On 4/9/20 4:31 PM, Eric Blake wrote:
> On 4/9/20 4:27 PM, Eric Blake wrote:
>> On 4/9/20 3:36 AM, Richard W.M. Jones wrote:
>>> The plugin should now support pre-fetch and extents, although most
ISO
>>> images will be non-sparse so extents probably isn't that
useful.
>>
>> True, but it can't hurt ;)
>>
>>> ---
>>>   plugins/iso/Makefile.am |  4 +-
>>>   plugins/iso/iso.c       | 99
+++++++++++++++++++----------------------
>>>   2 files changed, 48 insertions(+), 55 deletions(-)
>>>
>>
> 
> Oops, you forgot to set
> #define NBDKIT_API_VERSION 2
Scratch that, I see you did set it (I was grepping for it but 
pre-patch).  Still, the fact that I missed it means that this next idea 
is still worthwhile.
> 
> Maybe in patch 1 you should add this to fileops.h:
> #if NBDKIT_API_VERSION - 0 != 2
> # error This header requires API version 2
> #endif
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org