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

[Eric Blake]

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