Re: [Libguestfs] [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