On Sat, Oct 13, 2012 at 4:59 PM, Eric Blake <eblake@redhat.com> wrote:

When an image has no backing file, using VIR_STORAGE_FILE_AUTO for its type is a bit confusing. Additionally, a future patch would like to reserve a default value for the case of no file type specified in the XML, but different from the current use of -1 to imply probing, since probing is not always safe.

Also, a couple of file types were missing compared to supported code: libxl supports 'vhd', and qemu supports 'fat' for directories passed through as a file system.

* src/util/storage_file.h (virStorageFileFormat): Add VIR_STORAGE_FILE_NONE, VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD. * src/util/storage_file.c (virStorageFileMatchesVersion): Match documentation when version probing not supported. (cowGetBackingStore, qcowXGetBackingStore, qcow1GetBackingStore) (qedGetBackingStore, virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataFromFD): Take NONE into account. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Likewise. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Likewise. * src/conf/storage_conf.c (virStorageVolumeFormatFromString): New function. (poolTypeInfo): Use it. --- src/conf/domain_conf.c | 2 +- src/conf/storage_conf.c | 15 ++++++++--- src/qemu/qemu_driver.c | 2 +- src/util/storage_file.c | 69 +++++++++++++++++++++++++++++++++++-------------- src/util/storage_file.h | 8 ++++-- 5 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c87c615..460a57c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14648,7 +14648,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (STREQ(formatStr, "aio")) formatStr = "raw"; /* Xen compat */

- if ((format = virStorageFileFormatTypeFromString(formatStr)) < 0) { + if ((format = virStorageFileFormatTypeFromString(formatStr)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk format '%s' for %s"), disk->driverType, disk->src); diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4daf059..5d9e61a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -135,6 +135,15 @@ struct _virStoragePoolTypeInfo { virStorageVolOptions volOptions; };

+static int +virStorageVolumeFormatFromString(const char *format) +{ + int ret = virStorageFileFormatTypeFromString(format); + if (ret == VIR_STORAGE_FILE_NONE) + return -1; + return ret; +} + static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_LOGICAL, .poolOptions = { @@ -148,7 +157,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_DIR, .volOptions = { .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageFileFormatTypeFromString, + .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, }, }, @@ -161,7 +170,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { }, .volOptions = { .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageFileFormatTypeFromString, + .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, }, }, @@ -175,7 +184,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { }, .volOptions = { .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageFileFormatTypeFromString, + .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, }, }, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0787039..0e4b2d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9192,7 +9192,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,

/* Probe for magic formats */ if (disk->driverType) { - if ((format = virStorageFileFormatTypeFromString(disk->driverType)) < 0) { + if ((format = virStorageFileFormatTypeFromString(disk->driverType)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk format %s for %s"), disk->driverType, disk->src); diff --git a/src/util/storage_file.c b/src/util/storage_file.c index eea760c..3d1b7cf 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2011 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -45,9 +45,11 @@

VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, + "none", "raw", "dir", "bochs", "cloop", "cow", "dmg", "iso", - "qcow", "qcow2", "qed", "vmdk", "vpc") + "qcow", "qcow2", "qed", "vmdk", "vpc", + "fat", "vhd")

enum lv_endian { LV_LITTLE_ENDIAN = 1, /* 1234 */ @@ -123,8 +125,12 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t);

static struct FileTypeInfo const fileTypeInfo[] = { - [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, - [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, + [VIR_STORAGE_FILE_NONE] = { NULL, NULL, LV_LITTLE_ENDIAN, + -1, 0, 0, 0, 0, 0, NULL }, + [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, + -1, 0, 0, 0, 0, 0, NULL }, + [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, + -1, 0, 0, 0, 0, 0, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ NULL, NULL, @@ -180,6 +186,11 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 12, 0x10000, 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL }, + /* Not direct file formats, but used for various drivers */ + [VIR_STORAGE_FILE_FAT] = { NULL, NULL, LV_LITTLE_ENDIAN, + -1, 0, 0, 0, 0, 0, NULL }, + [VIR_STORAGE_FILE_VHD] = { NULL, NULL, LV_LITTLE_ENDIAN, + -1, 0, 0, 0, 0, 0, NULL }, }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST);

@@ -195,8 +206,10 @@ cowGetBackingStore(char **res,

if (buf_size < 4+4+ COW_FILENAME_MAXLEN) return BACKING_STORE_INVALID; - if (buf[4+4] == '\0') /* cow_header_v2.backing_file[0] */ + if (buf[4+4] == '\0') { /* cow_header_v2.backing_file[0] */ + *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; + }

*res = strndup ((const char*)buf + 4+4, COW_FILENAME_MAXLEN); if (*res == NULL) { @@ -298,8 +311,11 @@ qcowXGetBackingStore(char **res, | (buf[QCOWX_HDR_BACKING_FILE_SIZE+1] << 16) | (buf[QCOWX_HDR_BACKING_FILE_SIZE+2] << 8) | buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* QCowHeader.backing_file_size */ - if (size == 0) + if (size == 0) { + if (format) + *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; + } if (offset + size > buf_size || offset + size < offset) return BACKING_STORE_INVALID; if (size + 1 == 0) @@ -335,8 +351,12 @@ qcowXGetBackingStore(char **res, * between the end of the header (QCOW2_HDR_TOTAL_SIZE) * and the start of the backingStoreName (offset) */ - if (isQCow2 && format) - qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, offset); + if (isQCow2 && format) { + qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, + offset); + if (*format <= VIR_STORAGE_FILE_NONE) + return BACKING_STORE_INVALID; + }

return BACKING_STORE_OK; } @@ -348,10 +368,15 @@ qcow1GetBackingStore(char **res, const unsigned char *buf, size_t buf_size) { + int ret; + /* QCow1 doesn't have the extensions capability * used to store backing format */ *format = VIR_STORAGE_FILE_AUTO; - return qcowXGetBackingStore(res, NULL, buf, buf_size, false); + ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false); + if (ret == 0 && *buf == '\0') + *format = VIR_STORAGE_FILE_NONE; + return ret; }

static int @@ -401,6 +426,7 @@ vmdk4GetBackingStore(char **res, desc[len] = '\0'; start = strstr(desc, prefix); if (start == NULL) { + *format = VIR_STORAGE_FILE_NONE; ret = BACKING_STORE_OK; goto cleanup; } @@ -411,6 +437,7 @@ vmdk4GetBackingStore(char **res, goto cleanup; } if (end == start) { + *format = VIR_STORAGE_FILE_NONE; ret = BACKING_STORE_OK; goto cleanup; } @@ -464,8 +491,10 @@ qedGetBackingStore(char **res, if (buf_size < QED_HDR_FEATURES_OFFSET+8) return BACKING_STORE_INVALID; flags = qedGetHeaderULL(buf + QED_HDR_FEATURES_OFFSET); - if (!(flags & QED_F_BACKING_FILE)) + if (!(flags & QED_F_BACKING_FILE)) { + *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; + }

/* Parse the backing file */ if (buf_size < QED_HDR_BACKING_FILE_OFFSET+8) @@ -485,12 +514,10 @@ qedGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0';

- if (format) { - if (flags & QED_F_BACKING_FORMAT_NO_PROBE) - *format = virStorageFileFormatTypeFromString("raw"); - else - *format = VIR_STORAGE_FILE_AUTO_SAFE; - } + if (flags & QED_F_BACKING_FORMAT_NO_PROBE) + *format = virStorageFileFormatTypeFromString("raw"); + else + *format = VIR_STORAGE_FILE_AUTO_SAFE;

return BACKING_STORE_OK; } @@ -564,7 +591,7 @@ virStorageFileMatchesVersion(int format,

/* Validate version number info */ if (fileTypeInfo[format].versionOffset == -1) - return true; + return false;

if ((fileTypeInfo[format].versionOffset + 4) > buflen) return false; @@ -607,7 +634,9 @@ virStorageFileGetMetadataFromBuf(int format, /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */ - if (!fileTypeInfo[format].magic) { + if (format <= VIR_STORAGE_FILE_NONE || + format >= VIR_STORAGE_FILE_LAST || + !fileTypeInfo[format].magic) { return 0; }

@@ -682,7 +711,7 @@ virStorageFileGetMetadataFromBuf(int format, meta->backingStoreFormat = backingFormat; } else { meta->backingStore = NULL; - meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO; + meta->backingStoreFormat = VIR_STORAGE_FILE_NONE; } }

@@ -867,7 +896,7 @@ virStorageFileGetMetadataFromFD(const char *path, if (format == VIR_STORAGE_FILE_AUTO) format = virStorageFileProbeFormatFromBuf(path, head, len);

- if (format < 0 || + if (format <= VIR_STORAGE_FILE_NONE || format >= VIR_STORAGE_FILE_LAST) { virReportSystemError(EINVAL, _("unknown storage file format %d"), format); diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 2e76b1b..683ec73 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -29,7 +29,8 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_AUTO_SAFE = -2, VIR_STORAGE_FILE_AUTO = -1, - VIR_STORAGE_FILE_RAW = 0, + VIR_STORAGE_FILE_NONE = 0, + VIR_STORAGE_FILE_RAW, VIR_STORAGE_FILE_DIR, VIR_STORAGE_FILE_BOCHS, VIR_STORAGE_FILE_CLOOP, @@ -41,6 +42,9 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_QED, VIR_STORAGE_FILE_VMDK, VIR_STORAGE_FILE_VPC, + VIR_STORAGE_FILE_FAT, + VIR_STORAGE_FILE_VHD, + VIR_STORAGE_FILE_LAST, };

--

Visual ACK. I'll try to apply and build it later. -- Doug Goldstein

On 10/13/2012 04:00 PM, Eric Blake wrote:

Actually use the enum in the domain conf structure.

* src/conf/domain_conf.h (_virDomainDiskDef): Store enum rather than string for disk type. * src/conf/domain_conf.c (virDomainDiskDefFree) (virDomainDiskDefParseXML, virDomainDiskDefFormat) (virDomainDiskDefForeachPath): Adjust users. * src/xenxs/xen_sxpr.c (xenParseSxprDisks, xenFormatSxprDisk): Likewise. * src/xenxs/xen_xm.c (xenParseXM, xenFormatXMDisk): Likewise. * src/vbox/vbox_tmpl.c (vboxAttachDrives): Likewise. * src/libxl/libxl_conf.c (libxlMakeDisk): Likewise. ---

@@ -4158,23 +4152,34 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->wwn = wwn; wwn = NULL;

- if (!def->driverType && - caps->defaultDiskDriverType && - !(def->driverType = strdup(virStorageFileFormatTypeToString( - caps->defaultDiskDriverType)))) - goto no_memory; + if (driverType) { + def->format = virStorageFileFormatTypeFromString(driverType); + if (def->format <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown driver format value '%s'"), + driverType); + goto error; + } + } else { + def->format = caps->defaultDiskDriverType; + }

@@ -5209,11 +5210,10 @@ qemuBuildCommandLine(virConnectPtr conn,

if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) { /* QEMU only supports magic FAT format for now */ - if (disk->driverType && - STRNEQ(disk->driverType, "fat")) { + if (disk->format && disk->format != VIR_STORAGE_FILE_FAT) {

Going by inspection, I need a tweak here - in the old code, if you had a <disk type='dir'> but no <driver type='fat'/>, then disk->driverType was NULL, which we treated as a synonym for auto. But in the new code, disk->format defaults to whatever the driver capabilities state, which in patch 4/16 is either AUTO (-1) or RAW (1), so for the same behavior, this needs to check 'disk->format > 0' to work correctly for AUTO, as well as tweak domain_conf to not apply driver capability defaults to dir or network protocols (formats only make sense on files and block devices). Most of the rest of this patch did it correctly. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Sat, Oct 13, 2012 at 5:00 PM, Eric Blake <eblake@redhat.com> wrote:

This is the last use of raw strings for disk formats throughout the src/conf directory.

* src/conf/snapshot_conf.h (_virDomainSnapshotDiskDef): Store enum rather than string for disk type. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefClear) (virDomainSnapshotDiskDefParseXML, virDomainSnapshotDefFormat): Adjust users. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotCreateSingleDiskActive): Likewise. --- src/conf/snapshot_conf.c | 23 ++++++++++++++++------- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_driver.c | 30 +++++++++++------------------- 3 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 0085352..16c844d 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,7 +82,6 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); - VIR_FREE(disk->driverType); }

void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) @@ -134,15 +133,24 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, if (!def->file && xmlStrEqual(cur->name, BAD_CAST "source")) { def->file = virXMLPropString(cur, "file"); - } else if (!def->driverType && + } else if (!def->format && xmlStrEqual(cur->name, BAD_CAST "driver")) { - def->driverType = virXMLPropString(cur, "type"); + char *driver = virXMLPropString(cur, "type"); + def->format = virStorageFileFormatTypeFromString(driver); + if (def->format <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot driver '%s'"), + driver); + VIR_FREE(driver); + goto cleanup; + } + VIR_FREE(driver); } } cur = cur->next; }

- if (!def->snapshot && (def->file || def->driverType)) + if (!def->snapshot && (def->file || def->format)) def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;

ret = 0; @@ -536,11 +544,12 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, if (disk->snapshot) virBufferAsprintf(&buf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(disk->snapshot)); - if (disk->file || disk->driverType) { + if (disk->file || disk->format > 0) { virBufferAddLit(&buf, ">\n"); - if (disk->driverType) + if (disk->format > 0) virBufferEscapeString(&buf, " <driver type='%s'/>\n", - disk->driverType); + virStorageFileFormatTypeToString( + disk->format)); if (disk->file) virBufferEscapeString(&buf, " <source file='%s'/>\n", disk->file); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index efb0bf7..c00347f 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -52,7 +52,7 @@ struct _virDomainSnapshotDiskDef { int index; /* index within snapshot->dom->disks that matches name */ int snapshot; /* enum virDomainSnapshotLocation */ char *file; /* new source file when snapshot is external */ - char *driverType; /* file format type of new file */ + int format; /* enum virStorageFileFormat */ };

/* Stores the complete snapshot metadata */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 35aab74..4d1c63c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10569,17 +10569,15 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, break;

case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: - if (!disk->driverType) { - if (!(disk->driverType = strdup("qcow2"))) { - virReportOOMError(); - goto cleanup; - } - } else if (STRNEQ(disk->driverType, "qcow2") && - STRNEQ(disk->driverType, "qed")) { + if (!disk->format) { + disk->format = VIR_STORAGE_FILE_QCOW2; + } else if (disk->format != VIR_STORAGE_FILE_QCOW2 && + disk->format != VIR_STORAGE_FILE_QED) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot format for disk %s " "is unsupported: %s"), - disk->name, disk->driverType); + disk->name, + virStorageFileFormatTypeToString(disk->format)); goto cleanup; } if (stat(disk->file, &st) < 0) { @@ -10649,7 +10647,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; char *source = NULL; - int format = VIR_STORAGE_FILE_NONE; + int format = snap->format; + const char *formatStr = NULL; char *persistSource = NULL; int ret = -1; int fd = -1; @@ -10663,15 +10662,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, return -1; }

- if (snap->driverType) { - format = virStorageFileFormatTypeFromString(snap->driverType); - if (format <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown driver type %s"), snap->driverType); - goto cleanup; - } - } - if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || !(source = strdup(snap->file)) || (persistDisk && @@ -10717,8 +10707,10 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, disk->format = origdriver;

/* create the actual snapshot */ + if (snap->format) + formatStr = virStorageFileFormatTypeToString(snap->format); ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source, - snap->driverType, reuse); + formatStr, reuse); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) goto cleanup; --

Visually looks good. ACK. -- Doug Goldstein

On Sat, Oct 13, 2012 at 5:00 PM, Eric Blake <eblake@redhat.com> wrote:

Backing chains can end on a network protocol, such as nbd:xxx; we should not attempt to probe the file system in this case.

* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Only probe files. --- src/storage/storage_backend_fs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1e48a51..db19b87 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -108,7 +108,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, if (meta->backingStore) { *backingStore = meta->backingStore; meta->backingStore = NULL; - if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO) { + if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO && + meta->backingStoreIsFile) { if ((ret = virStorageFileProbeFormat(*backingStore)) < 0) { /* If the backing file is currently unavailable, only log an error, * but continue. Returning -1 here would disable the whole storage --

Makes sense. ACK. -- Doug Goldstein

On 10/13/2012 06:00 PM, Eric Blake wrote:

Previously, no one was using virStorageFileGetMetadata, and for good reason - it couldn't support root-squash NFS. Change the signature and make it useful to future patches, including enhancing the metadata to recursively track the entire chain.

* src/util/storage_file.h (_virStorageFileMetadata): Add field. (virStorageFileGetMetadata): Alter signature. * src/util/storage_file.c (virStorageFileGetMetadata): Rewrite. (virStorageFileGetMetadataRecurse): New function. (virStorageFileFreeMetadata): Handle recursion. --- src/util/storage_file.c | 94 +++++++++++++++++++++++++++++++++++++++---------- src/util/storage_file.h | 18 ++++++---- 2 files changed, 86 insertions(+), 26 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 3d1b7cf..0f879bd 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -40,6 +40,7 @@ #include "logging.h" #include "virfile.h" #include "c-ctype.h" +#include "virhash.h"

#define VIR_FROM_THIS VIR_FROM_STORAGE

@@ -840,7 +841,7 @@ virStorageFileProbeFormat(const char *path) * * Extract metadata about the storage volume with the specified * image format. If image format is VIR_STORAGE_FILE_AUTO, it - * will probe to automatically identify the format. + * will probe to automatically identify the format. Does not recurse. * * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a * format, since a malicious guest can turn a raw file into any @@ -910,12 +911,69 @@ cleanup: return ret; }

+/* Recursive workhorse for virStorageFileGetMetadata. */ +static virStorageFileMetadataPtr +virStorageFileGetMetadataRecurse(const char *path, int format, + uid_t uid, gid_t gid, + bool allow_probe, virHashTablePtr cycle) +{ + int fd; + virStorageFileMetadataPtr ret = NULL; + + if (virHashLookup(cycle, path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s is self-referential"), + path); + return NULL; + } + if (virHashAddEntry(cycle, path, (void *)1) < 0) + return NULL; + + if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("cannot open file '%s'"), path); + return NULL; + } + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + VIR_FORCE_CLOSE(fd); + return NULL; + } + if (virStorageFileGetMetadataFromFD(path, fd, format, ret) < 0) { + virStorageFileFreeMetadata(ret); + VIR_FORCE_CLOSE(fd); + return NULL; + } + + if (VIR_CLOSE(fd) < 0) + virReportSystemError(errno, _("could not close file %s"), path);

If this isn't fatal to the operation, shouldn't it be a warning instead of an error? (And if it is fatal, it should free the remaining resources and return NULL)

+ + if (ret->backingStoreIsFile) { + if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + ret->backingStoreFormat = VIR_STORAGE_FILE_RAW; + else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) + ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; + format = ret->backingStoreFormat;

Why are you bothering to set this, instead of just using ret->backingStoreFormat in the args to the following function call? (It's harmless, though, so no big deal)

+ ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + format, + uid, gid, + allow_probe, + cycle); + } + + return ret; +} + /** * virStorageFileGetMetadata: * * Extract metadata about the storage volume with the specified * image format. If image format is VIR_STORAGE_FILE_AUTO, it - * will probe to automatically identify the format. + * will probe to automatically identify the format. Recurses through + * the entire chain. + * + * Open files using UID and GID (or pass -1 for the current user/group). + * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. * * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a * format, since a malicious guest can turn a raw file into any @@ -923,27 +981,24 @@ cleanup: * * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO * it indicates the image didn't specify an explicit format for its - * backing store. Callers are advised against probing for the - * backing store format in this case. + * backing store. Callers are advised against using ALLOW_PROBE, as + * it would probe the backing store format in this case. * - * Caller MUST free @meta after use via virStorageFileFreeMetadata. + * Caller MUST free result after use via virStorageFileFreeMetadata. */ -int -virStorageFileGetMetadata(const char *path, - int format, - virStorageFileMetadata *meta) +virStorageFileMetadataPtr +virStorageFileGetMetadata(const char *path, int format, + uid_t uid, gid_t gid, + bool allow_probe) { - int fd, ret; - - if ((fd = open(path, O_RDONLY)) < 0) { - virReportSystemError(errno, _("cannot open file '%s'"), path); - return -1; - } - - ret = virStorageFileGetMetadataFromFD(path, fd, format, meta); - - VIR_FORCE_CLOSE(fd); + virHashTablePtr cycle = virHashCreate(5, NULL); + virStorageFileMetadataPtr ret;

+ if (!cycle) + return NULL; + ret = virStorageFileGetMetadataRecurse(path, format, uid, gid, + allow_probe, cycle); + virHashFree(cycle); return ret; }

@@ -958,6 +1013,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return;

+ virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta); } diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 683ec73..18dea6a 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -50,13 +50,16 @@ enum virStorageFileFormat {

VIR_ENUM_DECL(virStorageFileFormat);

-typedef struct _virStorageFileMetadata { +typedef struct _virStorageFileMetadata virStorageFileMetadata; +typedef virStorageFileMetadata *virStorageFileMetadataPtr; +struct _virStorageFileMetadata { char *backingStore; - int backingStoreFormat; + int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; + virStorageFileMetadataPtr backingMeta; unsigned long long capacity; bool encrypted; -} virStorageFileMetadata; +};

# ifndef DEV_BSIZE # define DEV_BSIZE 512 @@ -66,15 +69,16 @@ int virStorageFileProbeFormat(const char *path); int virStorageFileProbeFormatFromFD(const char *path, int fd);

-int virStorageFileGetMetadata(const char *path, - int format, - virStorageFileMetadata *meta); +virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe); int virStorageFileGetMetadataFromFD(const char *path, int fd, int format, virStorageFileMetadata *meta);

-void virStorageFileFreeMetadata(virStorageFileMetadata *meta); +void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta);

int virStorageFileResize(const char *path, unsigned long long capacity);

ACK, assuming acceptable response to my above questions.

On 10/13/2012 06:00 PM, Eric Blake wrote:

Requiring pre-allocation was an unusual idiom. It allowed iteration over the backing chain to use fewer mallocs, but made one-shot clients harder to read. Also, this makes it easier for a future patch to move away from opening fds on every iteration over the chain.

* src/util/storage_file.h (virStorageFileGetMetadataFromFD): Alter signature. * src/util/storage_file.c (virStorageFileGetMetadataFromFD): Allocate return value. (virStorageFileGetMetadata): Update clients. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Likewise. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. --- src/conf/domain_conf.c | 11 ++++------ src/qemu/qemu_driver.c | 9 +------- src/storage/storage_backend_fs.c | 12 +++------- src/util/storage_file.c | 47 +++++++++++++++++++--------------------- src/util/storage_file.h | 7 +++--- 5 files changed, 33 insertions(+), 53 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index acf1904..a1d5a1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14644,16 +14644,11 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, int ret = -1; size_t depth = 0; char *nextpath = NULL; - virStorageFileMetadata *meta; + virStorageFileMetadata *meta = NULL;

if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0;

- if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - return ret; - } - if (disk->format > 0) { format = disk->format; } else { @@ -14696,7 +14691,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } }

- if (virStorageFileGetMetadataFromFD(path, fd, format, meta) < 0) { + if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) { VIR_FORCE_CLOSE(fd); goto cleanup; } @@ -14730,6 +14725,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, /* Allow probing for image formats that are safe */ if (format == VIR_STORAGE_FILE_AUTO_SAFE) format = VIR_STORAGE_FILE_AUTO; + virStorageFileFreeMetadata(meta); + meta = NULL; } while (nextpath);

ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d1c63c..0e7665e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9202,14 +9202,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } }

- if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (virStorageFileGetMetadataFromFD(path, fd, - format, - meta) < 0) + if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) goto cleanup;

/* Get info for normal formats */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index db19b87..cecc2d2 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -68,12 +68,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, { int fd = -1; int ret = -1; - virStorageFileMetadata *meta; - - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - return ret; - } + virStorageFileMetadata *meta = NULL;

*backingStore = NULL; *backingStoreFormat = VIR_STORAGE_FILE_AUTO; @@ -96,9 +91,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, goto error; }

- if (virStorageFileGetMetadataFromFD(target->path, fd, - target->format, - meta) < 0) { + if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, + target->format))) { ret = -1; goto error; } diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 0f879bd..5387f1e 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -852,41 +852,43 @@ virStorageFileProbeFormat(const char *path) * backing store. Callers are advised against probing for the * backing store format in this case. * - * Caller MUST free @meta after use via virStorageFileFreeMetadata. + * Caller MUST free the result after use via virStorageFileFreeMetadata. */ -int +virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, - int format, - virStorageFileMetadata *meta) + int format) { + virStorageFileMetadata *meta = NULL; unsigned char *head = NULL; ssize_t len = STORAGE_MAX_HEAD; - int ret = -1; + virStorageFileMetadata *ret = NULL; struct stat sb;

- memset(meta, 0, sizeof(*meta)); + if (VIR_ALLOC(meta) < 0) { + virReportOOMError(); + return NULL; + }

if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); - return -1; + goto cleanup; }

- /* No header to probe for directories */ - if (S_ISDIR(sb.st_mode)) { - return 0; - } + /* No header to probe for directories, but also no backing file */ + if (S_ISDIR(sb.st_mode)) + return meta;

if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot seek to start of '%s'"), path); - return -1; + goto cleanup; }

if (VIR_ALLOC_N(head, len) < 0) { virReportOOMError(); - return -1; + goto cleanup; }

if ((len = read(fd, head, len)) < 0) { @@ -904,9 +906,13 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; }

- ret = virStorageFileGetMetadataFromBuf(format, path, head, len, meta); + if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0) + goto cleanup; + ret = meta; + meta = NULL;

cleanup: + virStorageFileFreeMetadata(meta); VIR_FREE(head); return ret; } @@ -934,21 +940,12 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; }

- if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); - VIR_FORCE_CLOSE(fd); - return NULL; - } - if (virStorageFileGetMetadataFromFD(path, fd, format, ret) < 0) { - virStorageFileFreeMetadata(ret); - VIR_FORCE_CLOSE(fd); - return NULL; - } + ret = virStorageFileGetMetadataFromFD(path, fd, format);

if (VIR_CLOSE(fd) < 0) virReportSystemError(errno, _("could not close file %s"), path);

- if (ret->backingStoreIsFile) { + if (ret && ret->backingStoreIsFile) { if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) ret->backingStoreFormat = VIR_STORAGE_FILE_RAW; else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 18dea6a..9a60451 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -73,10 +73,9 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, int format, uid_t uid, gid_t gid, bool allow_probe); -int virStorageFileGetMetadataFromFD(const char *path, - int fd, - int format, - virStorageFileMetadata *meta); +virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, + int fd, + int format);

void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta);

Fairly mechanical and straighforward. ACK.

On 10/13/2012 06:00 PM, Eric Blake wrote:

In order to search for a backing file name as literally present in a chain, we need to remember if the chain had relative names. Also, searching for absolute names is easier if we only have to canonicalize once, rather than on every iteration.

* src/util/storage_file.h (_virStorageFileMetadata): Add field. * src/util/storage_file.c (virStorageFileGetMetadataFromBuf): (virStorageFileFreeMetadata): Manage it (absolutePathFromBaseFile): Store absolute names in canonical form. --- src/util/storage_file.c | 28 ++++++++++++++++++++-------- src/util/storage_file.h | 3 ++- 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c

So I just noticed that this file is in util rather than store. I guess because its functions are called both by the storage driver and directly from qemu, right?

index 5387f1e..6f299c0 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -28,6 +28,7 @@ #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> +#include <stdlib.h> #ifdef __linux__ # if HAVE_LINUX_MAGIC_H # include <linux/magic.h> @@ -531,18 +532,22 @@ static char * absolutePathFromBaseFile(const char *base_file, const char *path) { char *res; + char *tmp; size_t d_len = dir_len (base_file);

/* If path is already absolute, or if dirname(base_file) is ".", just return a copy of path. */ if (*path == '/' || d_len == 0) - return strdup(path); + return canonicalize_file_name(path);

/* Ensure that the following cast-to-int is valid. */ if (d_len > INT_MAX) return NULL;

- ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path)); + if (virAsprintf(&tmp, "%.*s/%s", (int) d_len, base_file, path) < 0) + return NULL; + res = canonicalize_file_name(tmp); + VIR_FREE(tmp); return res; }

@@ -698,17 +703,23 @@ virStorageFileGetMetadataFromBuf(int format,

meta->backingStoreIsFile = false; if (backing != NULL) { + meta->backingStore = strdup(backing); + if (meta->backingStore == NULL) { + virReportOOMError(); + VIR_FREE(backing); + return -1; + } if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; + meta->backingStoreRaw = meta->backingStore; meta->backingStore = absolutePathFromBaseFile(path, backing); - } else { - meta->backingStore = strdup(backing); + if (meta->backingStore == NULL) { + virReportOOMError(); + VIR_FREE(backing); + return -1; + } } VIR_FREE(backing); - if (meta->backingStore == NULL) { - virReportOOMError(); - return -1; - } meta->backingStoreFormat = backingFormat; } else { meta->backingStore = NULL; @@ -1012,6 +1023,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta)

virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); + VIR_FREE(meta->backingStoreRaw); VIR_FREE(meta); }

diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 9a60451..685fcb8 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -53,7 +53,8 @@ VIR_ENUM_DECL(virStorageFileFormat); typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { - char *backingStore; + char *backingStore; /* Canonical name (absolute file, or protocol) */ + char *backingStoreRaw; /* If file, original name, possibly relative */ int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; virStorageFileMetadataPtr backingMeta;

ACK.

On 10/13/2012 06:00 PM, Eric Blake wrote:

Technically, we should not be re-probing any file that qemu might be currently writing to. As such, we should cache the backing file chain prior to starting qemu. This patch adds the cache, but does not use it until the next patch.

Ultimately, we want to also store the chain in domain XML, so that it is remembered across libvirtd restarts, and so that the only kosher way to modify the backing chain of an offline domain will be through libvirt API calls, but we aren't there yet. So for now, we merely invalidate the cache any time we do a live operation that alters the chain (block-pull, block-commit, external disk snapshot), as well as tear down the cache when the domain is not running.

* src/conf/domain_conf.h (_virDomainDiskDef): New field. * src/conf/domain_conf.c (virDomainDiskDefFree): Clean new field. * src/qemu/qemu_domain.h (qemuDomainDetermineDiskChain): New prototype. * src/qemu/qemu_domain.c (qemuDomainDetermineDiskChain): New function. * src/qemu/qemu_driver.c (qemuDomainAttachDeviceDiskLive) (qemuDomainChangeDiskMediaLive): Pre-populate chain. (qemuDomainSnapshotCreateSingleDiskActive): Uncache chain before snapshot. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Update chain after block pull. (qemuProcessStop): Uncache chain after process ends. --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 2 ++ src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ src/qemu/qemu_process.c | 20 ++++++++++++++++++++ 6 files changed, 73 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a1d5a1a..f4c05a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -971,6 +971,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->src); VIR_FREE(def->dst); VIR_FREE(def->driverName); + virStorageFileFreeMetadata(def->chain);

Do you think this name is descriptive enough? (Probably for you, but for someone looking at domain_conf.c for the first time...)

VIR_FREE(def->mirror); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1d20522..06c1ccd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -47,6 +47,7 @@ # include "virobject.h" # include "device_conf.h" # include "bitmap.h" +# include "storage_file.h"

/* forward declarations of all device types, required by * virDomainDeviceDef @@ -568,6 +569,7 @@ struct _virDomainDiskDef { } auth; char *driverName; int format; /* enum virStorageFileFormat */ + virStorageFileMetadataPtr chain;

char *mirror; int mirrorFormat; /* enum virStorageFileFormat */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b51edc2..9675454 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2010,3 +2010,36 @@ qemuDomainCleanupRun(struct qemud_driver *driver, priv->ncleanupCallbacks = 0; priv->ncleanupCallbacks_max = 0; } + +int +qemuDomainDetermineDiskChain(struct qemud_driver *driver, + virDomainDiskDefPtr disk, + bool force) +{ + int format; + + if (!disk->src) + return 0; + + if (disk->chain) { + if (force) { + virStorageFileFreeMetadata(disk->chain); + disk->chain = NULL; + } else { + return 0; + } + } + if (disk->format > 0) + format = disk->format; + else if (driver->allowDiskFormatProbing) + format = VIR_STORAGE_FILE_AUTO; + else + format = VIR_STORAGE_FILE_RAW; + + disk->chain = virStorageFileGetMetadata(disk->src, format, + driver->user, driver->group, + driver->allowDiskFormatProbing); + if (!disk->chain && !force) + return -1; + return 0;

I would naively think that if force was true, you would want to return failure if you couldn't get the chain, you're returning failure only if force is false. Since that seems counterintuitive to me, I thought I should point it out (very likely it's correct and I again just don't understand, but better safe than sorry :-)

+} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 26b6c55..8a66f14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -343,6 +343,9 @@ bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, int qemuDomainCheckDiskPresence(struct qemud_driver *driver, virDomainObjPtr vm, bool start_with_state); +int qemuDomainDetermineDiskChain(struct qemud_driver *driver, + virDomainDiskDefPtr disk, + bool force);

int qemuDomainCleanupAdd(virDomainObjPtr vm, qemuDomainCleanupCallback cb); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0e7665e..6588b82 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5705,6 +5705,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; }

+ if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) + goto end; +

.. after all - here you have force = false, and expect that it might fail, so I'm probably wrong...

if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -5933,6 +5936,9 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, virCgroupPtr cgroup = NULL; int ret = -1;

+ if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) + goto end; + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -10677,6 +10683,14 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, disk->src = source; origdriver = disk->format; disk->format = VIR_STORAGE_FILE_RAW; /* Don't want to probe backing files */ + /* XXX Here, we know we are about to alter disk->chain if + * successful, so we nuke the existing chain so that future + * commands will recompute it. Better would be storing the chain + * ourselves rather than reprobing, but this requires modifying + * domain_conf and our XML to fully track the chain across + * libvirtd restarts. */

So are you avoiding this for now just to reduce the complexity of this series? Or is there some other reason that it's not practical?

+ virStorageFileFreeMetadata(disk->chain); + disk->chain = NULL;

if (virDomainLockDiskAttach(driver->lockManager, driver->uri, vm, disk) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f8a2bfd..58cd8da 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -909,6 +909,14 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (disk) { path = disk->src; event = virDomainEventBlockJobNewFromObj(vm, path, type, status); + /* XXX If we completed a block pull, then recompute the cached + * backing chain to match. Better would be storing the chain + * ourselves rather than reprobing, but this requires + * modifying domain_conf and our XML to fully track the chain + * across libvirtd restarts. */ + if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL && + status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) + qemuDomainDetermineDiskChain(driver, disk, true); }

virDomainObjUnlock(vm); @@ -4062,6 +4070,18 @@ void qemuProcessStop(struct qemud_driver *driver, networkReleaseActualDevice(net); }

+ /* XXX For now, disk chains should only be cached while qemu is + * running. Since we don't track the chain in XML, a user is free + * to update the chain while the domain is offline, and then when + * they next boot the domain we should re-read the chain from the + * files at that point in time. Only when we track the chain in + * XML can we forbid the user from altering the chain of an + * offline domain. */

Well, there are two possible levels of caching: 1) cache it in the domain's status for as long as the domain is running (this would alleviate the need to recompute it when libvirtd is restarted), 2) cache it in the domain's persistent config, so that it survives restarts of the domain. Is it used so much that either level of caching would actually be a substantial gain?

+ for (i = 0; i < def->ndisks; i++) { + virStorageFileFreeMetadata(def->disks[i]->chain); + def->disks[i]->chain = NULL; + } +

If you're storing the cache in vm->def rather than vm->newDef, shouldn't this be taken care of automatically? (At domain startup in qemuProcessStart(), virDomainObjSetDefTransient() is called, which makes a copy of vm->def into vm->newDef, then at domain shutdown in qemuProcessStop() (right at the bottom of the function), vm->def is freed, and vm->newDef is moved into its place. Since you're only ever populating the chain in vm->def if the domain is running, and since vm->newDef always exists when the domain is running (unless it's a transient domain, in which case we don't care), you can be guaranteed that vm->def will *always* be freed during qemuProcessStop anyway, making the above loop redundant.

retry: if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) { if (ret == -EBUSY && (retries++ < 5)) {

In the end, the only real issue I see is the extra unneeded loop right there at the bottom. Other than that, if there's a better name to use in the virDomainDiskDef in place of simply "chain" that would be nice, but not necessary.

On 10/16/2012 04:07 PM, Eric Blake wrote:

On 10/16/2012 01:02 PM, Laine Stump wrote:

...

On 10/13/2012 06:00 PM, Eric Blake wrote:

...

Technically, we should not be re-probing any file that qemu might be currently writing to. As such, we should cache the backing file chain prior to starting qemu. This patch adds the cache, but does not use it until the next patch.

Ultimately, we want to also store the chain in domain XML, so that it is remembered across libvirtd restarts, and so that the only kosher way to modify the backing chain of an offline domain will be through libvirt API calls, but we aren't there yet. So for now, we merely invalidate the cache any time we do a live operation that alters the chain (block-pull, block-commit, external disk snapshot), as well as tear down the cache when the domain is not running.

+++ b/src/conf/domain_conf.c @@ -971,6 +971,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->src); VIR_FREE(def->dst); VIR_FREE(def->driverName); + virStorageFileFreeMetadata(def->chain); Do you think this name is descriptive enough? (Probably for you, but for someone looking at domain_conf.c for the first time...) Is diskdef->backingChain any better?

It sounds more informative to me anyway.

...

...

+ if (!disk->chain && !force) + return -1; + return 0;

I would naively think that if force was true, you would want to return failure if you couldn't get the chain, you're returning failure only if force is false. Since that seems counterintuitive to me, I thought I should point it out (very likely it's correct and I again just don't understand, but better safe than sorry :-) I was using 'force' more as a flag to invalidate any existing chain, and to optionally provide a new chain if convenient. qemu_process.c was the only caller in this patch that passes force=true, and it doesn't care about the state of the chain afterwards. I guess I can simplify this to just return -1 if !disk->chain, and then it is up to the caller that cares about having a valid chain to check for a 0 result to know if def->chain got populated.

If you think that the two things (forcing the invalidation of existing chain, and caring whether or no the chain was populated at the end) would ever be required in a different combination, then I suppose that would be good. On the other hand, if what you've got works for the current situation, it could always be enhanced later - it's not a public API or anything. So I'm fine either way.

...

...

+++ b/src/qemu/qemu_driver.c @@ -5705,6 +5705,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; }

+ if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) + goto end; + .. after all - here you have force = false, and expect that it might fail, so I'm probably wrong... Passing force=false says that 'if there is already a chain cached, assume it is right and reuse it; so the only way I can fail is if there is nothing cached, but I couldn't read the chain'.

Okay.

...

...

@@ -10677,6 +10683,14 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, disk->src = source; origdriver = disk->format; disk->format = VIR_STORAGE_FILE_RAW; /* Don't want to probe backing files */ + /* XXX Here, we know we are about to alter disk->chain if + * successful, so we nuke the existing chain so that future + * commands will recompute it. Better would be storing the chain + * ourselves rather than reprobing, but this requires modifying + * domain_conf and our XML to fully track the chain across + * libvirtd restarts. */ So are you avoiding this for now just to reduce the complexity of this series? Or is there some other reason that it's not practical? Short answer - avoiding the complexity of this series. I _do_ want to cache the entire backing chain, in both runtime and persistent XML, as a user-visible structure, but that's a much bigger task. Hence the XXX notes to remind whoever gets to that task

Ooh! Hot potato!!!! :-)

to remember that this is one of the places where we have told qemu to modify the chain, and therefore we would need to reflect it in the XML we show to users.

...

...

+ /* XXX For now, disk chains should only be cached while qemu is + * running. Since we don't track the chain in XML, a user is free + * to update the chain while the domain is offline, and then when + * they next boot the domain we should re-read the chain from the + * files at that point in time. Only when we track the chain in + * XML can we forbid the user from altering the chain of an + * offline domain. */ Well, there are two possible levels of caching: 1) cache it in the domain's status for as long as the domain is running (this would alleviate the need to recompute it when libvirtd is restarted), 2) cache it in the domain's persistent config, so that it survives restarts of the domain. Is it used so much that either level of caching would actually be a substantial gain? I'm not so sure about the gain for reduced number of open() calls, so much as the real flexibility in allowing the user to specify exactly which files qemu is allowed to open, and to visually see the entire chain at a glance (until this week's qemu.git, you couldn't even get a single 'qemu-img info' to show you an entire chain). But that can come later; for now, I needed only enough chain information to make it possible to implement the block-commit --shallow flag and selinux labeling.

...

...

+ for (i = 0; i < def->ndisks; i++) { + virStorageFileFreeMetadata(def->disks[i]->chain); + def->disks[i]->chain = NULL; + } + If you're storing the cache in vm->def rather than vm->newDef, shouldn't this be taken care of automatically? (At domain startup in qemuProcessStart(), virDomainObjSetDefTransient() is called, which makes a copy of vm->def into vm->newDef, then at domain shutdown in qemuProcessStop() (right at the bottom of the function), vm->def is freed, and vm->newDef is moved into its place. Ooh, good point. I'll simplify this code for v3.

On 10/13/2012 06:00 PM, Eric Blake wrote:

We used to walk the backing file chain at least twice per disk, once to set up cgroup device whitelisting, and once to set up security labeling. Rather than walk the chain every iteration, which possibly includes calls to fork() in order to open root-squashed NFS files, we can exploit the cache of the previous patch.

Obviously creating the rule that you can't mess with the backing chain outside the libvirt API, but I think you've already said that :-)

* src/conf/domain_conf.h (virDomainDiskDefForeachPath): Alter signature. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Require caller to supply backing chain via disk, if recursion is desired. * src/security/security_dac.c (virSecurityDACSetSecurityImageLabel): Adjust caller. * src/security/security_selinux.c (virSecuritySELinuxSetSecurityImageLabel): Likewise. * src/security/virt-aa-helper.c (get_files): Likewise. * src/qemu/qemu_cgroup.c (qemuSetupDiskCgroup) (qemuTeardownDiskCgroup): Likewise. (qemuSetupCgroup): Pre-populate chain. --- src/conf/domain_conf.c | 100 +++++++--------------------------------- src/conf/domain_conf.h | 2 - src/qemu/qemu_cgroup.c | 12 ++--- src/security/security_dac.c | 7 --- src/security/security_selinux.c | 11 ----- src/security/virt-aa-helper.c | 27 +++++++---- 6 files changed, 40 insertions(+), 119 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4c05a3..2c0b296 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14633,110 +14633,42 @@ done: }

+/* Call iter(disk, name, depth, opaque) for each element of disk and + its backing chain in the pre-populated disk->chain. + ignoreOpenFailure determines whether to warn about a chain that + mentions a backing file without also having metadata on that + file. */ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, - bool allowProbing, bool ignoreOpenFailure, - uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque) { - virHashTablePtr paths = NULL; - int format; int ret = -1; size_t depth = 0; - char *nextpath = NULL; - virStorageFileMetadata *meta = NULL; + virStorageFileMetadata *tmp;

if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0;

- if (disk->format > 0) { - format = disk->format; - } else { - if (allowProbing) { - format = VIR_STORAGE_FILE_AUTO; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk format for %s and probing is disabled"), - disk->src); - goto cleanup; - } - } - - paths = virHashCreate(5, NULL); - - do { - const char *path = nextpath ? nextpath : disk->src; - int fd; - - if (iter(disk, path, depth, opaque) < 0) - goto cleanup; + if (iter(disk, disk->src, 0, opaque) < 0) + goto cleanup;

- if (virHashLookup(paths, path)) { + tmp = disk->chain; + while (tmp && tmp->backingStoreIsFile) { + if (!ignoreOpenFailure && !tmp->backingMeta) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - disk->src); + _("unable to visit backing chain file %s"), + tmp->backingStore); goto cleanup; } - - if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { - if (ignoreOpenFailure) { - char ebuf[1024]; - VIR_WARN("Ignoring open failure on %s: %s", path, - virStrerror(-fd, ebuf, sizeof(ebuf))); - break; - } else { - virReportSystemError(-fd, _("unable to open disk path %s"), - path); - goto cleanup; - } - } - - if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) { - VIR_FORCE_CLOSE(fd); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) - virReportSystemError(errno, - _("could not close file %s"), - path); - - if (virHashAddEntry(paths, path, (void*)0x1) < 0) + if (iter(disk, tmp->backingStore, ++depth, opaque) < 0) goto cleanup; - - depth++; - VIR_FREE(nextpath); - nextpath = meta->backingStore; - meta->backingStore = NULL; - - /* Stop iterating if we reach a non-file backing store */ - if (nextpath && !meta->backingStoreIsFile) { - VIR_DEBUG("Stopping iteration on non-file backing store: %s", - nextpath); - break; - } - - format = meta->backingStoreFormat; - - if (format == VIR_STORAGE_FILE_AUTO && - !allowProbing) - format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */ - - /* Allow probing for image formats that are safe */ - if (format == VIR_STORAGE_FILE_AUTO_SAFE) - format = VIR_STORAGE_FILE_AUTO; - virStorageFileFreeMetadata(meta); - meta = NULL; - } while (nextpath); + tmp = tmp->backingMeta; + }

ret = 0;

cleanup: - virHashFree(paths); - VIR_FREE(nextpath); - virStorageFileFreeMetadata(meta); - return ret; }

Whew! Can't see the code for all the deletions! :-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 06c1ccd..87faa7e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2139,9 +2139,7 @@ typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk, void *opaque);

int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, - bool allowProbing, bool ignoreOpenFailure, - uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque);

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 166f9b9..10acb26 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -87,16 +87,14 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, }

-int qemuSetupDiskCgroup(struct qemud_driver *driver, +int qemuSetupDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { qemuCgroupData data = { vm, cgroup }; return virDomainDiskDefForeachPath(disk, - driver->allowDiskFormatProbing, true, - driver->user, driver->group, qemuSetupDiskPathAllow, &data); } @@ -129,16 +127,14 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, }

-int qemuTeardownDiskCgroup(struct qemud_driver *driver, +int qemuTeardownDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { qemuCgroupData data = { vm, cgroup }; return virDomainDiskDefForeachPath(disk, - driver->allowDiskFormatProbing, true, - driver->user, driver->group, qemuTeardownDiskPathDeny, &data); } @@ -232,7 +228,9 @@ int qemuSetupCgroup(struct qemud_driver *driver, }

for (i = 0; i < vm->def->ndisks ; i++) { - if (qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0) + if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], + false) < 0 || + qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0) goto cleanup; }

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index f126aa5..5f11810 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -358,8 +358,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk)

{ - uid_t user; - gid_t group; void *params[2]; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);

@@ -369,15 +367,10 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0;

- if (virSecurityDACGetImageIds(def, priv, &user, &group)) - return -1; - params[0] = mgr; params[1] = def; return virDomainDiskDefForeachPath(disk, - virSecurityManagerGetAllowDiskFormatProbing(mgr), false, - user, group, virSecurityDACSetSecurityFileLabel, params); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..00c34c2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1022,13 +1022,10 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk)

{ - bool allowDiskFormatProbing; virSecuritySELinuxCallbackData cbdata; cbdata.manager = mgr; cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);

- allowDiskFormatProbing = virSecurityManagerGetAllowDiskFormatProbing(mgr); - if (cbdata.secdef == NULL) return -1;

@@ -1038,16 +1035,8 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0;

- /* XXX On one hand, it would be nice to have the driver's uid:gid - * here so we could retry opens with it. On the other hand, it - * probably doesn't matter because in practice that's only useful - * for files on root-squashed NFS shares, and NFS doesn't properly - * support selinux anyway. - */ return virDomainDiskDefForeachPath(disk, - allowDiskFormatProbing, true, - -1, -1, /* current process uid:gid */ virSecuritySELinuxSetSecurityFileLabel, &cbdata); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 51daa4b..729c0d1 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -919,19 +919,30 @@ get_files(vahControl * ctl) }

for (i = 0; i < ctl->def->ndisks; i++) { + int ret; + int format; + virDomainDiskDefPtr disk = ctl->def->disks[i]; + + if (disk->format > 0) + format = disk->format; + else if (ctl->allowDiskFormatProbing) + format = VIR_STORAGE_FILE_AUTO; + else + format = VIR_STORAGE_FILE_RAW;

It seems like I've seen this same bit of code a few times now...

+ + /* XXX - if we knew the qemu user:group here we could send it in + * so that the open could be re-tried as that user:group. + */ + disk->chain = virStorageFileGetMetadata(disk->src, format, -1, -1, + ctl->allowDiskFormatProbing, + NULL); + /* XXX passing ignoreOpenFailure = true to get back to the behavior * from before using virDomainDiskDefForeachPath. actually we should * be passing ignoreOpenFailure = false and handle open errors more * careful than just ignoring them. - * XXX2 - if we knew the qemu user:group here we could send it in - * so that the open could be re-tried as that user:group. */ - int ret = virDomainDiskDefForeachPath(ctl->def->disks[i], - ctl->allowDiskFormatProbing, - true, - -1, -1, /* current uid:gid */ - add_file_path, - &buf); + ret = virDomainDiskDefForeachPath(disk, true, add_file_path, &buf); if (ret != 0) goto clean; }

ACK.

On 10/13/2012 06:00 PM, Eric Blake wrote:

qemu 1.3 will be adding a 'block-commit' monitor command, per qemu.git commit ed61fc1. It matches nicely to the libvirt API virDomainBlockCommit.

Hmm. Imagine that. What serendipity. I wonder how that could have happened. etc.... :-)

* src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCK_COMMIT): New bit. * src/qemu/qemu_capabilities.c (qemuCapsProbeQMPCommands): Set it. * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): New prototype. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit): Likewise. * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Implement it. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit): Likewise. (qemuMonitorJSONHandleBlockJobImpl) (qemuMonitorJSONGetBlockJobInfoOne): Handle new event type. ---

Change from v1: upstream qemu now has the commit recalculate backing chain after commit completes

src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_monitor.c | 30 ++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 7 +++++++ src/qemu/qemu_monitor_json.c | 34 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 7 +++++++ src/qemu/qemu_process.c | 15 +++++++++------ 7 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fb0fe54..73a12f1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -187,6 +187,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "reboot-timeout", /* 110 */ "dump-guest-core", "seamless-migration", + "block-commit", );

struct _qemuCaps { @@ -1881,6 +1882,8 @@ qemuCapsProbeQMPCommands(qemuCapsPtr caps, qemuCapsSet(caps, QEMU_CAPS_SPICE); else if (STREQ(name, "query-kvm")) qemuCapsSet(caps, QEMU_CAPS_KVM); + else if (STREQ(name, "block-commit")) + qemuCapsSet(caps, QEMU_CAPS_BLOCK_COMMIT); VIR_FREE(name); } VIR_FREE(commands); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 5d343c1..6939c45 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -150,6 +150,7 @@ enum qemuCapsFlags { QEMU_CAPS_REBOOT_TIMEOUT = 110, /* -boot reboot-timeout */ QEMU_CAPS_DUMP_GUEST_CORE = 111, /* dump-guest-core-parameter */ QEMU_CAPS_SEAMLESS_MIGRATION = 112, /* seamless-migration for SPICE */ + QEMU_CAPS_BLOCK_COMMIT = 113, /* block-commit */

QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 85b0bc2..68c6d3f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2786,6 +2786,36 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) return ret; }

+/* Start a block-commit block job. bandwidth is in MB/sec. */ +int +qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, + const char *top, const char *base, + unsigned long bandwidth) +{ + int ret = -1; + unsigned long long speed; + + VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", + mon, device, NULLSTR(top), NULLSTR(base), bandwidth); + + /* Convert bandwidth MiB to bytes */ + speed = bandwidth; + if (speed > ULLONG_MAX / 1024 / 1024) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + ULLONG_MAX / 1024 / 1024); + return -1; + } + speed <<= 20; + + if (mon->json) + ret = qemuMonitorJSONBlockCommit(mon, device, top, base, speed); + else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("block-commit requires JSON monitor")); + return ret; +} + int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 54b3a99..9bdc802 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -521,6 +521,13 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+int qemuMonitorBlockCommit(qemuMonitorPtr mon, + const char *device, + const char *top, + const char *base, + unsigned long bandwidth) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index bd52ce4..501b338 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -803,6 +803,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,

if (STREQ(type_str, "stream")) type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; + else if (STREQ(type_str, "commit")) + type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT;

switch ((virConnectDomainEventBlockJobStatus) event) { case VIR_DOMAIN_BLOCK_JOB_COMPLETED: @@ -3269,6 +3271,36 @@ cleanup: return ret; }

+/* speed is in bytes/sec */ +int +qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, + const char *top, const char *base, + unsigned long speed)

up above you're using unsigned long long for speed. If qemuMonitorJSONMakeCommand only accepts unsigned long, shouldn't you be limiting bandwidth in the caller to ULONG_MAX / 1024 / 1024?

+{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("block-commit", + "s:device", device, + "U:speed", speed, + "s:top", top, + base ? "s:base" : NULL, base, + NULL); + if (!cmd) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + ret = qemuMonitorJSONCheckError(cmd, reply); + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, char **reply_str, @@ -3385,6 +3417,8 @@ static int qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry, } if (STREQ(type, "stream")) info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; + else if (STREQ(type, "commit")) + info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT; else info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 63b84c6..71bc6aa 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -235,6 +235,13 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, bool reuse); int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions);

+int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, + const char *device, + const char *top, + const char *base, + unsigned long bandwidth) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, char **reply_str, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 58cd8da..3396258 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -909,12 +909,15 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (disk) { path = disk->src; event = virDomainEventBlockJobNewFromObj(vm, path, type, status); - /* XXX If we completed a block pull, then recompute the cached - * backing chain to match. Better would be storing the chain - * ourselves rather than reprobing, but this requires - * modifying domain_conf and our XML to fully track the chain - * across libvirtd restarts. */ - if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL && + /* XXX If we completed a block pull or commit, then recompute + * the cached backing chain to match. Better would be storing + * the chain ourselves rather than reprobing, but this + * requires modifying domain_conf and our XML to fully track + * the chain across libvirtd restarts. For that matter, if + * qemu gains support for committing the active layer, we have + * to update disk->src. */ + if ((type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) && status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) qemuDomainDetermineDiskChain(driver, disk, true); }

ACK dependent on your answer about unsigned long vs. unsigned long long and "speed".

On 10/13/2012 06:00 PM, Eric Blake wrote:

This is the bare minimum to kick off a block commit. In particular, flags support is missing (shallow requires us to crawl the backing chain to determine the file name to pass to the qemu monitor command; delete requires us to track what needs to be deleted at the time the completion event fires). Also, we are relying on qemu to do error checking (such as validating 'top' and 'base' as being members of the backing chain), including the fact that the current qemu code does not support committing the active layer (although it is still planned to add that before qemu 1.3). Since the active layer won't change, we have it easy and do not have to alter the domain XML. Additionally, this will fail if SELinux is enforcing, because we fail to grant qemu proper read/write access to the files it will modify.

* src/qemu/qemu_driver.c (qemuDomainBlockCommit): New function. (qemuDriver): Register it. ---

Change from v1: allow NULL top (let qemu reject it instead) drop unused event variable

src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6588b82..a9fd93a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12540,6 +12540,80 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return qemuDomainBlockRebase(dom, path, NULL, bandwidth, flags); }

+ +static int +qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, + const char *top, unsigned long bandwidth, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; + virDomainObjPtr vm = NULL; + char *device = NULL; + int ret = -1; + int idx; + virDomainDiskDefPtr disk; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + if (!qemuCapsGet(priv->caps, QEMU_CAPS_BLOCK_COMMIT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("online commit not supported with this QEMU binary")); + goto endjob; + } + + device = qemuDiskPathToAlias(vm, path, &idx); + if (!device) + goto endjob; + disk = vm->def->disks[idx]; + + if (!disk->src) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk %s has no source file to be committed"), + disk->dst); + goto endjob; + } + + if (!top) + top = disk->src; + + /* XXX For now, we are relying on qemu to check that 'top' and + * 'base' resolve to members of the backing chain in correct + * order; but if we ever get more paranoid and track the backing + * chain ourself, we should be pre-validating the data rather than + * relying on qemu. For that matter, we need to be changing the + * SELinux label on both 'base' and the parent of 'top', so that + * qemu can open(O_RDWR) those files for the duration of the + * commit. */ + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockCommit(priv->mon, device, top, base, bandwidth); + qemuDomainObjExitMonitor(driver, vm); + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) { + vm = NULL; + goto cleanup; + } + +cleanup: + VIR_FREE(device); + if (vm) + virDomainObjUnlock(vm); + return ret; +} + static int qemuDomainOpenGraphics(virDomainPtr dom, unsigned int idx, @@ -13882,6 +13956,7 @@ static virDriver qemuDriver = { .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ + .domainBlockCommit = qemuDomainBlockCommit, /* 0.10.3 */ .isAlive = qemuIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */

ACK (with the version change you've already said you would make).

On 10/13/2012 06:00 PM, Eric Blake wrote:

Now that we can crawl the chain of backing files, we can do argument validation and implement the 'shallow' flag. In testing this, I discovered that it can be handy to pass the shallow flag and an explicit base, as a means of validating that the base is indeed the file we expected.

* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Crawl through chain to implement shallow flag. * src/libvirt.c (virDomainBlockCommit): Relax API. --- src/libvirt.c | 2 -- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index 3c6d67d..9d27240 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19378,8 +19378,6 @@ int virDomainBlockCommit(virDomainPtr dom, const char *disk, }

virCheckNonNullArgGoto(disk, error); - if (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) - virCheckNullArgGoto(base, error);

if (conn->driver->domainBlockCommit) { int ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a9fd93a..e9bc215 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12553,8 +12553,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, int ret = -1; int idx; virDomainDiskDefPtr disk; + const char *top_canon = NULL; + virStorageFileMetadataPtr top_meta = NULL; + const char *top_parent = NULL; + const char *base_canon = NULL;

- virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);

if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -12585,20 +12589,51 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, disk->dst); goto endjob; } + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) + goto endjob;

- if (!top) - top = disk->src; + if (!top) { + top_canon = disk->src; + top_meta = disk->chain; + } else if (!(top_canon = virStorageFileChainLookup(disk->chain, disk->src, + top, &top_meta, + &top_parent))) { + virReportError(VIR_ERR_INVALID_ARG, + _("could not find top '%s' in chain for '%s'"), + top, path); + goto endjob; + } + if (!top_meta || !top_meta->backingStore) { + virReportError(VIR_ERR_INVALID_ARG, + _("top '%s' in chain for '%s' has no backing file"), + top, path); + goto endjob; + } + if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { + base_canon = top_meta->backingStore; + } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, + base, NULL, NULL))) { + virReportError(VIR_ERR_INVALID_ARG, + _("could not find base '%s' below '%s' in chain " + "for '%s'"), + base ? base : "(default)", top_canon, path); + goto endjob; + } + if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && + base_canon != top_meta->backingStore) { + virReportError(VIR_ERR_INVALID_ARG, + _("base '%s' is not immediately below '%s' in chain " + "for '%s'"), + base, top_canon, path); + goto endjob; + }

- /* XXX For now, we are relying on qemu to check that 'top' and - * 'base' resolve to members of the backing chain in correct - * order; but if we ever get more paranoid and track the backing - * chain ourself, we should be pre-validating the data rather than - * relying on qemu. For that matter, we need to be changing the - * SELinux label on both 'base' and the parent of 'top', so that - * qemu can open(O_RDWR) those files for the duration of the - * commit. */ + /* XXX We need to be changing the SELinux label on both 'base' and + * the parent of 'top', so that qemu can open(O_RDWR) those files + * for the duration of the commit. */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockCommit(priv->mon, device, top, base, bandwidth); + ret = qemuMonitorBlockCommit(priv->mon, device, top_canon, base_canon, + bandwidth); qemuDomainObjExitMonitor(driver, vm);

endjob:

Looks reasonable. ACK.

On 10/17/2012 06:09 PM, Laine Stump wrote:

On 10/16/2012 06:03 PM, Eric Blake wrote:

...

Previously, snapshot code did its own permission granting (lock manager, cgroup device controller, and security manager labeling) inline. But now that we are adding block-commit and block-copy which also have to change permissions, it's better to reuse common code for the task. While snapshot should fall back to no access if read-write access failed, block-commit will want to fall back to read-only access. The common code doesn't know whether failure to grant read-write access should revert to no access (snapshot, block-copy) or read-only access (block-commit). This code can also be used to revoke access to unused files after block-pull.

+ if (mode == VIR_DISK_CHAIN_NO_ACCESS) { + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); + if (cgroup && qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) + VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src); + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + VIR_WARN("Unable to release lock on %s", disk->src);

This feels like a bit of a hackish way to get around an inadequate security driver (and lock and cgroup?) API by hijacking what's available. But then I haven't looked at just how deep the changes would need to be to make it work properly, so I won't dismiss it out of hand. It seems like something that should require a promise of later cleanup though :-)

This is just code motion; this code was previously here:

...

@@ -10881,13 +10917,8 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, goto cleanup; }

- if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) - VIR_WARN("Unable to restore security label on %s", disk->src); - if (cgroup && qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src); - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", disk->src); + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, origdisk->src, + VIR_DISK_CHAIN_NO_ACCESS);

Basically, after granting rights, if you want to then revoke rights because some later step failed, you want to log if the revoke fails, but you're already on the failure path, so there's nothing more we can do than logging. I'm not sure what you are proposing would be a more adequate security manager API. Or is that you are really complaining about my mixing of the existing virDomainDiskDefPtr (which stores the read-only and read-write labels to be used by the security manager for all files in that disk's chain) with the hack of temporarily modifying the disk to only point to the one file that I'm adding or removing from the chain? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org