Re: [libvirt] [PATCH 1/5] Enhance the streams helper to support plain file I/O

On 03/15/2011 06:30 AM, Daniel P. Berrange wrote: > The O_NONBLOCK flag doesn't work as desired on plain files > or block devices. Introduce an I/O helper program that does > the blocking I/O operations, communicating over a pipe that > can support O_NONBLOCK > > * src/fdstream.c, src/fdstream.h: Add non-blocking I/O > on plain files/block devices > * src/Makefile.am, src/util/iohelper.c: I/O helper program > * src/qemu/qemu_driver.c, src/lxc/lxc_driver.c, > src/uml/uml_driver.c, src/xen/xen_driver.c: Update for > streams API change > @@ -206,6 +212,25 @@ static int virFDStreamFree(struct virFDStreamData *fdst) > { > int ret; > ret = VIR_CLOSE(fdst->fd); > + if (fdst->cmd) { > + int status; > + if (virCommandWait(fdst->cmd, &status) < 0) > + ret = -1; > + if (status != 0) { > + ssize_t len = 1024; > + char buf[len]; > + if ((len = saferead(fdst->errfd, buf, len)) < 0) { Is this safe to read from errfd after you've already waited for the child to exit? After all, exit() is allowed to discard unread data from a pipe, and once the child is exited, the parent may get EOF instead of that data. Conversely, can the child block trying to write into errfd, and thus never reach the exit, so that you have a deadlock where the parent is waiting without reading? I think you have to read errfd prior to calling virCommandWait.

> - if ((fd = open(path, flags)) < 0) { > + if (flags & O_CREAT) > + fd = open(path, flags, mode); > + else > + fd = open(path, flags); Technically, it's safe to call fd = open(path, flags, mode) even when flags does not contain O_CREAT, but I'm fine with keeping this if statement.

> + const char *opname; > + int childfd; > + char offsetstr[100]; > + char lengthstr[100]; > + > + snprintf(offsetstr, sizeof(offsetstr), "%llu", offset); > + snprintf(lengthstr, sizeof(lengthstr), "%llu", length); 100 is too big. Use this for a tighter bound: #include "intprops.h" char offsetstr[INT_BUFSIZE_BOUND(offset)]; But see below for an alternate suggestion that loses these variables altogether.

> + > + if (flags == O_RDONLY) { > + opname = "read"; > + } else if (flags == O_WRONLY) { > + opname = "write"; > + } else if (flags == (O_WRONLY|O_CREAT)) { > + opname = "create"; > + } else { > + streamsReportError(VIR_ERR_INTERNAL_ERROR, > + _("Non-blocking I/O is not supported on %s with flags %d"), Okay, so you insist on a specific subset of flags. But what about flags like O_CLOEXEC? Should you only be comparing against the bitmask of flags that you actually care about, while allowing other flags as needed? Also, should you support O_EXCL? I guess we can add support for more flags at the point where we add more clients that want to use those flags.

> + > + if (!cmd) > + goto error; > + > + //virCommandDaemonize(cmd); > + if (flags == O_RDONLY) { > + childfd = fds[1]; > + fd = fds[0]; > + virCommandSetOutputFD(cmd, &childfd); > + } else { > + childfd = fds[0]; > + fd = fds[1]; > + virCommandSetInputFD(cmd, childfd); Should we also be setting FD_CLOEXEC on the side of the pipe not given to the child? (I really need to find time to work on O_CLOEXEC support in gnulib, then do an audit over all of libvirt to take advantage of atomic CLOEXEC support by using things like pipe2).

> + if (fd < 0) { > + virReportSystemError(errno, _("Unable to open %s"), path); > + goto cleanup; > + } > + > +#if 0 > + if (length && (flags & O_CREAT)) { > + if (ftruncate(fd, length) < 0) { Same comment about O_TRUNC support.

> + if (argc != 5) { > + fprintf(stderr, _("%s: syntax FILENAME OPERATION OFFSET LENGTH\n"), argv[0]); > + exit(EXIT_FAILURE); > + } Do we want --help/--version support (and a man page), or is this program considered internal enough to not need the public-facing documentation?