On Tue, Mar 15, 2011 at 09:53:40AM -0600, Eric Blake wrote:
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.
Empirically it worked fine, but I've reordered it now.
- 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.
Yes, I think it is nice to be explicit here.
+ 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.
I switched to virCommandAddArgFormat, since I also now have two more integer args to pass
+ + 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.
I've killed this. Instead of passing an 'opname' to the command helper, I just pass the raw int value of 'flags' and 'mode' as parameters.
+ + 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).
We've not bothered with CLOEXEC anywhere else in our code really, since we loop over all FDs and explicitly close them.
+ 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.
I removed this unused code - it was from an earlier idea that didn't play out.
+ 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?
It isn't intended for end users to ever run this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|