[libvirt] [PATCH 0/5] REPOST: Allow data upload/download to/from storage volumes
This is a repost of http://www.redhat.com/archives/libvir-list/2011-February/msg00967.html No changes since the last posting, except for re-basing to latest GIT
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 --- src/Makefile.am | 12 +++ src/fdstream.c | 240 ++++++++++++++++++++++++++++++++++++----------- src/fdstream.h | 5 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 26 +++++- src/uml/uml_driver.c | 2 +- src/util/iohelper.c | 216 +++++++++++++++++++++++++++++++++++++++++++ src/xen/xen_driver.c | 2 +- 8 files changed, 445 insertions(+), 60 deletions(-) create mode 100644 src/util/iohelper.c diff --git a/src/Makefile.am b/src/Makefile.am index 645119e..59cf252 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -376,6 +376,9 @@ STORAGE_DRIVER_DISK_SOURCES = \ STORAGE_HELPER_DISK_SOURCES = \ storage/parthelper.c +UTIL_IO_HELPER_SOURCES = \ + util/iohelper.c + # Network filters NWFILTER_DRIVER_SOURCES = \ nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c \ @@ -1179,6 +1182,15 @@ EXTRA_DIST += $(LIBVIRT_QEMU_SYMBOL_FILE) libexec_PROGRAMS = +libexec_PROGRAMS += libvirt_iohelper +libvirt_iohelper_SOURCES = $(UTIL_IO_HELPER_SOURCES) +libvirt_iohelper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) +libvirt_iohelper_LDADD = \ + libvirt_util.la \ + ../gnulib/lib/libgnu.la + +libvirt_iohelper_CFLAGS = $(AM_CFLAGS) + if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper diff --git a/src/fdstream.c b/src/fdstream.c index 701fafc..2746c05 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -34,10 +34,12 @@ #include "fdstream.h" #include "virterror_internal.h" #include "datatypes.h" +#include "logging.h" #include "memory.h" #include "event.h" #include "util.h" #include "files.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STREAMS #define streamsReportError(code, ...) \ @@ -47,6 +49,10 @@ /* Tunnelled migration stream support */ struct virFDStreamData { int fd; + int errfd; + virCommandPtr cmd; + unsigned long long offset; + unsigned long long length; int watch; unsigned int cbRemoved; @@ -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) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("I/O helper exited with status %d"), status); + } else { + buf[len-1] = '\0'; + streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", + buf); + } + ret = -1; + } + virCommandFree(fdst->cmd); + } VIR_FREE(fdst); return ret; } @@ -217,6 +242,8 @@ virFDStreamClose(virStreamPtr st) struct virFDStreamData *fdst = st->privateData; int ret; + VIR_DEBUG("st=%p", st); + if (!fdst) return 0; @@ -250,6 +277,18 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) virMutexLock(&fdst->lock); + if (fdst->length) { + if (fdst->length == fdst->offset) { + virReportSystemError(ENOSPC, "%s", + _("cannot write to stream")); + virMutexUnlock(&fdst->lock); + return -1; + } + + if ((fdst->length - fdst->offset) < nbytes) + nbytes = fdst->length - fdst->offset; + } + retry: ret = write(fdst->fd, bytes, nbytes); if (ret < 0) { @@ -262,6 +301,8 @@ retry: virReportSystemError(errno, "%s", _("cannot write to stream")); } + } else if (fdst->length) { + fdst->offset += ret; } virMutexUnlock(&fdst->lock); @@ -288,6 +329,16 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virMutexLock(&fdst->lock); + if (fdst->length) { + if (fdst->length == fdst->offset) { + virMutexUnlock(&fdst->lock); + return 0; + } + + if ((fdst->length - fdst->offset) < nbytes) + nbytes = fdst->length - fdst->offset; + } + retry: ret = read(fdst->fd, bytes, nbytes); if (ret < 0) { @@ -300,6 +351,8 @@ retry: virReportSystemError(errno, "%s", _("cannot read from stream")); } + } else if (fdst->length) { + fdst->offset += ret; } virMutexUnlock(&fdst->lock); @@ -317,11 +370,17 @@ static virStreamDriver virFDStreamDrv = { .streamRemoveCallback = virFDStreamRemoveCallback }; -int virFDStreamOpen(virStreamPtr st, - int fd) +static int virFDStreamOpenInternal(virStreamPtr st, + int fd, + virCommandPtr cmd, + int errfd, + unsigned long long length) { struct virFDStreamData *fdst; + VIR_DEBUG("st=%p fd=%d cmd=%p errfd=%d length=%llu", + st, fd, cmd, errfd, length); + if ((st->flags & VIR_STREAM_NONBLOCK) && virSetNonBlock(fd) < 0) return -1; @@ -332,6 +391,9 @@ int virFDStreamOpen(virStreamPtr st, } fdst->fd = fd; + fdst->cmd = cmd; + fdst->errfd = errfd; + fdst->length = length; if (virMutexInit(&fdst->lock) < 0) { VIR_FREE(fdst); streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -346,6 +408,13 @@ int virFDStreamOpen(virStreamPtr st, } +int virFDStreamOpen(virStreamPtr st, + int fd) +{ + return virFDStreamOpenInternal(st, fd, NULL, -1, 0); +} + + #if HAVE_SYS_UN_H int virFDStreamConnectUNIX(virStreamPtr st, const char *path, @@ -387,7 +456,7 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); - if (virFDStreamOpen(st, fd) < 0) + if (virFDStreamOpenInternal(st, fd, NULL, -1, 0) < 0) goto error; return 0; @@ -406,19 +475,28 @@ int virFDStreamConnectUNIX(virStreamPtr st ATTRIBUTE_UNUSED, } #endif -int virFDStreamOpenFile(virStreamPtr st, - const char *path, - int flags) +static int +virFDStreamOpenFileInternal(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags, + int mode) { - int fd; + int fd = -1; + int fds[2] = { -1, -1 }; struct stat sb; + virCommandPtr cmd = NULL; + int errfd = -1; - if (flags & O_CREAT) { - streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected O_CREAT flag when opening existing file")); - } + VIR_DEBUG("st=%p path=%s flags=%d offset=%llu length=%llu mode=%d", + st, path, flags, offset, length, mode); - if ((fd = open(path, flags)) < 0) { + if (flags & O_CREAT) + fd = open(path, flags, mode); + else + fd = open(path, flags); + if (fd < 0) { virReportSystemError(errno, _("Unable to open stream for '%s'"), path); @@ -440,64 +518,114 @@ int virFDStreamOpenFile(virStreamPtr st, if ((st->flags & VIR_STREAM_NONBLOCK) && (!S_ISCHR(sb.st_mode) && !S_ISFIFO(sb.st_mode))) { - streamsReportError(VIR_ERR_INTERNAL_ERROR, - _("Non-blocking I/O is not supported on %s"), - path); - goto error; + const char *opname; + int childfd; + char offsetstr[100]; + char lengthstr[100]; + + snprintf(offsetstr, sizeof(offsetstr), "%llu", offset); + snprintf(lengthstr, sizeof(lengthstr), "%llu", length); + + 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"), + path, flags); + goto error; + } + + VIR_FORCE_CLOSE(fd); + if (pipe(fds) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create pipe")); + goto error; + } + + cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", + path, + opname, + offsetstr, + lengthstr, + NULL); + + 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); + } + virCommandSetErrorFD(cmd, &errfd); + + if (virCommandRunAsync(cmd, NULL) < 0) + goto error; + + VIR_FORCE_CLOSE(childfd); + } else { +#if 0 + if ((flags & O_CREAT) && length != 0) { + if (ftruncate(fd, length) < 0) { + virReportSystemError(errno, + _("Unable to truncate %s to %llu"), + path, length); + goto error; + } + } +#endif + if (offset && + lseek(fd, offset, SEEK_SET) != offset) { + virReportSystemError(errno, + _("Unable to seek %s to %llu"), + path, offset); + goto error; + } } - if (virFDStreamOpen(st, fd) < 0) + if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) goto error; return 0; error: + virCommandFree(cmd); + VIR_FORCE_CLOSE(fds[0]); + VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); return -1; } -int virFDStreamCreateFile(virStreamPtr st, - const char *path, - int flags, - mode_t mode) +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags) { - int fd = open(path, flags, mode); - struct stat sb; - - if (fd < 0) { - virReportSystemError(errno, - _("Unable to open stream for '%s'"), - path); - return -1; - } - - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("Unable to access stream for '%s'"), - path); - goto error; - } - - /* Thanks to the POSIX i/o model, we can't reliably get - * non-blocking I/O on block devs/regular files. To - * support those we need to fork a helper process todo - * the I/O so we just have a fifo. Or use AIO :-( - */ - if ((st->flags & VIR_STREAM_NONBLOCK) && - (!S_ISCHR(sb.st_mode) && - !S_ISFIFO(sb.st_mode))) { + if (flags & O_CREAT) { streamsReportError(VIR_ERR_INTERNAL_ERROR, - _("Non-blocking I/O is not supported on %s"), + _("Attempt to create %s without specifying mode"), path); - goto error; + return -1; } + return virFDStreamOpenFileInternal(st, path, offset, length, flags, 0); +} - if (virFDStreamOpen(st, fd) < 0) - goto error; - - return 0; - -error: - VIR_FORCE_CLOSE(fd); - return -1; +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags, + mode_t mode) +{ + return virFDStreamOpenFileInternal(st, path, offset, length, flags, mode); } diff --git a/src/fdstream.h b/src/fdstream.h index 53cbaa7..6b395b6 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -24,6 +24,7 @@ # define __VIR_FDSTREAM_H_ # include "internal.h" +# include "command.h" int virFDStreamOpen(virStreamPtr st, int fd); @@ -34,9 +35,13 @@ int virFDStreamConnectUNIX(virStreamPtr st, int virFDStreamOpenFile(virStreamPtr st, const char *path, + unsigned long long offset, + unsigned long long length, int flags); int virFDStreamCreateFile(virStreamPtr st, const char *path, + unsigned long long offset, + unsigned long long length, int flags, mode_t mode); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 79b6879..6c3e1f8 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2776,7 +2776,7 @@ lxcDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dac2bf2..317d532 100644 --- a/src/qemu/qemu_driver.c +++ 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; + } + if ((*nparams) == 0) { /* Current number of memory parameters supported by cgroups */ *nparams = QEMU_NB_MEM_PARAM; @@ -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; + } + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find cgroup for domain %s"), vm->def->name); @@ -7005,7 +7029,7 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 538d5f7..d364597 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2126,7 +2126,7 @@ umlDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/util/iohelper.c b/src/util/iohelper.c new file mode 100644 index 0000000..0dac055 --- /dev/null +++ b/src/util/iohelper.c @@ -0,0 +1,216 @@ +/* + * iohelper.c: Helper program to perform I/O operations on files + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + * + * Current support + * - Read existing file + * - Write existing file + * - Create & write new file + */ + +#include <config.h> + +#include <locale.h> +#include <unistd.h> +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> + +#include "util.h" +#include "threads.h" +#include "files.h" +#include "memory.h" +#include "virterror_internal.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +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); + } + if (fd < 0) { + virReportSystemError(errno, _("Unable to open %s"), path); + goto cleanup; + } + +#if 0 + if (length && (flags & O_CREAT)) { + if (ftruncate(fd, length) < 0) { + virReportSystemError(errno, _("Unable to truncate %s to %llu"), + path, length); + goto cleanup; + } + } +#endif + + if (offset) { + if (lseek(fd, offset, SEEK_SET) < 0) { + virReportSystemError(errno, _("Unable to seek %s to %llu"), + path, offset); + goto cleanup; + } + } + + if (VIR_ALLOC_N(buf, buflen) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((flags & O_RDONLY) == O_RDONLY) { + fdin = fd; + fdinname = path; + fdout = otherfd; + fdoutname = otherfdname; + } else { + fdin = otherfd; + fdinname = otherfdname; + fdout = fd; + fdoutname = path; + } + + offset = 0; + while (1) { + ssize_t got; + + if (length && + (length - offset) < buflen) + buflen = length - offset; + + if (buflen == 0) + break; + + if ((got = saferead(fdin, buf, buflen)) < 0) { + virReportSystemError(errno, _("Unable to read %s"), fdinname); + goto cleanup; + } + if (got == 0) + break; + offset += got; + if (safewrite(fdout, buf, got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), fdoutname); + goto cleanup; + } + } + + ret = 0; + +cleanup: + if (VIR_CLOSE(fd) < 0 && + ret == 0) { + virReportSystemError(errno, _("Unable to close %s"), path); + ret = -1; + } + + VIR_FREE(buf); + return ret; +} + +int main(int argc, char **argv) +{ + const char *path; + const char *op; + virErrorPtr err; + unsigned long long offset; + unsigned long long length; + int flags; + int mode; + int otherfd; + const char *otherfdname; + + if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0 || + virRandomInitialize(time(NULL) ^ getpid())) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (argc != 5) { + fprintf(stderr, _("%s: syntax FILENAME OPERATION OFFSET LENGTH\n"), argv[0]); + exit(EXIT_FAILURE); + } + + path = argv[1]; + op = argv[2]; + + if (virStrToLong_ull(argv[3], NULL, 10, &offset) < 0) { + fprintf(stderr, _("%s: malformed file offset %s"), argv[0], argv[3]); + exit(EXIT_FAILURE); + } + if (virStrToLong_ull(argv[4], NULL, 10, &length) < 0) { + fprintf(stderr, _("%s: malformed file length %s"), argv[0], argv[4]); + exit(EXIT_FAILURE); + } + + if (STREQ(op, "read")) { + flags = O_RDONLY; + otherfd = STDOUT_FILENO; + otherfdname = "stdout"; + } else if (STREQ(op, "write")) { + flags = O_WRONLY; + otherfd = STDIN_FILENO; + otherfdname = "stdin"; + } else if (STREQ(op, "create")) { + flags = O_WRONLY|O_CREAT; + mode = 0644; + otherfd = STDIN_FILENO; + otherfdname = "stdin"; + } else { + fprintf(stderr, _("%s: unknown file operation %s\n"), argv[0], op); + exit(EXIT_FAILURE); + } + + if (runIO(path, flags, mode, otherfd, otherfdname, offset, length) < 0) + goto error; + + return 0; + +error: + err = virGetLastError(); + if (err) { + fprintf(stderr, "%s: %s\n", argv[0], err->message); + } else { + fprintf(stderr, _("%s: unknown failure with %s\n"), argv[0], path); + } + exit(EXIT_FAILURE); +} diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 7d08df3..d54fe02 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1994,7 +1994,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; -- 1.7.4
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :|
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
+ if (offset && + lseek(fd, offset, SEEK_SET) != offset) { + virReportSystemError(errno, + _("Unable to seek %s to %llu"), + path, offset); + goto error; + }
One more thing to think about - since unsigned long long is unsigned, but off_t used by lseek is signed, do we need to worry about overflow or other out-of-bounds problems? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation * include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload, virStorageVolCreateUpload APIs * src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub code for new APIs * src/storage/storage_driver.c, src/esx/esx_storage_driver.c: Add dummy entries in driver table for new APIs --- include/libvirt/libvirt.h.in | 10 ++++ include/libvirt/virterror.h | 1 + src/driver.h | 14 +++++ src/esx/esx_storage_driver.c | 2 + src/libvirt.c | 120 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/storage/storage_driver.c | 2 + src/util/virterror.c | 6 ++ 8 files changed, 157 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eaeccd6..17ed09d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1499,6 +1499,16 @@ virStorageVolPtr virStorageVolCreateXMLFrom (virStoragePoolPtr pool, const char *xmldesc, virStorageVolPtr clonevol, unsigned int flags); +int virStorageVolDownload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +int virStorageVolUpload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); int virStorageVolDelete (virStorageVolPtr vol, unsigned int flags); int virStorageVolWipe (virStorageVolPtr vol, diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 6b8c789..64bdcec 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -229,6 +229,7 @@ typedef enum { VIR_ERR_HOOK_SCRIPT_FAILED = 70, /* a synchronous hook script failed */ VIR_ERR_INVALID_DOMAIN_SNAPSHOT = 71,/* invalid domain snapshot */ VIR_ERR_NO_DOMAIN_SNAPSHOT = 72, /* domain snapshot not found */ + VIR_ERR_INVALID_STREAM = 73 /* stream pointer not valid */ } virErrorNumber; /** diff --git a/src/driver.h b/src/driver.h index 6a83325..0ab23c0 100644 --- a/src/driver.h +++ b/src/driver.h @@ -899,6 +899,18 @@ typedef virStorageVolPtr const char *xmldesc, virStorageVolPtr clone, unsigned int flags); +typedef int + (*virDrvStorageVolDownload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +typedef int + (*virDrvStorageVolUpload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); typedef int (*virDrvStoragePoolIsActive)(virStoragePoolPtr pool); @@ -953,6 +965,8 @@ struct _virStorageDriver { virDrvStorageVolLookupByPath volLookupByPath; virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; + virDrvStorageVolDownload volDownload; + virDrvStorageVolUpload volUpload; virDrvStorageVolDelete volDelete; virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index 136a90b..9e4dd9e 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -1671,6 +1671,8 @@ static virStorageDriver esxStorageDriver = { esxStorageVolumeLookupByPath, /* volLookupByPath */ esxStorageVolumeCreateXML, /* volCreateXML */ esxStorageVolumeCreateXMLFrom, /* volCreateXMLFrom */ + NULL, /* volDownload */ + NULL, /* volUpload */ esxStorageVolumeDelete, /* volDelete */ esxStorageVolumeWipe, /* volWipe */ esxStorageVolumeGetInfo, /* volGetInfo */ diff --git a/src/libvirt.c b/src/libvirt.c index 713291f..77b9074 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9059,6 +9059,126 @@ error: /** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from + * @length: limit on amount of data to download + * @flags: flags for creation (unused, pass 0) + * + * Download the content of the volume as a stream. + * + * Returns 0, or -1 upon error. + */ +int +virStorageVolDownload(virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + return -1; + } + + if (!VIR_IS_STREAM(stream)) { + virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__); + return -1; + } + + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (vol->conn->storageDriver && + vol->conn->storageDriver->volDownload) { + int ret; + ret = vol->conn->storageDriver->volDownload(vol, + stream, + offset, + length, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} + + +/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start writing to + * @length: limit on amount of data to upload + * @flags: flags for creation (unused, pass 0) + * + * Upload new content to the volume from a stream. + * + * Returns 0, or -1 upon error. + */ +int +virStorageVolUpload(virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + return -1; + } + + if (!VIR_IS_STREAM(stream)) { + virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__); + return -1; + } + + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (vol->conn->storageDriver && + vol->conn->storageDriver->volUpload) { + int ret; + ret = vol->conn->storageDriver->volUpload(vol, + stream, + offset, + length, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} + + +/** * virStorageVolDelete: * @vol: pointer to storage volume * @flags: future flags, use 0 for now diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index c027bf7..dfe0196 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -431,6 +431,8 @@ LIBVIRT_0.9.0 { virDomainSetMemoryFlags; virEventRegisterDefaultImpl; virEventRunDefaultImpl; + virStorageVolDownload; + virStorageVolUpload; } LIBVIRT_0.8.8; # .... define new API here using predicted next version number .... diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5373025..ce528cf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1989,6 +1989,8 @@ static virStorageDriver storageDriver = { .volLookupByPath = storageVolumeLookupByPath, .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, + .volDownload = NULL, + .volUpload = NULL, .volDelete = storageVolumeDelete, .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, diff --git a/src/util/virterror.c b/src/util/virterror.c index f136054..dc00957 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1198,6 +1198,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Domain snapshot not found: %s"); break; + case VIR_ERR_INVALID_STREAM: + if (info == NULL) + errmsg = _("invalid stream pointer in"); + else + errmsg = _("invalid stream pointer in %s"); + break; } return (errmsg); } -- 1.7.4
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation
* include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload, virStorageVolCreateUpload APIs
I see virStorageVol{Up,Down}load, but not virStorageVolCreateUpload in this patch.
* src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub code for new APIs * src/storage/storage_driver.c, src/esx/esx_storage_driver.c: Add dummy entries in driver table for new APIs --- include/libvirt/libvirt.h.in | 10 ++++ include/libvirt/virterror.h | 1 + src/driver.h | 14 +++++ src/esx/esx_storage_driver.c | 2 + src/libvirt.c | 120 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/storage/storage_driver.c | 2 + src/util/virterror.c | 6 ++ 8 files changed, 157 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eaeccd6..17ed09d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1499,6 +1499,16 @@ virStorageVolPtr virStorageVolCreateXMLFrom (virStoragePoolPtr pool, const char *xmldesc, virStorageVolPtr clonevol, unsigned int flags); +int virStorageVolDownload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +int virStorageVolUpload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); int virStorageVolDelete (virStorageVolPtr vol,
Are you missing a function declaration?
+++ b/src/driver.h @@ -899,6 +899,18 @@ typedef virStorageVolPtr const char *xmldesc, virStorageVolPtr clone, unsigned int flags); +typedef int + (*virDrvStorageVolDownload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +typedef int + (*virDrvStorageVolUpload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags);
Makes sense that the drivers only need two callbacks, even if you add three public functions, since the CreateUpload can piece together multiple driver callbacks.
+++ b/src/libvirt.c @@ -9059,6 +9059,126 @@ error:
/** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from + * @length: limit on amount of data to download
Does 0 (or UINT64_MAX) have a special meaning of read-to-end? If so, document that.
+ + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + }
Why is reading a volume prohibited on a RO connection?
+/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start writing to + * @length: limit on amount of data to upload
Again, is there any special meaning of length 0, or a directive to easily specify to end of input?
+ * @flags: flags for creation (unused, pass 0)
Do we need a flag to allow for expansion of the volume if the input stream (and length) are larger than the current size of the volume, for volumes that can be expanded in size?
+ + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + }
Agree here that RO connection should not modify volumes.
+++ b/src/util/virterror.c @@ -1198,6 +1198,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Domain snapshot not found: %s"); break; + case VIR_ERR_INVALID_STREAM: + if (info == NULL) + errmsg = _("invalid stream pointer in");
s/ in// -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 15, 2011 at 10:37:01AM -0600, Eric Blake wrote:
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation
* include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload, virStorageVolCreateUpload APIs
I see virStorageVol{Up,Down}load, but not virStorageVolCreateUpload in this patch.
As per previous posting, I removed VolCreateUpload since it isn't needed and is complex to support.
+++ b/src/libvirt.c @@ -9059,6 +9059,126 @@ error:
/** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from + * @length: limit on amount of data to download
Does 0 (or UINT64_MAX) have a special meaning of read-to-end? If so, document that.
I've added docs about '0'
+ + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + }
Why is reading a volume prohibited on a RO connection?
The data inside a storage volume may be security sensitive. Thus we don't want readonly users to be able to read guest disks.
+/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start writing to + * @length: limit on amount of data to upload
Again, is there any special meaning of length 0, or a directive to easily specify to end of input?
Added docs here.
+ * @flags: flags for creation (unused, pass 0)
Do we need a flag to allow for expansion of the volume if the input stream (and length) are larger than the current size of the volume, for volumes that can be expanded in size?
I prefer to keep this side-effect free. We should have a separate API for enlarging storage, particularly since everything except plain raw files cannot do grow-on-demand. 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 :|
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands --- .x-sc_avoid_write | 1 + tools/virsh.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 0 deletions(-) diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index f6fc1b2..0784984 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -5,5 +5,6 @@ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ +^tools/virsh\.c$ ^gnulib/ ^tools/console.c$ diff --git a/tools/virsh.c b/tools/virsh.c index 42ebd55..9c4eac8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -263,6 +263,9 @@ static int vshCommandOptString(const vshCmd *cmd, const char *name, static int vshCommandOptLongLong(const vshCmd *cmd, const char *name, long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +static int vshCommandOptULongLong(const vshCmd *cmd, const char *name, + unsigned long long *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; static int vshCommandOptBool(const vshCmd *cmd, const char *name); static char *vshCommandOptArgv(const vshCmd *cmd, int count); @@ -7078,6 +7081,204 @@ cleanup: return ret; } + +static int +cmdVolUploadSource(virStreamPtr st ATTRIBUTE_UNUSED, + char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return read(*fd, bytes, nbytes); +} + + +/* + * "vol-upload" command + */ +static const vshCmdInfo info_vol_upload[] = { + {"help", gettext_noop("upload a file into a volume")}, + {"desc", gettext_noop("Upload a file into a volume")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_upload[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")}, + {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, + {"length", VSH_OT_INT, 0, N_("amount of data to upload") }, + {NULL, 0, 0, NULL} +}; + +/* + */ +static int +cmdVolUpload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset, length; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0; + + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_RDONLY)) < 0) { + vshError(ctl, "cannot read %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolUpload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot upload to volume %s", name); + goto cleanup; + } + + if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) { + vshError(ctl, "cannot send data to volume %s", name); + goto cleanup; + } + + if (close(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + fd = -1; + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (st) + virStreamFree(st); + if (fd != -1) + close(fd); + return ret; +} + + + +/* + * "vol-download" command + */ +static const vshCmdInfo info_vol_download[] = { + {"help", gettext_noop("Download a volume to a file")}, + {"desc", gettext_noop("Download a volume to a file")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_download[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")}, + {"offset", VSH_OT_INT, 0, N_("volume offset to download from") }, + {"length", VSH_OT_INT, 0, N_("amount of data to download") }, + {NULL, 0, 0, NULL} +}; + + +static int +cmdVolDownloadSink(virStreamPtr st ATTRIBUTE_UNUSED, + const char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return write(*fd, bytes, nbytes); +} + +/* + */ +static int +cmdVolDownload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset, length; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0; + + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) { + vshError(ctl, "cannot create %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolDownload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot download from volume %s", name); + goto cleanup; + } + + if (virStreamRecvAll(st, cmdVolDownloadSink, &fd) < 0) { + vshError(ctl, "cannot receive data from volume %s", name); + goto cleanup; + } + + if (close(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + fd = -1; + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (ret == FALSE) + unlink(file); + if (vol) + virStorageVolFree(vol); + if (st) + virStreamFree(st); + if (fd != -1) + close(fd); + return ret; +} + + /* * "vol-delete" command */ @@ -9897,6 +10098,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) return ret; } + /* * "net-edit" command */ @@ -10534,6 +10736,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-create", cmdVolCreate, opts_vol_create, info_vol_create}, {"vol-create-from", cmdVolCreateFrom, opts_vol_create_from, info_vol_create_from}, {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete}, + {"vol-download", cmdVolDownload, opts_vol_download, info_vol_download }, {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-key", cmdVolKey, opts_vol_key, info_vol_key}, @@ -10541,6 +10744,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-name", cmdVolName, opts_vol_name, info_vol_name}, {"vol-path", cmdVolPath, opts_vol_path, info_vol_path}, {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool}, + {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload }, {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe}, {NULL, NULL, NULL, NULL} }; @@ -11031,6 +11235,27 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, return ret; } +static int +vshCommandOptULongLong(const vshCmd *cmd, const char *name, + unsigned long long *value) +{ + vshCmdOpt *arg = vshCommandOpt(cmd, name); + int ret = 0; + unsigned long long num; + char *end_p = NULL; + + if ((arg != NULL) && (arg->data != NULL)) { + num = strtoull(arg->data, &end_p, 10); + ret = -1; + if ((arg->data != end_p) && (*end_p == 0)) { + *value = num; + ret = 1; + } + } + return ret; +} + + /* * Returns TRUE/FALSE if the option exists */ -- 1.7.4
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands --- .x-sc_avoid_write | 1 + tools/virsh.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 0 deletions(-)
diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index f6fc1b2..0784984 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -5,5 +5,6 @@ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ +^tools/virsh\.c$
Jim Meyering has a pending upstream patch for gnulib that will get rid of the .x-sc* files in favor of cfg.mk; if that goes in first, it will affect this patch. Do we really need to add an unprotected write() to virsh.c? That's such a big file to exempt, and it would be nicer if we could just exempt a helper file that does the single usage while keeping the overall virsh.c file protected.
^gnulib/ ^tools/console.c$ diff --git a/tools/virsh.c b/tools/virsh.c index 42ebd55..9c4eac8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -263,6 +263,9 @@ static int vshCommandOptString(const vshCmd *cmd, const char *name, static int vshCommandOptLongLong(const vshCmd *cmd, const char *name, long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +static int vshCommandOptULongLong(const vshCmd *cmd, const char *name, + unsigned long long *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
Is it worth adding the helper API in a separate patch, to make it easier to cherry pick just vshCommandOptULongLong and some followup patch that uses it independently of vol-upload?
+/* + * "vol-upload" command + */ +static const vshCmdInfo info_vol_upload[] = { + {"help", gettext_noop("upload a file into a volume")},
I thought we had a syntax-check rule that required you to use N_ instead of gettext_noop here. If we don't, that's probably worth adding, to enforce consistency with the rest of the file.
+ {"desc", gettext_noop("Upload a file into a volume")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_upload[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")},
The above are required,
+ {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, + {"length", VSH_OT_INT, 0, N_("amount of data to upload") },
But these two should be optional (offset of 0, length of entire file [and that in turn depends on answering my question in patch 2/5 about whether 0 for length behaves special, or whether we need some other sentinel like -1]).
+ unsigned long long offset, length; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0;
Not quite the right usage pattern. If the result is < 0, then that was a syntax error (such as --offset=foo) and should be diagnosed. Meanwhile, if the result is 0, then this uses offset uninitialized.
+ if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) { + vshError(ctl, "cannot send data to volume %s", name); + goto cleanup; + } + + if (close(fd) < 0) {
s/close/VIR_CLOSE/
+ vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + fd = -1;
then you don't need this line.
+ if (fd != -1) + close(fd);
VIR_FORCE_CLOSE - 'make syntax-check' should have caught this
+ return ret; +} + + + +/* + * "vol-download" command + */ +static const vshCmdInfo info_vol_download[] = { + {"help", gettext_noop("Download a volume to a file")}, + {"desc", gettext_noop("Download a volume to a file")},
N_()
+ {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_download[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")}, + {"offset", VSH_OT_INT, 0, N_("volume offset to download from") }, + {"length", VSH_OT_INT, 0, N_("amount of data to download") }, + {NULL, 0, 0, NULL} +}; + + +static int +cmdVolDownloadSink(virStreamPtr st ATTRIBUTE_UNUSED, + const char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return write(*fd, bytes, nbytes); +}
Consistency - for upload, you put the helper before the vshCmdInfo/vshCmdOptDef declarations, but for download, you put it in between the declarations and their associated function. I like the style used for upload.
+ + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0;
Same usage problems as for upload.
+ + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0; + + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) {
Should --mode be allowed as an optional argument?
+ + if (close(fd) < 0) {
VIR_CLOSE
+ /* * "net-edit" command */ @@ -10534,6 +10736,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-create", cmdVolCreate, opts_vol_create, info_vol_create}, {"vol-create-from", cmdVolCreateFrom, opts_vol_create_from, info_vol_create_from}, {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete}, + {"vol-download", cmdVolDownload, opts_vol_download, info_vol_download }, {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-key", cmdVolKey, opts_vol_key, info_vol_key}, @@ -10541,6 +10744,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-name", cmdVolName, opts_vol_name, info_vol_name}, {"vol-path", cmdVolPath, opts_vol_path, info_vol_path}, {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool}, + {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload }, {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe}, {NULL, NULL, NULL, NULL}
No vol-create-upload command? Is the commit message out-of-date, or is this patch incomplete? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 15, 2011 at 11:07:53AM -0600, Eric Blake wrote:
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands --- .x-sc_avoid_write | 1 + tools/virsh.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 0 deletions(-)
diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index f6fc1b2..0784984 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -5,5 +5,6 @@ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ +^tools/virsh\.c$
Jim Meyering has a pending upstream patch for gnulib that will get rid of the .x-sc* files in favor of cfg.mk; if that goes in first, it will affect this patch.
Do we really need to add an unprotected write() to virsh.c? That's such a big file to exempt, and it would be nicer if we could just exempt a helper file that does the single usage while keeping the overall virsh.c file protected.
Yes, I can in fact change it to saferead/safewrite. This was a leftover from earlier code
+/* + * "vol-upload" command + */ +static const vshCmdInfo info_vol_upload[] = { + {"help", gettext_noop("upload a file into a volume")},
I thought we had a syntax-check rule that required you to use N_ instead of gettext_noop here. If we don't, that's probably worth adding, to enforce consistency with the rest of the file.
Opps, this was a really old code I forgot.
+ {"desc", gettext_noop("Upload a file into a volume")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_upload[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")},
The above are required,
+ {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, + {"length", VSH_OT_INT, 0, N_("amount of data to upload") },
But these two should be optional (offset of 0, length of entire file [and that in turn depends on answering my question in patch 2/5 about whether 0 for length behaves special, or whether we need some other sentinel like -1]).
Err, they are optional already - '0' instead of VSH_OFLAG_REQ.
+ unsigned long long offset, length; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0;
Not quite the right usage pattern. If the result is < 0, then that was a syntax error (such as --offset=foo) and should be diagnosed. Meanwhile, if the result is 0, then this uses offset uninitialized.
Yep, I see this now. 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 :|
* src/storage/storage_driver.c: Wire up upload/download APIs --- src/storage/storage_driver.c | 133 +++++++++++++++++++++++++++++++++++++++++- 1 files changed, 131 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ce528cf..706db74 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -46,6 +46,7 @@ #include "storage_backend.h" #include "logging.h" #include "files.h" +#include "fdstream.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1528,6 +1529,134 @@ cleanup: } +static int +storageVolumeDownload(virStorageVolPtr obj, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, + "%s", _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("storage pool is not active")); + goto out; + } + + vol = virStorageVolDefFindByName(pool, obj->name); + + if (vol == NULL) { + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); + goto out; + } + + if (vol->building) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("volume '%s' is still being allocated."), + vol->name); + goto out; + } + + if (virFDStreamOpenFile(stream, + vol->target.path, + offset, length, + O_RDONLY) < 0) + goto out; + + ret = 0; + +out: + if (pool) + virStoragePoolObjUnlock(pool); + + return ret; +} + + +static int +storageVolumeUpload(virStorageVolPtr obj, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, + "%s", _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("storage pool is not active")); + goto out; + } + + vol = virStorageVolDefFindByName(pool, obj->name); + + if (vol == NULL) { + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); + goto out; + } + + if (vol->building) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("volume '%s' is still being allocated."), + vol->name); + goto out; + } + + + /* Not using O_CREAT because the file is required to + * already exist at this point */ + if (virFDStreamOpenFile(stream, + vol->target.path, + offset, length, + O_WRONLY) < 0) + goto out; + + ret = 0; + +out: + if (pool) + virStoragePoolObjUnlock(pool); + + return ret; +} + + + /* If the volume we're wiping is already a sparse file, we simply * truncate and extend it to its original size, filling it with * zeroes. This behavior is guaranteed by POSIX: @@ -1989,8 +2118,8 @@ static virStorageDriver storageDriver = { .volLookupByPath = storageVolumeLookupByPath, .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, - .volDownload = NULL, - .volUpload = NULL, + .volDownload = storageVolumeDownload, + .volUpload = storageVolumeUpload, .volDelete = storageVolumeDelete, .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, -- 1.7.4
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* src/storage/storage_driver.c: Wire up upload/download APIs --- src/storage/storage_driver.c | 133 +++++++++++++++++++++++++++++++++++++++++- 1 files changed, 131 insertions(+), 2 deletions(-)
ACK - looks pretty straightforward. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* daemon/remote.c, src/remote/remote_driver.c: Implementation of storage vol upload/download APIs * src/remote/remote_protocol.x: Wire protocol definition for upload/download * daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h, daemon/remote_dispatch_table.h, src/remote/remote_protocol.h, src/remote/remote_protocol.c: Re-generate --- daemon/remote.c | 92 +++++++++++++++++++++++++++++++++++ daemon/remote_dispatch_args.h | 2 + daemon/remote_dispatch_prototypes.h | 16 ++++++ daemon/remote_dispatch_table.h | 10 ++++ src/remote/remote_driver.c | 87 +++++++++++++++++++++++++++++++++ src/remote/remote_protocol.c | 30 +++++++++++ src/remote/remote_protocol.h | 22 ++++++++ src/remote/remote_protocol.x | 19 +++++++- 8 files changed, 277 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f410982..7e8103c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5816,7 +5816,99 @@ remoteDispatchNodeDeviceDestroy(struct qemud_server *server ATTRIBUTE_UNUSED, virNodeDeviceFree(dev); return 0; } +static int remoteDispatchStorageVolUpload(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *rerr, + remote_storage_vol_upload_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int rv = -1; + struct qemud_client_stream *stream = NULL; + virStorageVolPtr vol; + + vol = get_nonnull_storage_vol(conn, args->vol); + if (vol == NULL) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + stream = remoteCreateClientStream(conn, hdr); + if (!stream) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (virStorageVolUpload(vol, stream->st, + args->offset, args->length, + args->flags) < 0) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (remoteAddClientStream(client, stream, 0) < 0) { + remoteDispatchConnError(rerr, conn); + virStreamAbort(stream->st); + goto cleanup; + } + + rv = 0; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (stream && rv != 0) + remoteFreeClientStream(client, stream); + return rv; +} +static int remoteDispatchStorageVolDownload(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *rerr, + remote_storage_vol_download_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int rv = -1; + struct qemud_client_stream *stream = NULL; + virStorageVolPtr vol; + + vol = get_nonnull_storage_vol (conn, args->vol); + if (vol == NULL) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + stream = remoteCreateClientStream(conn, hdr); + if (!stream) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (virStorageVolDownload(vol, stream->st, + args->offset, args->length, + args->flags) < 0) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (remoteAddClientStream(client, stream, 1) < 0) { + remoteDispatchConnError(rerr, conn); + virStreamAbort(stream->st); + goto cleanup; + } + + rv = 0; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (stream && rv != 0) + remoteFreeClientStream(client, stream); + return rv; +} /*************************** diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index b32ae1f..da378c1 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -175,3 +175,5 @@ remote_domain_set_memory_flags_args val_remote_domain_set_memory_flags_args; remote_domain_set_blkio_parameters_args val_remote_domain_set_blkio_parameters_args; remote_domain_get_blkio_parameters_args val_remote_domain_get_blkio_parameters_args; + remote_storage_vol_upload_args val_remote_storage_vol_upload_args; + remote_storage_vol_download_args val_remote_storage_vol_download_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index 4db6c76..65f7f8e 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -1530,6 +1530,14 @@ static int remoteDispatchStorageVolDelete( remote_error *err, remote_storage_vol_delete_args *args, void *ret); +static int remoteDispatchStorageVolDownload( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_download_args *args, + void *ret); static int remoteDispatchStorageVolDumpXml( struct qemud_server *server, struct qemud_client *client, @@ -1578,6 +1586,14 @@ static int remoteDispatchStorageVolLookupByPath( remote_error *err, remote_storage_vol_lookup_by_path_args *args, remote_storage_vol_lookup_by_path_ret *ret); +static int remoteDispatchStorageVolUpload( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_upload_args *args, + void *ret); static int remoteDispatchStorageVolWipe( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index c50d038..06b083e 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -1037,3 +1037,13 @@ .args_filter = (xdrproc_t) xdr_remote_domain_get_blkio_parameters_args, .ret_filter = (xdrproc_t) xdr_remote_domain_get_blkio_parameters_ret, }, +{ /* StorageVolUpload => 207 */ + .fn = (dispatch_fn) remoteDispatchStorageVolUpload, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_upload_args, + .ret_filter = (xdrproc_t) xdr_void, +}, +{ /* StorageVolDownload => 208 */ + .fn = (dispatch_fn) remoteDispatchStorageVolDownload, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_download_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 82e094b..4f6ed3c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9560,6 +9560,91 @@ done: return rv; } +static int +remoteStorageVolUpload(virStorageVolPtr vol, + virStreamPtr st, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + struct private_data *priv = vol->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_storage_vol_upload_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1, + REMOTE_PROC_STORAGE_VOL_UPLOAD, + priv->counter))) + goto done; + + st->driver = &remoteStreamDrv; + st->privateData = privst; + + make_nonnull_storage_vol(&args.vol, vol); + args.offset = offset; + args.length = length; + args.flags = flags; + + if (call (vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_UPLOAD, + (xdrproc_t) xdr_remote_storage_vol_upload_args, (char *) &args, + (xdrproc_t) xdr_void, NULL) == -1) { + remoteStreamRelease(st); + goto done; + } + + rv = 0; + +done: + remoteDriverUnlock(priv); + + return rv; +} + + +static int +remoteStorageVolDownload(virStorageVolPtr vol, + virStreamPtr st, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + struct private_data *priv = vol->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_storage_vol_download_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD, + priv->counter))) + goto done; + + st->driver = &remoteStreamDrv; + st->privateData = privst; + + make_nonnull_storage_vol(&args.vol, vol); + args.offset = offset; + args.length = length; + args.flags = flags; + + if (call (vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_DOWNLOAD, + (xdrproc_t) xdr_remote_storage_vol_download_args, (char *) &args, + (xdrproc_t) xdr_void, NULL) == -1) { + remoteStreamRelease(st); + goto done; + } + + rv = 0; + +done: + remoteDriverUnlock(priv); + + return rv; +} + static int remoteDomainOpenConsole(virDomainPtr dom, @@ -11228,6 +11313,8 @@ static virStorageDriver storage_driver = { .volLookupByPath = remoteStorageVolLookupByPath, .volCreateXML = remoteStorageVolCreateXML, .volCreateXMLFrom = remoteStorageVolCreateXMLFrom, + .volDownload = remoteStorageVolDownload, + .volUpload = remoteStorageVolUpload, .volDelete = remoteStorageVolDelete, .volWipe = remoteStorageVolWipe, .volGetInfo = remoteStorageVolGetInfo, diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index ea2bdf7..9cadcb7 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3859,6 +3859,36 @@ xdr_remote_domain_open_console_args (XDR *xdrs, remote_domain_open_console_args } bool_t +xdr_remote_storage_vol_upload_args (XDR *xdrs, remote_storage_vol_upload_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->offset)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->length)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_storage_vol_download_args (XDR *xdrs, remote_storage_vol_download_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->offset)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->length)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_procedure (XDR *xdrs, remote_procedure *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index a55f7c4..4bf2b0f 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -2177,6 +2177,22 @@ struct remote_domain_open_console_args { u_int flags; }; typedef struct remote_domain_open_console_args remote_domain_open_console_args; + +struct remote_storage_vol_upload_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; +typedef struct remote_storage_vol_upload_args remote_storage_vol_upload_args; + +struct remote_storage_vol_download_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; +typedef struct remote_storage_vol_download_args remote_storage_vol_download_args; #define REMOTE_PROGRAM 0x20008086 #define REMOTE_PROTOCOL_VERSION 1 @@ -2387,6 +2403,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_MEMORY_FLAGS = 204, REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 205, REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, + REMOTE_PROC_STORAGE_VOL_UPLOAD = 207, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 208, }; typedef enum remote_procedure remote_procedure; @@ -2767,6 +2785,8 @@ extern bool_t xdr_remote_domain_snapshot_current_ret (XDR *, remote_domain_snap extern bool_t xdr_remote_domain_revert_to_snapshot_args (XDR *, remote_domain_revert_to_snapshot_args*); extern bool_t xdr_remote_domain_snapshot_delete_args (XDR *, remote_domain_snapshot_delete_args*); extern bool_t xdr_remote_domain_open_console_args (XDR *, remote_domain_open_console_args*); +extern bool_t xdr_remote_storage_vol_upload_args (XDR *, remote_storage_vol_upload_args*); +extern bool_t xdr_remote_storage_vol_download_args (XDR *, remote_storage_vol_download_args*); extern bool_t xdr_remote_procedure (XDR *, remote_procedure*); extern bool_t xdr_remote_message_type (XDR *, remote_message_type*); extern bool_t xdr_remote_message_status (XDR *, remote_message_status*); @@ -3121,6 +3141,8 @@ extern bool_t xdr_remote_domain_snapshot_current_ret (); extern bool_t xdr_remote_domain_revert_to_snapshot_args (); extern bool_t xdr_remote_domain_snapshot_delete_args (); extern bool_t xdr_remote_domain_open_console_args (); +extern bool_t xdr_remote_storage_vol_upload_args (); +extern bool_t xdr_remote_storage_vol_download_args (); extern bool_t xdr_remote_procedure (); extern bool_t xdr_remote_message_type (); extern bool_t xdr_remote_message_status (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index aa710a4..4bcad65 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1920,6 +1920,21 @@ struct remote_domain_open_console_args { unsigned int flags; }; +struct remote_storage_vol_upload_args { + remote_nonnull_storage_vol vol; + unsigned hyper offset; + unsigned hyper length; + unsigned int flags; +}; + +struct remote_storage_vol_download_args { + remote_nonnull_storage_vol vol; + unsigned hyper offset; + unsigned hyper length; + unsigned int flags; +}; + + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -2152,7 +2167,9 @@ enum remote_procedure { REMOTE_PROC_GET_SYSINFO = 203, REMOTE_PROC_DOMAIN_SET_MEMORY_FLAGS = 204, REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 205, - REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206 + REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, + REMOTE_PROC_STORAGE_VOL_UPLOAD = 207, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 208 /* * Notice how the entries are grouped in sets of 10 ? -- 1.7.4
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* daemon/remote.c, src/remote/remote_driver.c: Implementation of storage vol upload/download APIs * src/remote/remote_protocol.x: Wire protocol definition for upload/download * daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h, daemon/remote_dispatch_table.h, src/remote/remote_protocol.h, src/remote/remote_protocol.c: Re-generate
Missing changes to src/remote_protocol-structs (make check should warn if you have the dwarves package installed). Which reminds me - this patch is still pending review to make that file avoid TABs: https://www.redhat.com/archives/libvir-list/2011-March/msg00519.html But the rest of the patch looks fine. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 15, 2011 at 01:09:09PM -0600, Eric Blake wrote:
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* daemon/remote.c, src/remote/remote_driver.c: Implementation of storage vol upload/download APIs * src/remote/remote_protocol.x: Wire protocol definition for upload/download * daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h, daemon/remote_dispatch_table.h, src/remote/remote_protocol.h, src/remote/remote_protocol.c: Re-generate
Missing changes to src/remote_protocol-structs (make check should warn if you have the dwarves package installed). Which reminds me - this patch is still pending review to make that file avoid TABs: https://www.redhat.com/archives/libvir-list/2011-March/msg00519.html
Yes, I had this chunk much later in my patch series (in unrelated code). 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 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake