4:34 p.m.
Fixing easy problem (patch 1) lead me to finding out that there is yet
another problem that needs fixing (patch 2). For more information
read the code.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Martin Kletzander (2):
qemu: Don't mangle the storage format for type='dir'
qemu: Also treat directories properly when using namespaces
src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++++++++++++++-----
src/storage/storage_source.c | 12 +++++++++---
tests/virstoragetest.c | 11 +++++++++--
3 files changed, 53 insertions(+), 10 deletions(-)
--
2.14.1
4:34 p.m.
New subject: [libvirt] [PATCH v2 1/2] qemu: Don't mangle the storage format for type='dir'
Our backing probing code handles directory file types properly in
virStorageFileGetMetadataRecurse(), by that I mean it leaves them
alone. However its caller, the virStorageFileGetMetadata() resets the
type to raw before probing, without even checking the type. We need
to special-case TYPE_DIR in order to achieve desired results.
Also, in order to properly test this, we need to stop resetting format
of volumes in tests for TYPE_DIR (probably the reason why we didn't
catch that and why the test data didn't need to be modified).
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/storage/storage_source.c | 12 +++++++++---
tests/virstoragetest.c | 11 +++++++++--
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c
index b620153f1e5a..bf47622372ab 100644
--- a/src/storage/storage_source.c
+++ b/src/storage/storage_source.c
@@ -527,14 +527,20 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
allow_probe, report_broken);
virHashTablePtr cycle = NULL;
+ virStorageType actualType = virStorageSourceGetActualType(src);
int ret = -1;
if (!(cycle = virHashCreate(5, NULL)))
return -1;
- if (src->format <= VIR_STORAGE_FILE_NONE)
- src->format = allow_probe ?
- VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
+ if (src->format <= VIR_STORAGE_FILE_NONE) {
+ if (actualType == VIR_STORAGE_TYPE_DIR)
+ src->format = VIR_STORAGE_FILE_DIR;
+ else if (allow_probe)
+ src->format = VIR_STORAGE_FILE_AUTO;
+ else
+ src->format = VIR_STORAGE_FILE_RAW;
+ }
ret = virStorageFileGetMetadataRecurse(src, src, uid, gid,
allow_probe, report_broken, cycle);
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index d83db78f566f..60e3164b0ac8 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -111,7 +111,6 @@ testStorageFileGetMetadata(const char *path,
if (stat(path, &st) == 0) {
if (S_ISDIR(st.st_mode)) {
ret->type = VIR_STORAGE_TYPE_DIR;
- ret->format = VIR_STORAGE_FILE_DIR;
} else if (S_ISBLK(st.st_mode)) {
ret->type = VIR_STORAGE_TYPE_BLOCK;
}
@@ -963,7 +962,15 @@ mymain(void)
.type = VIR_STORAGE_TYPE_DIR,
.format = VIR_STORAGE_FILE_DIR,
};
- TEST_CHAIN(absdir, VIR_STORAGE_FILE_AUTO,
+ testFileData dir_as_raw = {
+ .path = canondir,
+ .type = VIR_STORAGE_TYPE_DIR,
+ .format = VIR_STORAGE_FILE_RAW,
+ };
+ TEST_CHAIN(absdir, VIR_STORAGE_FILE_RAW,
+ (&dir_as_raw), EXP_PASS,
+ (&dir_as_raw), ALLOW_PROBE | EXP_PASS);
+ TEST_CHAIN(absdir, VIR_STORAGE_FILE_NONE,
(&dir), EXP_PASS,
(&dir), ALLOW_PROBE | EXP_PASS);
TEST_CHAIN(absdir, VIR_STORAGE_FILE_DIR,
--
2.14.1
4:34 p.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Also treat directories properly when using namespaces
When recreating folders with namespaces, the directory type was not
being handled at all. It's not special, we probably just didn't know
that that can be used as a volume path as well. The code failed
gracefully, but we want to allow that so that we can use <disk
type='dir'> in domains again.
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c77a6442467..2549f9bf3290 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7855,6 +7855,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
bool isLink = false;
bool isDev = false;
bool isReg = false;
+ bool isDir = false;
bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
@@ -7879,6 +7880,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
isLink = S_ISLNK(sb.st_mode);
isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode);
+ isDir = S_ISDIR(sb.st_mode);
/* Here, @device might be whatever path in the system. We
* should create the path in the namespace iff it's "/dev"
@@ -7996,6 +7998,10 @@ qemuDomainCreateDeviceRecursive(const char *device,
goto cleanup;
/* Just create the file here so that code below sets
* proper owner and mode. Bind mount only after that. */
+ } else if (isDir) {
+ if (create &&
+ virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
+ goto cleanup;
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8057,7 +8063,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
#endif
/* Finish mount process started earlier. */
- if (isReg &&
+ if ((isReg || isDir) &&
virFileBindMountDevice(device, devicePath) < 0)
goto cleanup;
@@ -8686,6 +8692,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
bool isLink = S_ISLNK(data->sb.st_mode);
bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
+ bool isDir = S_ISDIR(data->sb.st_mode);
qemuSecurityPostFork(data->driver->securityManager);
@@ -8741,6 +8748,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
delDevice = true;
/* Just create the file here so that code below sets
* proper owner and mode. Move the mount only after that. */
+ } else if (isDir) {
+ /* We are not cleaning up disks on virDomainDetachDevice
+ * because disk might be still in use by different disk
+ * as its backing chain. This might however clash here.
+ * Therefore do the cleanup here. */
+ if (umount(data->file) < 0 &&
+ errno != ENOENT && errno != EINVAL) {
+ virReportSystemError(errno,
+ _("Unable to umount %s"),
+ data->file);
+ goto cleanup;
+ }
+ if (virFileMakePathWithMode(data->file, data->sb.st_mode) < 0)
+ goto cleanup;
+ delDevice = true;
+ /* Just create the folder here so that code below sets
+ * proper owner and mode. Move the mount only after that. */
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8788,14 +8812,18 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
# endif
/* Finish mount process started earlier. */
- if (isReg &&
+ if ((isReg || isDir) &&
virFileMoveMount(data->target, data->file) < 0)
goto cleanup;
ret = 0;
cleanup:
- if (ret < 0 && delDevice)
- unlink(data->file);
+ if (ret < 0 && delDevice) {
+ if (isDir)
+ virFileDeleteTree(data->file);
+ else
+ unlink(data->file);
+ }
# ifdef WITH_SELINUX
freecon(data->tcon);
# endif
@@ -8818,6 +8846,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
char *target = NULL;
bool isLink;
bool isReg;
+ bool isDir;
if (!ttl) {
virReportSystemError(ELOOP,
@@ -8840,8 +8869,9 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
isLink = S_ISLNK(data.sb.st_mode);
isReg = S_ISREG(data.sb.st_mode) || S_ISFIFO(data.sb.st_mode) ||
S_ISSOCK(data.sb.st_mode);
+ isDir = S_ISDIR(data.sb.st_mode);
- if (isReg && STRPREFIX(file, DEVPREFIX)) {
+ if ((isReg || isDir) && STRPREFIX(file, DEVPREFIX)) {
cfg = virQEMUDriverGetConfig(driver);
if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file)))
goto cleanup;
--
2.14.1
8:10 p.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: Also treat directories properly when using namespaces
On 08/29/2017 10:34 AM, Martin Kletzander wrote:
When recreating folders with namespaces, the directory type was not
being handled at all. It's not special, we probably just didn't know
that that can be used as a volume path as well. The code failed
gracefully, but we want to allow that so that we can use <disk
type='dir'> in domains again.
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c77a6442467..2549f9bf3290 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7855,6 +7855,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
bool isLink = false;
bool isDev = false;
bool isReg = false;
+ bool isDir = false;
bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
@@ -7879,6 +7880,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
isLink = S_ISLNK(sb.st_mode);
isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode);
isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode);
+ isDir = S_ISDIR(sb.st_mode);
/* Here, @device might be whatever path in the system. We
* should create the path in the namespace iff it's "/dev"
@@ -7996,6 +7998,10 @@ qemuDomainCreateDeviceRecursive(const char *device,
goto cleanup;
/* Just create the file here so that code below sets
* proper owner and mode. Bind mount only after that. */
+ } else if (isDir) {
+ if (create &&
+ virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
+ goto cleanup;
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8057,7 +8063,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
#endif
/* Finish mount process started earlier. */
- if (isReg &&
+ if ((isReg || isDir) &&
virFileBindMountDevice(device, devicePath) < 0)
goto cleanup;
@@ -8686,6 +8692,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
bool isLink = S_ISLNK(data->sb.st_mode);
bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
+ bool isDir = S_ISDIR(data->sb.st_mode);
qemuSecurityPostFork(data->driver->securityManager);
@@ -8741,6 +8748,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
delDevice = true;
/* Just create the file here so that code below sets
* proper owner and mode. Move the mount only after that. */
+ } else if (isDir) {
+ /* We are not cleaning up disks on virDomainDetachDevice
+ * because disk might be still in use by different disk
+ * as its backing chain. This might however clash here.
+ * Therefore do the cleanup here. */
+ if (umount(data->file) < 0 &&
+ errno != ENOENT && errno != EINVAL) {
+ virReportSystemError(errno,
+ _("Unable to umount %s"),
+ data->file);
+ goto cleanup;
+ }
+ if (virFileMakePathWithMode(data->file, data->sb.st_mode) < 0)
+ goto cleanup;
+ delDevice = true;
+ /* Just create the folder here so that code below sets
+ * proper owner and mode. Move the mount only after that. */
Or, you can merge this with the previous else branch since it's
practically identical. Just like you're doing in the next hunk.
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
@@ -8788,14 +8812,18 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
# endif
/* Finish mount process started earlier. */
- if (isReg &&
+ if ((isReg || isDir) &&
virFileMoveMount(data->target, data->file) < 0)
goto cleanup;
ret = 0;
cleanup:
- if (ret < 0 && delDevice)
- unlink(data->file);
+ if (ret < 0 && delDevice) {
+ if (isDir)
+ virFileDeleteTree(data->file);
+ else
+ unlink(data->file);
+ }
# ifdef WITH_SELINUX
freecon(data->tcon);
# endif
ACK series.
Michal
2626
days inactive
2626
days old
3 comments
2 participants
participants (2)
-
Martin Kletzander -
Michal Privoznik