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) (qcow2GetBackingStoreFormat, 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. --- v3: fix case when qcow2 uses probing for backing format src/conf/domain_conf.c | 2 +- src/conf/storage_conf.c | 15 ++++++++--- src/qemu/qemu_driver.c | 2 +- src/util/storage_file.c | 70 ++++++++++++++++++++++++++++++++++--------------- src/util/storage_file.h | 8 ++++-- 5 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f0c5d50..74b843d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14777,7 +14777,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 adfbfa6..ead5d8f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9303,7 +9303,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..0d8cb86 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) { @@ -256,7 +269,8 @@ qcow2GetBackingStoreFormat(int *format, break; *format = virStorageFileFormatTypeFromString( ((const char *)buf)+offset); - break; + if (*format <= VIR_STORAGE_FILE_NONE) + return -1; } offset += len; @@ -298,8 +312,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 +352,10 @@ 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) < 0) + return BACKING_STORE_INVALID; return BACKING_STORE_OK; } @@ -348,10 +367,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 +425,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 +436,7 @@ vmdk4GetBackingStore(char **res, goto cleanup; } if (end == start) { + *format = VIR_STORAGE_FILE_NONE; ret = BACKING_STORE_OK; goto cleanup; } @@ -464,8 +490,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 +513,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 = VIR_STORAGE_FILE_RAW; + else + *format = VIR_STORAGE_FILE_AUTO_SAFE; return BACKING_STORE_OK; } @@ -564,7 +590,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 +633,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 +710,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 +895,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, }; -- 1.7.11.7

At one point, the code passed through arbitrary strings for file formats, which supposedly lets qemu handle a new file type even before libvirt has been taught to handle it. However, to properly label files, libvirt has to learn the file type anyway, so we might as well make our life easier by only accepting file types that we are prepared to handle. This patch lets the RNG validation ensure that only known strings are let through. * docs/schemas/domaincommon.rng (driverFormat): Limit to list of supported strings. * docs/schemas/domainsnapshot.rng (driver): Likewise. --- docs/schemas/domaincommon.rng | 27 ++++++++++++++++++++++++--- docs/schemas/domainsnapshot.rng | 2 +- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 70bc0e2..1bfebde 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1188,11 +1188,32 @@ <ref name="genericName"/> </attribute> <optional> - <attribute name="type"> - <ref name="genericName"/> + <attribute name='type'> + <choice> + <ref name='diskFormat'/> + <value>aio</value> <!-- back-compat for 'raw' --> + </choice> </attribute> </optional> </define> + <define name='diskFormat'> + <choice> + <value>raw</value> + <value>dir</value> + <value>bochs</value> + <value>cloop</value> + <value>cow</value> + <value>dmg</value> + <value>iso</value> + <value>qcow</value> + <value>qcow2</value> + <value>qed</value> + <value>vmdk</value> + <value>vpc</value> + <value>fat</value> + <value>vhd</value> + </choice> + </define> <define name="driverCache"> <attribute name="cache"> <choice> @@ -3352,7 +3373,7 @@ </attribute> <optional> <attribute name='format'> - <ref name="genericName"/> + <ref name='diskFormat'/> </attribute> </optional> <optional> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 0ef0631..ecaafe9 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -105,7 +105,7 @@ <element name='driver'> <optional> <attribute name='type'> - <ref name='genericName'/> + <ref name='diskFormat'/> </attribute> </optional> <empty/> -- 1.7.11.7

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. --- v3: fix check for fat on qemu dir passthru, and only default to driver caps for file and blocks (not dir or network protocol) src/conf/domain_conf.c | 61 +++++++++++++++++---------------- src/conf/domain_conf.h | 4 +-- src/libxl/libxl_conf.c | 42 +++++++++++++---------- src/libxl/libxl_driver.c | 6 +--- src/qemu/qemu_command.c | 18 +++++----- src/qemu/qemu_domain.c | 7 ++-- src/qemu/qemu_driver.c | 89 ++++++++++++++++-------------------------------- src/qemu/qemu_hotplug.c | 9 ++--- src/vbox/vbox_tmpl.c | 6 ++-- src/xenxs/xen_sxpr.c | 30 +++++++++------- src/xenxs/xen_xm.c | 33 ++++++++++-------- 11 files changed, 146 insertions(+), 159 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15d646b..93590bf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -971,9 +971,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->src); VIR_FREE(def->dst); VIR_FREE(def->driverName); - VIR_FREE(def->driverType); VIR_FREE(def->mirror); - VIR_FREE(def->mirrorFormat); VIR_FREE(def->auth.username); VIR_FREE(def->wwn); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) @@ -4172,12 +4170,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, authUsername = NULL; def->driverName = driverName; driverName = NULL; - def->driverType = driverType; - driverType = NULL; def->mirror = mirror; mirror = NULL; - def->mirrorFormat = mirrorFormat; - mirrorFormat = NULL; def->mirroring = mirroring; def->encryption = encryption; encryption = NULL; @@ -4186,23 +4180,35 @@ 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 if (def->type == VIR_DOMAIN_DISK_TYPE_FILE || + def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) { + def->format = caps->defaultDiskDriverType; + } if (!def->driverName && caps->defaultDiskDriverName && !(def->driverName = strdup(caps->defaultDiskDriverName))) goto no_memory; - - if (def->mirror && !def->mirrorFormat && - caps->defaultDiskDriverType && - !(def->mirrorFormat = strdup(virStorageFileFormatTypeToString( - caps->defaultDiskDriverType)))) - goto no_memory; + if (mirrorFormat) { + def->mirrorFormat = virStorageFileFormatTypeFromString(mirrorFormat); + if (def->mirrorFormat <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror format value '%s'"), + driverType); + goto error; + } + } else if (def->mirror) { + def->mirrorFormat = caps->defaultDiskDriverType; + } if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(caps, def) < 0) @@ -11814,13 +11820,14 @@ virDomainDiskDefFormat(virBufferPtr buf, virDomainSnapshotLocationTypeToString(def->snapshot)); virBufferAddLit(buf, ">\n"); - if (def->driverName || def->driverType || def->cachemode || + if (def->driverName || def->format > 0 || def->cachemode || def->ioeventfd || def->event_idx || def->copy_on_read) { virBufferAddLit(buf, " <driver"); if (def->driverName) virBufferAsprintf(buf, " name='%s'", def->driverName); - if (def->driverType) - virBufferAsprintf(buf, " type='%s'", def->driverType); + if (def->format > 0) + virBufferAsprintf(buf, " type='%s'", + virStorageFileFormatTypeToString(def->format)); if (def->cachemode) virBufferAsprintf(buf, " cache='%s'", cachemode); if (def->error_policy) @@ -11932,7 +11939,8 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { virBufferEscapeString(buf, " <mirror file='%s'", def->mirror); if (def->mirrorFormat) - virBufferAsprintf(buf, " format='%s'", def->mirrorFormat); + virBufferAsprintf(buf, " format='%s'", + virStorageFileFormatTypeToString(def->mirrorFormat)); if (def->mirroring) virBufferAddLit(buf, " ready='yes'"); virBufferAddLit(buf, "/>\n"); @@ -14780,15 +14788,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, return ret; } - if (disk->driverType) { - const char *formatStr = disk->driverType; - - if ((format = virStorageFileFormatTypeFromString(formatStr)) <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown disk format '%s' for %s"), - disk->driverType, disk->src); - goto cleanup; - } + if (disk->format > 0) { + format = disk->format; } else { if (allowProbing) { format = VIR_STORAGE_FILE_AUTO; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1ee4b30..eec1f25 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -567,10 +567,10 @@ struct _virDomainDiskDef { } secret; } auth; char *driverName; - char *driverType; + int format; /* enum virStorageFileFormat */ char *mirror; - char *mirrorFormat; + int mirrorFormat; /* enum virStorageFileFormat */ bool mirroring; struct { diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b988fa7..fa931c3 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -41,6 +41,7 @@ #include "capabilities.h" #include "libxl_driver.h" #include "libxl_conf.h" +#include "storage_file.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -505,25 +506,30 @@ libxlMakeDisk(virDomainDefPtr def, virDomainDiskDefPtr l_disk, if (l_disk->driverName) { if (STREQ(l_disk->driverName, "tap") || STREQ(l_disk->driverName, "tap2")) { - if (l_disk->driverType) { - if (STREQ(l_disk->driverType, "qcow")) { - x_disk->format = DISK_FORMAT_QCOW; - x_disk->backend = DISK_BACKEND_QDISK; - } else if (STREQ(l_disk->driverType, "qcow2")) { - x_disk->format = DISK_FORMAT_QCOW2; - x_disk->backend = DISK_BACKEND_QDISK; - } else if (STREQ(l_disk->driverType, "vhd")) { - x_disk->format = DISK_FORMAT_VHD; - x_disk->backend = DISK_BACKEND_TAP; - } else if (STREQ(l_disk->driverType, "aio") || - STREQ(l_disk->driverType, "raw")) { - x_disk->format = DISK_FORMAT_RAW; - x_disk->backend = DISK_BACKEND_TAP; - } - } else { + switch (l_disk->format) { + case VIR_STORAGE_FILE_QCOW: + x_disk->format = DISK_FORMAT_QCOW; + x_disk->backend = DISK_BACKEND_QDISK; + break; + case VIR_STORAGE_FILE_QCOW2: + x_disk->format = DISK_FORMAT_QCOW2; + x_disk->backend = DISK_BACKEND_QDISK; + break; + case VIR_STORAGE_FILE_VHD: + x_disk->format = DISK_FORMAT_VHD; + x_disk->backend = DISK_BACKEND_TAP; + break; + case VIR_STORAGE_FILE_NONE: /* No subtype specified, default to raw/tap */ - x_disk->format = DISK_FORMAT_RAW; - x_disk->backend = DISK_BACKEND_TAP; + case VIR_STORAGE_FILE_RAW: + x_disk->format = DISK_FORMAT_RAW; + x_disk->backend = DISK_BACKEND_TAP; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("libxenlight does not support disk driver %s"), + virStorageFileFormatTypeToString(l_disk->format)); + return -1; } } else if (STREQ(l_disk->driverName, "file")) { x_disk->format = DISK_FORMAT_RAW; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6fe284f..f4e9aa6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3202,11 +3202,7 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) orig->driverName = disk->driverName; disk->driverName = NULL; } - if (disk->driverType) { - VIR_FREE(orig->driverType); - orig->driverType = disk->driverType; - disk->driverType = NULL; - } + orig->format = disk->format; disk->src = NULL; break; default: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0c0c400..4196caf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -43,6 +43,7 @@ #include "virnetdevtap.h" #include "base64.h" #include "device_conf.h" +#include "storage_file.h" #include <sys/utsname.h> #include <sys/stat.h> @@ -2128,11 +2129,10 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { 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 > 0 && disk->format != VIR_STORAGE_FILE_FAT) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported disk driver type for '%s'"), - disk->driverType); + virStorageFileFormatTypeToString(disk->format)); goto error; } if (!disk->readonly) { @@ -2229,10 +2229,11 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, _("transient disks not supported yet")); goto error; } - if (disk->driverType && *disk->driverType != '\0' && + if (disk->format > 0 && disk->type != VIR_DOMAIN_DISK_TYPE_DIR && qemuCapsGet(caps, QEMU_CAPS_DRIVE_FORMAT)) - virBufferAsprintf(&opt, ",format=%s", disk->driverType); + virBufferAsprintf(&opt, ",format=%s", + virStorageFileFormatTypeToString(disk->format)); /* generate geometry command string */ if (disk->geometry.cylinders > 0 && @@ -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 > 0 && disk->format != VIR_STORAGE_FILE_FAT) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported disk driver type for '%s'"), - disk->driverType); + virStorageFileFormatTypeToString(disk->format)); goto error; } if (!disk->readonly) { @@ -7024,7 +7024,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportOOMError(); goto cleanup; } - def->driverType = values[i]; + def->format = virStorageFileFormatTypeFromString(values[i]); values[i] = NULL; } else if (STREQ(keywords[i], "cache")) { if (STREQ(values[i], "off") || diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 427258d..b51edc2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -36,6 +36,7 @@ #include "virfile.h" #include "domain_event.h" #include "virtime.h" +#include "storage_file.h" #include <sys/time.h> #include <fcntl.h> @@ -1408,7 +1409,7 @@ void qemuDomainObjCheckDiskTaint(struct qemud_driver *driver, virDomainDiskDefPtr disk, int logFD) { - if (!disk->driverType && + if ((!disk->format || disk->format == VIR_STORAGE_FILE_AUTO) && driver->allowDiskFormatProbing) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_DISK_PROBING, logFD); @@ -1660,8 +1661,8 @@ qemuDomainSnapshotForEachQcow2Raw(struct qemud_driver *driver, for (i = 0; i < ndisks; i++) { /* FIXME: we also need to handle LVM here */ if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (!def->disks[i]->driverType || - STRNEQ(def->disks[i]->driverType, "qcow2")) { + if (def->disks[i]->format > 0 && + def->disks[i]->format != VIR_STORAGE_FILE_QCOW2) { if (try_all) { /* Continue on even in the face of error, since other * disks in this VM may have the same snapshot name. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 740632d..5ecb3e8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6335,11 +6335,8 @@ qemuDomainUpdateDeviceConfig(qemuCapsPtr caps, orig->driverName = disk->driverName; disk->driverName = NULL; } - if (disk->driverType) { - VIR_FREE(orig->driverType); - orig->driverType = disk->driverType; - disk->driverType = NULL; - } + if (disk->format) + orig->format = disk->format; disk->src = NULL; break; @@ -9302,13 +9299,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } /* Probe for magic formats */ - if (disk->driverType) { - if ((format = virStorageFileFormatTypeFromString(disk->driverType)) <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown disk format %s for %s"), - disk->driverType, disk->src); - goto cleanup; - } + if (disk->format) { + format = disk->format; } else { if (driver->allowDiskFormatProbing) { if ((format = virStorageFileProbeFormat(disk->src)) < 0) @@ -10480,7 +10472,7 @@ qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) if ((disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) || (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && - STRNEQ_NULLABLE(disk->driverType, "qcow2"))) { + disk->format > 0 && disk->format != VIR_STORAGE_FILE_QCOW2)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Disk '%s' does not support snapshotting"), disk->src); @@ -10674,13 +10666,14 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name); goto cleanup; } - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + if (vm->def->disks[i]->format > 0 && + vm->def->disks[i]->format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("internal snapshot for disk %s unsupported " "for storage type %s"), disk->name, - NULLSTR(vm->def->disks[i]->driverType)); + virStorageFileFormatTypeToString( + vm->def->disks[i]->format)); goto cleanup; } found = true; @@ -10767,13 +10760,12 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; char *source = NULL; - char *driverType = NULL; + int format = VIR_STORAGE_FILE_NONE; char *persistSource = NULL; - char *persistDriverType = NULL; int ret = -1; int fd = -1; char *origsrc = NULL; - char *origdriver = NULL; + int origdriver; bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { @@ -10782,14 +10774,19 @@ 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)) || - (STRNEQ_NULLABLE(disk->driverType, snap->driverType) && - !(driverType = strdup(snap->driverType))) || (persistDisk && - (!(persistSource = strdup(source)) || - (STRNEQ_NULLABLE(persistDisk->driverType, snap->driverType) && - !(persistDriverType = strdup(snap->driverType)))))) { + !(persistSource = strdup(source)))) { virReportOOMError(); goto cleanup; } @@ -10806,8 +10803,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origsrc = disk->src; disk->src = source; - origdriver = disk->driverType; - disk->driverType = (char *) "raw"; /* Don't want to probe backing files */ + origdriver = disk->format; + disk->format = VIR_STORAGE_FILE_RAW; /* Don't want to probe backing files */ if (virDomainLockDiskAttach(driver->lockManager, driver->uri, vm, disk) < 0) @@ -10828,8 +10825,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, disk->src = origsrc; origsrc = NULL; - disk->driverType = origdriver; - origdriver = NULL; + disk->format = origdriver; /* create the actual snapshot */ ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source, @@ -10843,34 +10839,24 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, VIR_FREE(disk->src); disk->src = source; source = NULL; - if (driverType) { - VIR_FREE(disk->driverType); - disk->driverType = driverType; - driverType = NULL; - } + disk->format = format; if (persistDisk) { VIR_FREE(persistDisk->src); persistDisk->src = persistSource; persistSource = NULL; - if (persistDriverType) { - VIR_FREE(persistDisk->driverType); - persistDisk->driverType = persistDriverType; - persistDriverType = NULL; - } + persistDisk->format = format; } cleanup: if (origsrc) { disk->src = origsrc; - disk->driverType = origdriver; + disk->format = origdriver; } if (need_unlink && unlink(source)) VIR_WARN("unable to unlink just-created %s", source); VIR_FREE(device); VIR_FREE(source); - VIR_FREE(driverType); VIR_FREE(persistSource); - VIR_FREE(persistDriverType); return ret; } @@ -10887,17 +10873,12 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, bool need_unlink) { char *source = NULL; - char *driverType = NULL; char *persistSource = NULL; - char *persistDriverType = NULL; struct stat st; if (!(source = strdup(origdisk->src)) || - (origdisk->driverType && - !(driverType = strdup(origdisk->driverType))) || (persistDisk && - (!(persistSource = strdup(source)) || - (driverType && !(persistDriverType = strdup(driverType)))))) { + !(persistSource = strdup(source)))) { virReportOOMError(); goto cleanup; } @@ -10917,27 +10898,17 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, VIR_FREE(disk->src); disk->src = source; source = NULL; - VIR_FREE(disk->driverType); - if (driverType) { - disk->driverType = driverType; - driverType = NULL; - } + disk->format = origdisk->format; if (persistDisk) { VIR_FREE(persistDisk->src); persistDisk->src = persistSource; persistSource = NULL; - VIR_FREE(persistDisk->driverType); - if (persistDriverType) { - persistDisk->driverType = persistDriverType; - persistDriverType = NULL; - } + persistDisk->format = origdisk->format; } cleanup: VIR_FREE(source); - VIR_FREE(driverType); VIR_FREE(persistSource); - VIR_FREE(persistDriverType); } /* The domain is expected to be locked and active. */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 25540bd..ca441f2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -45,6 +45,7 @@ #include "virnetdevbridge.h" #include "virnetdevtap.h" #include "device_conf.h" +#include "storage_file.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -107,10 +108,10 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, if (disk->src) { const char *format = NULL; if (disk->type != VIR_DOMAIN_DISK_TYPE_DIR) { - if (disk->driverType) - format = disk->driverType; - else if (origdisk->driverType) - format = origdisk->driverType; + if (disk->format > 0) + format = virStorageFileFormatTypeToString(disk->format); + else if (origdisk->format > 0) + format = virStorageFileFormatTypeToString(origdisk->format); } ret = qemuMonitorChangeMedia(priv->mon, driveAlias, diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 0eeac85..32a903e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3846,7 +3846,8 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) VIR_DEBUG("disk(%d) src: %s", i, def->disks[i]->src); VIR_DEBUG("disk(%d) dst: %s", i, def->disks[i]->dst); VIR_DEBUG("disk(%d) driverName: %s", i, def->disks[i]->driverName); - VIR_DEBUG("disk(%d) driverType: %s", i, def->disks[i]->driverType); + VIR_DEBUG("disk(%d) driverType: %s", i, + virStorageFileFormatTypeToString(def->disks[i]->format)); VIR_DEBUG("disk(%d) cachemode: %d", i, def->disks[i]->cachemode); VIR_DEBUG("disk(%d) readonly: %s", i, (def->disks[i]->readonly ? "True" : "False")); @@ -4125,7 +4126,8 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) VIR_DEBUG("disk(%d) src: %s", i, def->disks[i]->src); VIR_DEBUG("disk(%d) dst: %s", i, def->disks[i]->dst); VIR_DEBUG("disk(%d) driverName: %s", i, def->disks[i]->driverName); - VIR_DEBUG("disk(%d) driverType: %s", i, def->disks[i]->driverType); + VIR_DEBUG("disk(%d) driverType: %s", i, + virStorageFileFormatTypeToString(def->disks[i]->format)); VIR_DEBUG("disk(%d) cachemode: %d", i, def->disks[i]->cachemode); VIR_DEBUG("disk(%d) readonly: %s", i, (def->disks[i]->readonly ? "True" : "False")); diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 38a7e13..35ad496 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -36,6 +36,7 @@ #include "count-one-bits.h" #include "xenxs_private.h" #include "xen_sxpr.h" +#include "storage_file.h" /* Get a domain id from a S-expression string */ int xenGetDomIdFromSxprString(const char *sexpr, int xendConfigVersion) @@ -427,6 +428,8 @@ xenParseSxprDisks(virDomainDefPtr def, if (STREQ (disk->driverName, "tap") || STREQ (disk->driverName, "tap2")) { + char *driverType = NULL; + offset = strchr(src, ':'); if (!offset) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -434,21 +437,19 @@ xenParseSxprDisks(virDomainDefPtr def, goto error; } - if (VIR_ALLOC_N(disk->driverType, (offset-src)+1)< 0) + if (!(driverType = strndup(src, offset - src))) goto no_memory; - if (virStrncpy(disk->driverType, src, offset-src, - (offset-src)+1) == NULL) { + if (STREQ(driverType, "aio")) + disk->format = VIR_STORAGE_FILE_RAW; + else + disk->format = + virStorageFileFormatTypeFromString(driverType); + VIR_FREE(driverType); + if (disk->format <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Driver type %s too big for destination"), - src); + _("Unknown driver type %s"), src); goto error; } - if (STREQ(disk->driverType, "aio")) { - /* In-place conversion to "raw" */ - disk->driverType[0] = 'r'; - disk->driverType[1] = 'a'; - disk->driverType[2] = 'w'; - } src = offset + 1; /* Its possible to use blktap driver for block devs @@ -1837,9 +1838,12 @@ xenFormatSxprDisk(virDomainDiskDefPtr def, if (def->driverName) { if (STREQ(def->driverName, "tap") || STREQ(def->driverName, "tap2")) { - const char *type = def->driverType ? def->driverType : "aio"; - if (STREQ(type, "raw")) + const char *type; + + if (!def->format || def->format == VIR_STORAGE_FILE_RAW) type = "aio"; + else + type = virStorageFileFormatTypeToString(def->format); virBufferEscapeSexpr(buf, "(uname '%s:", def->driverName); virBufferEscapeSexpr(buf, "%s:", type); virBufferEscapeSexpr(buf, "%s')", def->src); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index c564d35..2e560a6 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -37,6 +37,7 @@ #include "xen_xm.h" #include "xen_sxpr.h" #include "domain_conf.h" +#include "storage_file.h" /* Convenience method to grab a long int from the config file object */ static int xenXMConfigGetBool(virConfPtr conf, @@ -554,24 +555,25 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, /* And the sub-type for tap:XXX: type */ if (disk->driverName && STREQ(disk->driverName, "tap")) { + char *driverType; + if (!(tmp = strchr(disk->src, ':'))) goto skipdisk; - if (VIR_ALLOC_N(disk->driverType, (tmp - disk->src) + 1) < 0) + + if (!(driverType = strndup(disk->src, tmp - disk->src))) goto no_memory; - if (virStrncpy(disk->driverType, disk->src, - (tmp - disk->src), - (tmp - disk->src) + 1) == NULL) { + if (STREQ(driverType, "aio")) + disk->format = VIR_STORAGE_FILE_RAW; + else + disk->format = + virStorageFileFormatTypeFromString(driverType); + VIR_FREE(driverType); + if (disk->format <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Driver type %s too big for destination"), + _("Unknown driver type %s"), disk->src); goto cleanup; } - if (STREQ(disk->driverType, "aio")) { - /* In-place conversion to "raw" */ - disk->driverType[0] = 'r'; - disk->driverType[1] = 'a'; - disk->driverType[2] = 'w'; - } /* Strip the prefix we found off the source file name */ memmove(disk->src, disk->src+(tmp-disk->src)+1, @@ -1208,10 +1210,13 @@ static int xenFormatXMDisk(virConfValuePtr list, virConfValuePtr val, tmp; if(disk->src) { - if (disk->driverName) { - const char *type = disk->driverType ? disk->driverType : "aio"; - if (STREQ(type, "raw")) + if (disk->format) { + const char *type; + + if (disk->format == VIR_STORAGE_FILE_RAW) type = "aio"; + else + type = virStorageFileFormatTypeToString(disk->format); virBufferAsprintf(&buf, "%s:", disk->driverName); if (STREQ(disk->driverName, "tap")) virBufferAsprintf(&buf, "%s:", type); -- 1.7.11.7

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 -- 1.7.11.7

On 10/17/2012 06:30 PM, Eric Blake wrote: ACK.

On 10/17/2012 06:30 PM, Eric Blake wrote: Previous ACK stands.

On 10/17/2012 06:30 PM, Eric Blake wrote: Previous ACK stands.

82507838 refactured the code to keep both the raw and canonicalized form of the backingStore, which breaks badly when the storage pool contains a storage volume, which is missing its backing store file: # ./daemon/libvirtd -l 2012-11-07 12:43:33.279+0000: 22175: info : libvirt version: 1.0.0 2012-11-07 12:43:33.279+0000: 22175: error : absolutePathFromBaseFile:542 : Can't canonicalize path '/var/lib/libvirt/images/base.qcow2': No such file or directory 2012-11-07 12:43:33.280+0000: 22175: error : storageDriverAutostart:115 : Failed to autostart storage pool 'default': Can't canonicalize path '/var/lib/libvirt/images/base.qcow2': No such file or directory This is because virStorageFileGetMetadataFromBuf() aborts with -1 if the filename of the backingStore can not be canonicalized: #0 absolutePathFromBaseFile () at util/storage_file.c:541 #1 virStorageFileGetMetadataFromBuf () at util/storage_file.c:728 #2 virStorageFileGetMetadataFromFD () at util/storage_file.c:932 #3 virStorageBackendProbeTarget () at storage/storage_backend_fs.c:94 #4 virStorageBackendFileSystemRefresh () at storage/storage_backend_fs.c:849 #5 storagePoolStart () at storage/storage_driver.c:700 #6 virStoragePoolCreate () at libvirt.c:12471 ... Treat files which miss their backing file as standalone files. Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- src/util/storage_file.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index e9771d7..2249212 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -727,8 +727,9 @@ virStorageFileGetMetadataFromBuf(int format, meta->backingStoreRaw = meta->backingStore; meta->backingStore = absolutePathFromBaseFile(path, backing); if (meta->backingStore == NULL) { - VIR_FREE(backing); - return -1; + /* the backing file is (currently) unavailable, treat this + * file as standalone */ + backingFormat = VIR_STORAGE_FILE_NONE; } } VIR_FREE(backing); -- 1.7.1

In order to temporarily label files read/write during a commit operation, we need to crawl the backing chain and find the absolute file name that needs labeling in the first place, as well as the name of the file that owns the backing file. * src/util/storage_file.c (virStorageFileChainLookup): New function. * src/util/storage_file.h: Declare it. * src/libvirt_private.syms (storage_file.h): Export it. --- v3: fix -Wshadow failure, improve comments src/libvirt_private.syms | 1 + src/util/storage_file.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/storage_file.h | 7 ++++++ 3 files changed, 72 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6155738..ccee23d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1130,6 +1130,7 @@ virStorageGenerateQcowPassphrase; # storage_file.h +virStorageFileChainLookup; virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; virStorageFileFreeMetadata; diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 2ff444c..882df6e 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1262,3 +1262,67 @@ const char *virStorageFileGetSCSIKey(const char *path) return NULL; } #endif + +/* Given a CHAIN that starts at the named file START, return a string + * pointing to either START or within CHAIN that gives the preferred + * name for the backing file NAME within that chain. Pass NULL for + * NAME to find the base of the chain. If META is not NULL, set *META + * to the point in the chain that describes NAME (or to NULL if the + * backing element is not a file). If PARENT is not NULL, set *PARENT + * to the preferred name of the parent (or to NULL if NAME matches + * START). Since the results point within CHAIN, they must not be + * independently freed. */ +const char * +virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, + const char *name, virStorageFileMetadataPtr *meta, + const char **parent) +{ + virStorageFileMetadataPtr owner; + const char *tmp; + + if (!parent) + parent = &tmp; + + *parent = NULL; + if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) : + !chain->backingStore) { + if (meta) + *meta = chain; + return start; + } + + owner = chain; + *parent = start; + while (owner) { + if (!owner->backingStore) + goto error; + if (!name) { + if (!owner->backingMeta || + !owner->backingMeta->backingStore) + break; + } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) || + STREQ(name, owner->backingStore)) { + break; + } else if (owner->backingStoreIsFile) { + char *absName = absolutePathFromBaseFile(*parent, name); + if (absName && STREQ(absName, owner->backingStore)) { + VIR_FREE(absName); + break; + } + VIR_FREE(absName); + } + *parent = owner->backingStore; + owner = owner->backingMeta; + } + if (!owner) + goto error; + if (meta) + *meta = owner->backingMeta; + return owner->backingStore; + +error: + *parent = NULL; + if (meta) + *meta = NULL; + return NULL; +} diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 685fcb8..9b00dba 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -78,6 +78,13 @@ virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format); +const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, + const char *start, + const char *name, + virStorageFileMetadataPtr *meta, + const char **parent) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta); int virStorageFileResize(const char *path, unsigned long long capacity); -- 1.7.11.7

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. --- v3: drop useless processStop cleanup, s/chain/backingChain/, don't make force discard errors src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 2 ++ src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ src/qemu/qemu_process.c | 8 ++++++++ 6 files changed, 54 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a360c25..6487e6b 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->backingChain); 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 eec1f25..e7f0e9c 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 backingChain; char *mirror; int mirrorFormat; /* enum virStorageFileFormat */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b51edc2..45f3a5e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2010,3 +2010,29 @@ qemuDomainCleanupRun(struct qemud_driver *driver, priv->ncleanupCallbacks = 0; priv->ncleanupCallbacks_max = 0; } + +int +qemuDomainDetermineDiskChain(struct qemud_driver *driver, + virDomainDiskDefPtr disk, + bool force) +{ + bool probe = driver->allowDiskFormatProbing; + + if (!disk->src) + return 0; + + if (disk->backingChain) { + if (force) { + virStorageFileFreeMetadata(disk->backingChain); + disk->backingChain = NULL; + } else { + return 0; + } + } + disk->backingChain = virStorageFileGetMetadata(disk->src, disk->format, + driver->user, driver->group, + probe); + if (!disk->backingChain) + return -1; + return 0; +} 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 7f240a0..dfbf04d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5816,6 +5816,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) + goto end; + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6044,6 +6047,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) { @@ -10788,6 +10794,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->backingChain 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. */ + virStorageFileFreeMetadata(disk->backingChain); + disk->backingChain = 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 e08ec67..0e98c8b 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); -- 1.7.11.7

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. * 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. --- v3: rebase to earlier changes, fix comment 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 | 20 ++++---- 6 files changed, 32 insertions(+), 120 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6487e6b..afa4cfe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14767,110 +14767,42 @@ done: } +/* Call iter(disk, name, depth, opaque) for each element of disk and + * its backing chain in the pre-populated disk->backingChain. + * 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->backingChain; + 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; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7f0e9c..10ef841 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2141,9 +2141,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 6b94686..428befd 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 9dbf95d..a67f5d6 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 b5108ab..eee8d71 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1062,13 +1062,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; @@ -1078,16 +1075,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..263fc92 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -919,20 +919,22 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->ndisks; i++) { + virDomainDiskDefPtr disk = ctl->def->disks[i]; + + /* 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, disk->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); - if (ret != 0) + if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0) goto clean; } -- 1.7.11.7

Minor cleanup made possible by previous simplifications. * src/qemu/qemu_cgroup.h (qemuSetupDiskCgroup) (qemuTeardownDiskCgroup): Alter signature. * src/qemu/qemu_cgroup.c (qemuSetupDiskCgroup) (qemuTeardownDiskCgroup, qemuSetupCgroup): Update all uses. * src/qemu/qemu_hotplug.c (qemuDomainDetachPciDiskDevice) (qemuDomainDetachDiskDevice): Likewise. * src/qemu/qemu_driver.c (qemuDomainAttachDeviceDiskLive) (qemuDomainChangeDiskMediaLive) (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotUndoSingleDiskActive): Likewise. --- v3: new patch src/qemu/qemu_cgroup.c | 8 +++----- src/qemu/qemu_cgroup.h | 6 ++---- src/qemu/qemu_driver.c | 14 +++++++------- src/qemu/qemu_hotplug.c | 4 ++-- 4 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 428befd..db371a0 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -87,8 +87,7 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, } -int qemuSetupDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, - virDomainObjPtr vm, +int qemuSetupDiskCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { @@ -127,8 +126,7 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, } -int qemuTeardownDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, - virDomainObjPtr vm, +int qemuTeardownDiskCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { @@ -230,7 +228,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, for (i = 0; i < vm->def->ndisks ; i++) { if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], false) < 0 || - qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0) + qemuSetupDiskCgroup(vm, cgroup, vm->def->disks[i]) < 0) goto cleanup; } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 362080a..c552162 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -36,12 +36,10 @@ typedef struct _qemuCgroupData qemuCgroupData; bool qemuCgroupControllerActive(struct qemud_driver *driver, int controller); -int qemuSetupDiskCgroup(struct qemud_driver *driver, - virDomainObjPtr vm, +int qemuSetupDiskCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk); -int qemuTeardownDiskCgroup(struct qemud_driver *driver, - virDomainObjPtr vm, +int qemuTeardownDiskCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk); int qemuSetupHostUsbDeviceCgroup(usbDevice *dev, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfbf04d..d5c15ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5826,7 +5826,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, vm->def->name); goto end; } - if (qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) + if (qemuSetupDiskCgroup(vm, cgroup, disk) < 0) goto end; } switch (disk->device) { @@ -5862,7 +5862,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, } if (ret != 0 && cgroup) { - if (qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) + if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } @@ -6058,7 +6058,7 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, vm->def->name); goto end; } - if (qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) + if (qemuSetupDiskCgroup(vm, cgroup, disk) < 0) goto end; } @@ -6077,7 +6077,7 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, } if (ret != 0 && cgroup) { - if (qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) + if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(disk->src)); } @@ -10806,14 +10806,14 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, if (virDomainLockDiskAttach(driver->lockManager, driver->uri, vm, disk) < 0) goto cleanup; - if (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) { + if (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", source); goto cleanup; } if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, disk) < 0) { - if (cgroup && qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) + if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", source); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", source); @@ -10885,7 +10885,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, 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) + if (cgroup && qemuTeardownDiskCgroup(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); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ca441f2..7381921 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2006,7 +2006,7 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); if (cgroup != NULL) { - if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } @@ -2089,7 +2089,7 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); if (cgroup != NULL) { - if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } -- 1.7.11.7

qemu 1.3 will be adding a 'block-commit' monitor command, per qemu.git commit ed61fc1. It matches nicely to the libvirt API virDomainBlockCommit. * 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. --- v3: use long long for JSON speed argument 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 4313451..2d9c44c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2796,6 +2796,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 f33b94c..8856d9f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -524,6 +524,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 2daf8ea..d1ce9cc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -805,6 +805,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: @@ -3278,6 +3280,36 @@ cleanup: return ret; } +/* speed is in bytes/sec */ +int +qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, + const char *top, const char *base, + unsigned long long speed) +{ + 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, @@ -3394,6 +3426,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..61127a7 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 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 0e98c8b..3a087e2 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); } -- 1.7.11.7

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. --- v3: mention release 1.0.0 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 d5c15ef..c63d16b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12651,6 +12651,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, @@ -13993,6 +14067,7 @@ static virDriver qemuDriver = { .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ + .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */ .isAlive = qemuIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */ -- 1.7.11.7

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. --- v3: rebase to changes earlier in series src/libvirt.c | 2 -- src/qemu/qemu_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index e3ddf27..6d65965 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19381,8 +19381,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 c63d16b..73804fe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12664,8 +12664,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; @@ -12696,20 +12700,55 @@ 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->backingChain; + } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain, + 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; + } + /* Note that this code exploits the fact that + * virStorageFileChainLookup guarantees a simple pointer + * comparison will work, rather than needing full-blown STREQ. */ + 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: -- 1.7.11.7

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. * src/qemu/qemu_driver.c (qemuDomainPrepareDiskChainElement): New function. (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotUndoSingleDiskActive): Use it. --- v3: rebase to earlier patches in series src/qemu/qemu_driver.c | 101 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73804fe..072ec17 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10453,6 +10453,66 @@ cleanup: return ret; } +typedef enum { + VIR_DISK_CHAIN_NO_ACCESS, + VIR_DISK_CHAIN_READ_ONLY, + VIR_DISK_CHAIN_READ_WRITE, +} qemuDomainDiskChainMode; + +/* Several operations end up adding or removing a single element of a + * disk backing file chain; this helper function ensures that the lock + * manager, cgroup device controller, and security manager labelling + * are all aware of each new file before it is added to a chain, and + * can revoke access to a file no longer needed in a chain. */ +static int +qemuDomainPrepareDiskChainElement(struct qemud_driver *driver, + virDomainObjPtr vm, + virCgroupPtr cgroup, + virDomainDiskDefPtr disk, + char *file, + qemuDomainDiskChainMode mode) +{ + /* The easiest way to label a single file with the same + * permissions it would have as if part of the disk chain is to + * temporarily modify the disk in place. */ + char *origsrc = disk->src; + int origformat = disk->format; + virStorageFileMetadataPtr origchain = disk->backingChain; + bool origreadonly = disk->readonly; + int ret = -1; + + disk->src = file; + disk->format = VIR_STORAGE_FILE_RAW; + disk->backingChain = NULL; + disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; + + 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(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); + } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0 || + (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) || + virSecurityManagerSetImageLabel(driver->securityManager, + vm->def, disk) < 0) { + goto cleanup; + } + + ret = 0; + +cleanup: + disk->src = origsrc; + disk->format = origformat; + disk->backingChain = origchain; + disk->readonly = origreadonly; + return ret; +} + + static bool qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { @@ -10762,8 +10822,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, char *persistSource = NULL; int ret = -1; int fd = -1; - char *origsrc = NULL; - int origdriver; bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { @@ -10790,10 +10848,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, VIR_FORCE_CLOSE(fd); } - origsrc = disk->src; - 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->backingChain if * successful, so we nuke the existing chain so that future * commands will recompute it. Better would be storing the chain @@ -10803,27 +10857,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virStorageFileFreeMetadata(disk->backingChain); disk->backingChain = NULL; - if (virDomainLockDiskAttach(driver->lockManager, driver->uri, - vm, disk) < 0) - goto cleanup; - if (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) { - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", source); - goto cleanup; - } - if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, - disk) < 0) { - if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", source); - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", source); + if (qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source, + VIR_DISK_CHAIN_READ_WRITE) < 0) { + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source, + VIR_DISK_CHAIN_NO_ACCESS); goto cleanup; } - disk->src = origsrc; - origsrc = NULL; - disk->format = origdriver; - /* create the actual snapshot */ if (snap->format) formatStr = virStorageFileFormatTypeToString(snap->format); @@ -10847,10 +10887,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, } cleanup: - if (origsrc) { - disk->src = origsrc; - disk->format = origdriver; - } if (need_unlink && unlink(source)) VIR_WARN("unable to unlink just-created %s", source); VIR_FREE(device); @@ -10882,13 +10918,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(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); if (need_unlink && stat(disk->src, &st) == 0 && S_ISREG(st.st_mode) && unlink(disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src); -- 1.7.11.7

On 10/17/2012 06:30 PM, Eric Blake wrote: Umm. I don't know that I agree with your statement about the relative importance of undoing all the labelling when you're done, but if the alternative to this is to require turning off selinux, then it's definitely a win. ... since you're misusing the diskdef anyway :-) It looks to me like everything in cgroup.c returns < 0 ( -errno in most, if not all cases). Any idea why all its callers are testing for != 0 instead of the more standard < 0 ? (you're just following the convention, so no complaints on your patch, but it looks a bit odd.) Is it always the case that it was previously read-only, or is this just a naive assumption? ACK.

On 10/17/2012 06:30 PM, Eric Blake wrote: I *think* I recognized everything in the interdiff from my comments, or else they made sense. So ACK to the interdiffs, and I guess if Doug has already ACKed the v2 of PATCH 01-07, then this should extend those ACKs up to v3 (does that make sense? At any rate, you have my explicit ACKs on path 08-19, and on the v2-v3 differences to patch 01-08.)