From 89b6284fd94ce5b13ee6b002f9167f5d9074aa7a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 19 Aug 2011 20:38:36 -0600 Subject: [PATCH] snapshot: also support disks by path I got confused when 'virsh domblkinfo dom disk' required the path to a disk (which can be ambiguous, since a single file can back multiple disks), rather than the unambiguous target device name that I was using in disk snapshots. So, in true developer fashion, I went for the best of both worlds - all interfaces that operate on a disk (aka block) now accept either the target name or the unambiguous path to the backing file used by the disk. * src/conf/domain_conf.h (virDomainDiskIndexByName): Add parameter. (virDomainDiskPathByName): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainDiskIndexByName): Also allow searching by path, and decide whether ambiguity is okay. (virDomainDiskPathByName): New function. (virDomainDiskRemoveByName, virDomainSnapshotAlignDisks): Update callers. * src/qemu/qemu_driver.c (qemudDomainBlockPeek) (qemuDomainAttachDeviceConfig, qemuDomainUpdateDeviceConfig) (qemuDomainGetBlockInfo, qemuDiskPathToAlias): Likewise. * src/qemu/qemu_process.c (qemuProcessFindDomainDiskByPath): Likewise. * src/libxl/libxl_driver.c (libxlDomainAttachDeviceDiskLive) (libxlDomainDetachDeviceDiskLive, libxlDomainAttachDeviceConfig) (libxlDomainUpdateDeviceConfig): Likewise. * src/uml/uml_driver.c (umlDomainBlockPeek): Likewise. * src/xen/xend_internal.c (xenDaemonDomainBlockPeek): Likewise. * docs/formatsnapshot.html.in: Update documentation. * tools/virsh.pod (domblkstat, domblkinfo): Likewise. * docs/schemas/domaincommon.rng (diskTarget): Tighten pattern on disk targets. * docs/schemas/domainsnapshot.rng (disksnapshot): Update to match. * tests/domainsnapshotxml2xmlin/disk_snapshot.xml: Update test. --- docs/formatsnapshot.html.in | 7 +- docs/schemas/domaincommon.rng | 7 +- docs/schemas/domainsnapshot.rng | 5 +- src/conf/domain_conf.c | 57 +++++++-- src/conf/domain_conf.h | 4 +- src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 11 +- src/qemu/qemu_driver.c | 112 ++++++++---------- src/qemu/qemu_process.c | 11 +- src/uml/uml_driver.c | 58 ++++----- src/xen/xend_internal.c | 12 +- .../domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tools/virsh.pod | 8 +- 13 files changed, 160 insertions(+), 135 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 5c6b28202e..ec5ebf3451 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -129,8 +129,9 @@
disk
This sub-element describes the snapshot properties of a specific disk. The attribute name is - mandatory, and must match the <target - dev='name'/> of one of + mandatory, and must match either the <target + dev='name'/> or an unambiguous <source + file='name'/> of one of the disk devices specified for the domain at the time of the snapshot. The attribute snapshot is @@ -203,7 +204,7 @@ <domainsnapshot> <description>Snapshot of OS install and updates</description> <disks> - <disk name='vda'> + <disk name='/path/to/old'> <source file='/path/to/new'/> </disk> <disk name='vdb' snapshot='no'/> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e622cede6f..e2269d004c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -770,10 +770,15 @@ + + + (ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+ + + - + diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 671fbe0698..0ef0631246 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -82,7 +82,10 @@ - + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1881d3f8c1..77428f5aa7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5811,17 +5811,44 @@ virDomainChrTargetTypeToString(int deviceType, return type; } -int virDomainDiskIndexByName(virDomainDefPtr def, const char *name) +int +virDomainDiskIndexByName(virDomainDefPtr def, const char *name, + bool allow_ambiguous) { virDomainDiskDefPtr vdisk; int i; + int candidate = -1; + /* We prefer the name (it's shorter, required + * for all disks, and should be unambiguous), but also support + * (if unambiguous). Assume dst if there is + * no leading slash, source name otherwise. */ for (i = 0; i < def->ndisks; i++) { vdisk = def->disks[i]; - if (STREQ(vdisk->dst, name)) - return i; + if (*name != '/') { + if (STREQ(vdisk->dst, name)) + return i; + } else if (vdisk->src && + STREQ(vdisk->src, name)) { + if (allow_ambiguous) + return i; + if (candidate >= 0) + return -1; + candidate = i; + } } - return -1; + return candidate; +} + +/* Return the path to a disk image if a string identifies at least one + * disk belonging to the domain (both device strings 'vda' and paths + * '/path/to/file' are converted into '/path/to/file'). */ +const char * +virDomainDiskPathByName(virDomainDefPtr def, const char *name) +{ + int i = virDomainDiskIndexByName(def, name, true); + + return i < 0 ? NULL : def->disks[i]->src; } int virDomainDiskInsert(virDomainDefPtr def, @@ -5897,7 +5924,7 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i) int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) { - int i = virDomainDiskIndexByName(def, name); + int i = virDomainDiskIndexByName(def, name, false); if (i < 0) return -1; virDomainDiskRemove(def, i); @@ -11651,11 +11678,12 @@ disksorter(const void *a, const void *b) /* Align def->disks to def->domain. Sort the list of def->disks, * filling in any missing disks or snapshot state defaults given by - * the domain, with a fallback to a passed in default. Issue an error - * and return -1 if any def->disks[n]->name appears more than once or - * does not map to dom->disks. If require_match, also require that - * existing def->disks snapshot states do not override explicit - * def->dom settings. */ + * the domain, with a fallback to a passed in default. Convert paths + * to disk targets for uniformity. Issue an error and return -1 if + * any def->disks[n]->name appears more than once or does not map to + * dom->disks. If require_match, also require that existing + * def->disks snapshot states do not override explicit def->dom + * settings. */ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, int default_snapshot, @@ -11693,7 +11721,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, /* Double check requested disks. */ for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - int idx = virDomainDiskIndexByName(def->dom, disk->name); + int idx = virDomainDiskIndexByName(def->dom, disk->name, false); int disk_snapshot; if (idx < 0) { @@ -11731,6 +11759,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->file, disk->name); goto cleanup; } + if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) { + VIR_FREE(disk->name); + if (!(disk->name = strdup(def->dom->disks[idx]->dst))) { + virReportOOMError(); + goto cleanup; + } + } } /* Provide defaults for all remaining disks. */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fd4dbced62..bb9d3dbc76 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1648,7 +1648,9 @@ int virDomainVcpuPinAdd(virDomainDefPtr def, int virDomainVcpuPinDel(virDomainDefPtr def, int vcpu); -int virDomainDiskIndexByName(virDomainDefPtr def, const char *name); +int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, + bool allow_ambiguous); +const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk); void virDomainDiskInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c100e7237..0780f2d033 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -287,6 +287,7 @@ virDomainDiskInsert; virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; +virDomainDiskPathByName; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskSnapshotTypeFromString; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 91da438175..4302c8be93 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2929,7 +2929,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv, break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { - if (virDomainDiskIndexByName(vm->def, l_disk->dst) >= 0) { + if (virDomainDiskIndexByName(vm->def, l_disk->dst, true) >= 0) { libxlError(VIR_ERR_OPERATION_FAILED, _("target %s already exists"), l_disk->dst); goto cleanup; @@ -2991,7 +2991,8 @@ libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv, if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { if ((i = virDomainDiskIndexByName(vm->def, - dev->data.disk->dst)) < 0) { + dev->data.disk->dst, + false)) < 0) { libxlError(VIR_ERR_OPERATION_FAILED, _("disk %s not found"), dev->data.disk->dst); goto cleanup; @@ -3061,7 +3062,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) { + if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) { libxlError(VIR_ERR_INVALID_ARG, _("target %s already exists."), disk->dst); return -1; @@ -3172,9 +3173,9 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if ((i = virDomainDiskIndexByName(vmdef, disk->dst)) < 0) { + if ((i = virDomainDiskIndexByName(vmdef, disk->dst, false)) < 0) { libxlError(VIR_ERR_INVALID_ARG, - _("target %s doesn't exists."), disk->dst); + _("target %s doesn't exist."), disk->dst); goto cleanup; } orig = vmdef->disks[i]; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 843a4971da..d1c8659fcb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5495,7 +5495,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) { + if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) { qemuReportError(VIR_ERR_INVALID_ARG, _("target %s already exists."), disk->dst); return -1; @@ -5613,10 +5613,10 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst); + pos = virDomainDiskIndexByName(vmdef, disk->dst, false); if (pos < 0) { qemuReportError(VIR_ERR_INVALID_ARG, - _("target %s doesn't exists."), disk->dst); + _("target %s doesn't exist."), disk->dst); return -1; } orig = vmdef->disks[pos]; @@ -7355,7 +7355,8 @@ qemudDomainBlockPeek (virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - int fd = -1, ret = -1, i; + int fd = -1, ret = -1; + const char *actual; virCheckFlags(0, -1); @@ -7377,41 +7378,34 @@ qemudDomainBlockPeek (virDomainPtr dom, goto cleanup; } - /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - ret = 0; - break; - } - } - - if (ret == 0) { - ret = -1; - /* The path is correct, now try to open it and get its size. */ - fd = open (path, O_RDONLY); - if (fd == -1) { - virReportSystemError(errno, - _("%s: failed to open"), path); - goto cleanup; - } - - /* Seek and read. */ - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || - saferead (fd, buffer, size) == (ssize_t) -1) { - virReportSystemError(errno, - _("%s: failed to seek or read"), path); - goto cleanup; - } - - ret = 0; - } else { + /* Check the path belongs to this domain. */ + if (!(actual = virDomainDiskPathByName(vm->def, path))) { qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("invalid path")); + _("invalid path '%s'"), path); + goto cleanup; } + path = actual; + + /* The path is correct, now try to open it and get its size. */ + fd = open(path, O_RDONLY); + if (fd == -1) { + virReportSystemError(errno, + _("%s: failed to open"), path); + goto cleanup; + } + + /* Seek and read. */ + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + if (lseek(fd, offset, SEEK_SET) == (off_t) -1 || + saferead(fd, buffer, size) == (ssize_t) -1) { + virReportSystemError(errno, + _("%s: failed to seek or read"), path); + goto cleanup; + } + + ret = 0; cleanup: VIR_FORCE_CLOSE(fd); @@ -7491,7 +7485,7 @@ qemudDomainMemoryPeek (virDomainPtr dom, qemuDomainObjExitMonitor(driver, vm); /* Read the memory file into buffer. */ - if (saferead (fd, buffer, size) == (ssize_t) -1) { + if (saferead(fd, buffer, size) == (ssize_t) -1) { virReportSystemError(errno, _("failed to read temporary file " "created with template %s"), tmp); @@ -7527,8 +7521,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; - int i; int format; + const char *actual; virCheckFlags(0, -1); @@ -7550,22 +7544,15 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - disk = vm->def->disks[i]; - break; - } - } - - if (!disk) { + if (!(actual = virDomainDiskPathByName(vm->def, path))) { qemuReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto cleanup; } + path = actual; /* The path is correct, now try to open it and get its size. */ - fd = open (path, O_RDONLY); + fd = open(path, O_RDONLY); if (fd == -1) { virReportSystemError(errno, _("failed to open path '%s'"), path); @@ -7624,7 +7611,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should * be 64 bits on all platforms. */ - end = lseek (fd, 0, SEEK_END); + end = lseek(fd, 0, SEEK_END); if (end == (off_t)-1) { virReportSystemError(errno, _("failed to seek to end of %s"), path); @@ -9831,23 +9818,26 @@ static const char * qemuDiskPathToAlias(virDomainObjPtr vm, const char *path) { int i; char *ret = NULL; + virDomainDiskDefPtr disk; - for (i = 0 ; i < vm->def->ndisks ; i++) { - virDomainDiskDefPtr disk = vm->def->disks[i]; + i = virDomainDiskIndexByName(vm->def, path, true); + if (i < 0) + goto cleanup; - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->type != VIR_DOMAIN_DISK_TYPE_FILE) - continue; + disk = vm->def->disks[i]; - if (disk->src != NULL && STREQ(disk->src, path)) { - if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) { - virReportOOMError(); - return NULL; - } - break; + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + disk->type != VIR_DOMAIN_DISK_TYPE_FILE) + goto cleanup; + + if (disk->src) { + if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) { + virReportOOMError(); + return NULL; } } +cleanup: if (!ret) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("No device found for specified path")); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c22974fcc6..f36efeaf95 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -192,15 +192,10 @@ static virDomainDiskDefPtr qemuProcessFindDomainDiskByPath(virDomainObjPtr vm, const char *path) { - int i; + int i = virDomainDiskIndexByName(vm->def, path, true); - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDefPtr disk; - - disk = vm->def->disks[i]; - if (disk->src != NULL && STREQ(disk->src, path)) - return disk; - } + if (i >= 0) + return vm->def->disks[i]; qemuReportError(VIR_ERR_INTERNAL_ERROR, _("no disk found with path %s"), diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 19b6c55859..32f09d2e94 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2175,7 +2175,8 @@ umlDomainBlockPeek(virDomainPtr dom, { struct uml_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - int fd = -1, ret = -1, i; + int fd = -1, ret = -1; + const char *actual; virCheckFlags(0, -1); @@ -2196,41 +2197,34 @@ umlDomainBlockPeek(virDomainPtr dom, } /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - ret = 0; - break; - } + if (!(actual = virDomainDiskPathByName(vm->def, path))) { + umlReportError(VIR_ERR_INVALID_ARG, + _("invalid path '%s'"), path); + goto cleanup; + } + path = actual; + + /* The path is correct, now try to open it and get its size. */ + fd = open(path, O_RDONLY); + if (fd == -1) { + virReportSystemError(errno, + _("cannot open %s"), path); + goto cleanup; } - if (ret == 0) { - ret = -1; - /* The path is correct, now try to open it and get its size. */ - fd = open (path, O_RDONLY); - if (fd == -1) { - virReportSystemError(errno, - _("cannot open %s"), path); - goto cleanup; - } - - /* Seek and read. */ - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || - saferead (fd, buffer, size) == (ssize_t) -1) { - virReportSystemError(errno, - _("cannot read %s"), path); - goto cleanup; - } - - ret = 0; - } else { - umlReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid path")); + /* Seek and read. */ + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + if (lseek(fd, offset, SEEK_SET) == (off_t) -1 || + saferead(fd, buffer, size) == (ssize_t) -1) { + virReportSystemError(errno, + _("cannot read %s"), path); + goto cleanup; } + ret = 0; + cleanup: VIR_FORCE_CLOSE(fd); if (vm) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 0a7fb4806e..8e2170122c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3849,11 +3849,11 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, xenUnifiedPrivatePtr priv; struct sexpr *root = NULL; int fd = -1, ret = -1; - int found = 0, i; virDomainDefPtr def; int id; char * tty; int vncport; + const char *actual; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3889,18 +3889,12 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, vncport))) goto cleanup; - for (i = 0 ; i < def->ndisks ; i++) { - if (def->disks[i]->src && - STREQ(def->disks[i]->src, path)) { - found = 1; - break; - } - } - if (!found) { + if (!(actual = virDomainDiskPathByName(def, path))) { virXendError(VIR_ERR_INVALID_ARG, _("%s: invalid path"), path); goto cleanup; } + path = actual; /* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index 1f0beb6b06..ee6b46a863 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -2,7 +2,7 @@ my snap name !@#$%^ - + diff --git a/tools/virsh.pod b/tools/virsh.pod index 8edf30efdd..f92444af3c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -503,7 +503,9 @@ snapshot metadata with B. =item B I I -Get device block stats for a running domain. +Get device block stats for a running domain. A I corresponds +to a unique target name () or source file () for one of the disk devices attached to I. =item B I I @@ -515,7 +517,9 @@ Get memory stats for a running domain. =item B I I -Get block device size info for a domain. +Get block device size info for a domain. A I corresponds +to a unique target name () or source file () for one of the disk devices attached to I. =item B I I [I]