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 | 104 ++++++++++------------ src/qemu/qemu_process.c | 11 +-- src/uml/uml_driver.c | 56 ++++++------- src/xen/xend_internal.c | 12 +-- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tools/virsh.pod | 8 ++- 13 files changed, 155 insertions(+), 130 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 5aebd72..e476c8f 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -109,8 +109,9 @@ <dt><code>disk</code></dt> <dd>This sub-element describes the snapshot properties of a specific disk. The attribute <code>name</code> is - mandatory, and must match the <code><target - dev='name'/></code> of one of + mandatory, and must match either the <code><target + dev='name'/></code> or an unambiguous <code><source + file='name'/></code> of one of the <a href="formatdomain.html#elementsDisks">disk devices</a> specified for the domain at the time of the snapshot. The attribute <code>snapshot</code> is @@ -179,7 +180,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 0af9e0f..e1c6a95 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -770,10 +770,15 @@ </choice> </element> </define> + <define name="diskTarget"> + <data type="string"> + <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> + </data> + </define> <define name="target"> <element name="target"> <attribute name="dev"> - <ref name="deviceName"/> + <ref name="diskTarget"/> </attribute> <optional> <attribute name="bus"> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 671fbe0..0ef0631 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -82,7 +82,10 @@ <define name='disksnapshot'> <element name='disk'> <attribute name='name'> - <ref name='deviceName'/> + <choice> + <ref name='diskTarget'/> + <ref name='absFilePath'/> + </choice> </attribute> <choice> <attribute name='snapshot'> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 043e73f..ec75dbd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5516,17 +5516,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 <target dev='name'/> name (it's shorter, required + * for all disks, and should be unambiguous), but also support + * <source file='name'/> (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, @@ -5602,7 +5629,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); @@ -11205,11 +11232,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, @@ -11246,7 +11274,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) { @@ -11284,6 +11312,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->file, disk->name); goto cleanup; } + if (STRNEQ(disk->file, def->dom->disks[idx]->dst)) { + VIR_FREE(disk->file); + if (!(disk->file = 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 d835f96..c240fe3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1557,7 +1557,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 a953326..5e8f30c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -284,6 +284,7 @@ virDomainDiskInsert; virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; +virDomainDiskPathByName; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskSnapshotTypeFromString; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d6e0c28..fe141bc 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 31d2c7c..111214e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5349,7 +5349,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; @@ -5467,10 +5467,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]; @@ -7209,7 +7209,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); @@ -7231,42 +7232,35 @@ 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; - } + /* Check the path belongs to this domain. */ + if (!(actual = virDomainDiskPathByName(vm->def, path))) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid path '%s'"), path); + goto cleanup; } + path = actual; - 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; - } + /* 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; + } - ret = 0; - } else { - qemuReportError(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, + _("%s: failed to seek or read"), path); + goto cleanup; } + ret = 0; + cleanup: VIR_FORCE_CLOSE(fd); if (vm) @@ -7381,8 +7375,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; - int i; - int format; + int format; /* */ + const char *actual; virCheckFlags(0, -1); @@ -7404,19 +7398,12 @@ 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); @@ -9560,23 +9547,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 27214ad..1accfad 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; - - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDefPtr disk; + int i = virDomainDiskIndexByName(vm->def, path, true); - 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 19b6c55..75c8f27 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; - 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; - } + /* 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; + } - 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 6d5a854..6d62431 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3851,11 +3851,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; @@ -3891,18 +3891,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 1f0beb6..ee6b46a 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -2,7 +2,7 @@ <name>my snap name</name> <description>!@#$%^</description> <disks> - <disk name='hda'/> + <disk name='/dev/HostVG/QEMUGuest1'/> <disk name='hdb' snapshot='no'/> <disk name='hdc' snapshot='internal'/> <disk name='hdd' snapshot='external'> diff --git a/tools/virsh.pod b/tools/virsh.pod index c77b1e4..c37c9e4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -429,7 +429,9 @@ the domain is persistent, these flags are ignored. =item B<domblkstat> I<domain> I<block-device> -Get device block stats for a running domain. +Get device block stats for a running domain. A I<block-device> corresponds +to a unique target name (<target dev='name'/>) or source file (<source +file='name'/>) for one of the disk devices attached to I<domain>. =item B<domifstat> I<domain> I<interface-device> @@ -441,7 +443,9 @@ Get memory stats for a running domain. =item B<domblkinfo> I<domain> I<block-device> -Get block device size info for a domain. +Get block device size info for a domain. A I<block-device> corresponds +to a unique target name (<target dev='name'/>) or source file (<source +file='name'/>) for one of the disk devices attached to I<domain>. =item B<blockpull> I<domain> I<path> [I<bandwidth>] -- 1.7.4.4 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list