Unlike create options, if the file to be removed is already in the pool, then the uid/gid will come from the pool. If it's the same as the currently running process, then just do the unlink/rmdir directly rather than going through the fork processing unnecessarily Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 2332589..3d7efdc 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2316,10 +2316,11 @@ virFileRemove(const char *path, int ngroups; /* If not running as root or if a non explicit uid/gid was being used for - * the file/volume, then use unlink directly + * the file/volume or the explicit uid/gid matches, then use unlink directly */ if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1))) { + ((uid == (uid_t) -1) && (gid == (gid_t) -1)) || + (uid == geteuid() && gid == getegid())) { if (virFileIsDir(path)) return rmdir(path); else -- 2.1.0

On 02.10.2015 15:41, John Ferlan wrote: Yep, this matches not only the rest of the function (e.g. when unlinking without forking) but the win32 impl too. Checked all the callers (ehm, so far the only one), and this behaviour is expected there too. ACK Michal

On 02.10.2015 15:41, John Ferlan wrote: Okay, this makes sense. Not only from the POV you are presenting, but I'd expect my pool to be refreshed after I create new volume in it. And I think we have a way to eliminate even the little window - just track if we successfully built the volume or not. Something among these lines: diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ddf4405..dd28f3f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1857,7 +1857,7 @@ storageVolCreateXML(virStoragePoolPtr obj, voldef->building = true; virStoragePoolObjUnlock(pool); - buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags); + buildret = backend->buildVol(obj->conn, pool, buildvoldef, &created, flags); storageDriverLock(); virStoragePoolObjLock(pool); @@ -1866,7 +1866,7 @@ storageVolCreateXML(virStoragePoolPtr obj, voldef->building = false; pool->asyncjobs--; - if (buildret < 0) { + if (buildret < 0 && created) { VIR_FREE(buildvoldef); storageVolDeleteInternal(volobj, backend, pool, voldef, 0, false); At any rate, ACK to this patch as is. Michal

On Mon, Oct 05, 2015 at 07:27:34PM -0400, John Ferlan wrote: This is a workaround for the linked bug, not a fix. Refreshing the pool seems excessive to me. Other volume APIs don't refresh the pool, I think we should be consistent. After properly propagating the error value if the volume exists, we don't need this patch refreshing the pool to work around the original bug. Jan

Create a new function virStorageBackendDiskValidLabel to handle checking whether there is a label on the device and whether it's valid or not. While initially for the purpose of determining whether the label can be overwritten during DiskBuild, a future use during DiskStart could determine whether the pool should be started using the label found. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 47 +++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index ba70a5a..da2a4d4 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -432,6 +432,33 @@ virStorageBackendDiskFindLabel(const char* device) /** + * Determine whether the label on the disk is valid or in a known format + * for the purpose of rewriting the label during build + * + * Return: True if it's OK + * False if something's wrong + */ +static bool +virStorageBackendDiskValidLabel(const char *device) +{ + bool valid = false; + int check; + + check = virStorageBackendDiskFindLabel(device); + if (check > 0) { + valid = true; + } else if (check < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Error checking for disk label")); + } else { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Disk label already present")); + } + return valid; +} + + +/** * Write a new partition table header */ static int @@ -450,23 +477,11 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, error); - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) ok_to_mklabel = true; - } else { - int check; - - check = virStorageBackendDiskFindLabel( - pool->def->source.devices[0].path); - if (check > 0) { - ok_to_mklabel = true; - } else if (check < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Error checking for disk label")); - } else { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Disk label already present")); - } - } + else + ok_to_mklabel = virStorageBackendDiskValidLabel( + pool->def->source.devices[0].path); if (ok_to_mklabel) { /* eg parted /dev/sda mklabel --script msdos */ -- 2.1.0

Let's check to ensure we can find the Partition Table in the label and that libvirt actually recognizes that type; otherwise, when we go to read the partitions during a refresh operation we may not be reading what we expect. This will expand upon the types of errors or reason that a build would fail, so we can create more direct error messages. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/storage/storage_backend_disk.c | 41 +++++++++++++++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c87efa1..415ed19 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -834,6 +834,7 @@ virStoragePoolDefParseFile; virStoragePoolDefParseNode; virStoragePoolDefParseSourceString; virStoragePoolDefParseString; +virStoragePoolFormatDiskTypeFromString; virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 6f9fab1..b66a4a1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -396,7 +396,9 @@ virStorageBackendDiskRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, * Check for a valid disk label (partition table) on device * * return: 0 - valid disk label found - * >0 - no or unrecognized disk label + * 1 - no or unrecognized disk label + * 2 - did not find the Partition Table type + * 3 - Partition Table type unknown * <0 - error finding the disk label */ static int @@ -408,6 +410,7 @@ virStorageBackendDiskFindLabel(const char* device) virCommandPtr cmd = virCommandNew(PARTED); char *output = NULL; char *error = NULL; + char *start, *end; int ret = -1; virCommandAddArgSet(cmd, args); @@ -422,15 +425,40 @@ virStorageBackendDiskFindLabel(const char* device) (error && strstr(error, "unrecognised disk label"))) { ret = 1; } + goto cleanup; + } + + /* Search for "Partition Table:" in the output. If not present, + * then we cannot validate the partition table type. + */ + if (!(start = strstr(output, "Partition Table: ")) || + !(end = strstr(start, "\n"))) { + VIR_DEBUG("Unable to find tag in output: %s", output); + ret = 2; + goto cleanup; + } + start += strlen("Partition Table: "); + *end = '\0'; + + /* on disk it's "msdos", but we document/use "dos" so deal with it here */ + if (STREQ(start, "msdos")) + start += 2; + + /* Make sure we know about this type */ + if (virStoragePoolFormatDiskTypeFromString(start) < 0) { + ret = 3; + goto cleanup; } + ret = 0; + + cleanup: virCommandFree(cmd); VIR_FREE(output); VIR_FREE(error); return ret; } - /** * Determine whether the label on the disk is valid or in a known format * for the purpose of rewriting the label during build @@ -452,12 +480,19 @@ virStorageBackendDiskValidLabel(const char *device, int check; check = virStorageBackendDiskFindLabel(device); - if (check > 0) { + if (check == 1) { if (writelabel) valid = true; else virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unrecognized disk label found, requires build")); + } else if (check == 2) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Unable to determine Partition Type, " + "requires build --overwrite")); + } else if (check == 3) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Unknown Partition Type, requires build --overwrite")); } else if (check < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Error checking for disk label, failed to get " -- 2.1.0

https://bugzilla.redhat.com/show_bug.cgi?id=1247987 Calculation of the extended and logical partition values for the disk pool is complex. As the bz points out an extended partition should have it's allocation initialized to 0 (zero) and keep the capacity as the size dictated by the extents read. Then for each logical partition found, adjust the allocation of the extended partition. Finally, previous logic tried to avoid recalculating things if a logical partition was deleted; however, since we now have special logic to handle the allocation of the extended partition, just make life easier by reading the partition table again - rather than doing the reverse adjustment. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index c6317a2..7baecc1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -156,7 +156,8 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, VIR_STORAGE_VOL_OPEN_DEFAULT | VIR_STORAGE_VOL_OPEN_NOERROR) == -1) return -1; - vol->target.allocation = vol->target.capacity = + vol->target.allocation = 0; + vol->target.capacity = (vol->source.extents[0].end - vol->source.extents[0].start); } else { if (virStorageBackendUpdateVolInfo(vol, false, @@ -164,6 +165,20 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } + /* Find the extended partition and increase the allocation value */ + if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_LOGICAL) { + size_t i; + + for (i = 0; i < pool->volumes.count; i++) { + if (pool->volumes.objs[i]->source.partType == + VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + pool->volumes.objs[i]->target.allocation += + vol->target.allocation; + break; + } + } + } + if (STRNEQ(groups[2], "metadata")) pool->def->allocation += vol->target.allocation; if (vol->source.extents[0].end > pool->def->capacity) @@ -841,17 +856,14 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn, goto cleanup; } - /* If this is not a logical partition, then either we've removed an - * extended partition or a primary partion - refresh the pool which - * includes resetting the [n]freeExtents data so a subsequent allocation - * might be able to use what was deleted. A logical partition is part - * of an extended partition and handled differently + /* Refreshing the pool is the easiest option as LOGICAL and EXTENDED + * partition allocation/capacity management is handled within + * virStorageBackendDiskMakeDataVol and trying to redo that logic + * here is pointless */ - if (vol->source.partType != VIR_STORAGE_VOL_DISK_TYPE_LOGICAL) { - virStoragePoolObjClearVols(pool); - if (virStorageBackendDiskRefreshPool(conn, pool) < 0) - goto cleanup; - } + virStoragePoolObjClearVols(pool); + if (virStorageBackendDiskRefreshPool(conn, pool) < 0) + goto cleanup; rc = 0; cleanup: -- 2.1.0