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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list