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.
> + cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper",
> + path,
> + opname,
> + offsetstr,
> + lengthstr,
> + NULL);
Rather than computing offsetstr and lengthstr into temporary strings
yourself, why not:
cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper",
path,
opname,
NULL);
virCommandAddArgFormat(cmd, "%llu", offset);
virCommandAddArgFormat(cmd, "%llu", length);
> +
> + 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).
> + } else {
> +#if 0
> + if ((flags & O_CREAT) && length != 0) {
> + if (ftruncate(fd, length) < 0) {
This almost sounds like you want to honor O_TRUNC as one of the
supported flags, rather than twisting O_CREAT. It's in an #if 0; what
were your thoughts in coding this if we aren't going to use it?
> +++ b/src/qemu/qemu_driver.c
> @@ -4775,6 +4775,12 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
> goto cleanup;
> }
>
> + if (!virDomainObjIsActive (vm)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
This hunk should be in a separate patch.
> @@ -4788,6 +4794,24 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
> goto cleanup;
> }
>
> + if (!virDomainObjIsActive (vm)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
> +
> + if (!virDomainObjIsActive (vm)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
> +
> + if (!virDomainObjIsActive (vm)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
Yikes! Rebase problems?
> +static int runIO(const char *path,
> + int flags,
> + int mode,
> + int otherfd,
> + const char *otherfdname,
> + unsigned long long offset,
> + unsigned long long length)
> +{
> + char *buf = NULL;
> + size_t buflen = 1024*1024;
> + int fd;
> + int ret = -1;
> + int fdin, fdout;
> + const char *fdinname, *fdoutname;
> +
> + if (flags & O_CREAT) {
> + fd = open(path, flags, mode);
> + } else {
> + fd = open(path, flags);
> + }
Same comment about not strictly needing the if.
> + 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?
> + } else if (STREQ(op, "create")) {
> + flags = O_WRONLY|O_CREAT;
> + mode = 0644;
Hard-coded mode?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list