[libvirt] [PATCH] qemu: Don't chown files on NFS share if dynamic_ownership is off
When dynamic ownership is disabled we don't want to chown any files, not just local. --- src/qemu/qemu_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52b7dfd..968865f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2163,11 +2163,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is_reg = true; } else { is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular local file which exists + /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg && !driver->dynamicOwnership && - virStorageFileIsSharedFS(path) == 0) { + if (is_reg && !driver->dynamicOwnership) { uid=sb.st_uid; gid=sb.st_gid; } -- 1.7.5.rc3
On 07/07/2011 09:33 AM, Michal Privoznik wrote:
When dynamic ownership is disabled we don't want to chown any files, not just local.
Is there more details on a scenario where this was causing an issue? Either a BZ number or a set of steps to reproduce the problem.
--- src/qemu/qemu_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52b7dfd..968865f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2163,11 +2163,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is_reg = true; } else { is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular local file which exists + /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg && !driver->dynamicOwnership && - virStorageFileIsSharedFS(path) == 0) { + if (is_reg && !driver->dynamicOwnership) {
The code change looks fine, but without a pointer to a reproducer case proving that it is a bug fix, I'm not sure if this would have unintended consequences. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07.07.2011 17:52, Eric Blake wrote:
On 07/07/2011 09:33 AM, Michal Privoznik wrote:
When dynamic ownership is disabled we don't want to chown any files, not just local.
Is there more details on a scenario where this was causing an issue? Either a BZ number or a set of steps to reproduce the problem.
--- src/qemu/qemu_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52b7dfd..968865f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2163,11 +2163,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is_reg = true; } else { is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular local file which exists + /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg && !driver->dynamicOwnership && - virStorageFileIsSharedFS(path) == 0) { + if (is_reg && !driver->dynamicOwnership) {
The code change looks fine, but without a pointer to a reproducer case proving that it is a bug fix, I'm not sure if this would have unintended consequences.
Sure, https://bugzilla.redhat.com/show_bug.cgi?id=716478 Michal
On 07/07/2011 12:00 PM, Michal Privoznik wrote:
On 07.07.2011 17:52, Eric Blake wrote:
On 07/07/2011 09:33 AM, Michal Privoznik wrote:
When dynamic ownership is disabled we don't want to chown any files, not just local. Is there more details on a scenario where this was causing an issue? Either a BZ number or a set of steps to reproduce the problem.
--- src/qemu/qemu_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52b7dfd..968865f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2163,11 +2163,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is_reg = true; } else { is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular local file which exists + /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg&& !driver->dynamicOwnership&& - virStorageFileIsSharedFS(path) == 0) { + if (is_reg&& !driver->dynamicOwnership) { The code change looks fine, but without a pointer to a reproducer case proving that it is a bug fix, I'm not sure if this would have unintended consequences.
And I can't think of any reason why it *should* check for local (if anything, we should do *less* changing of ownership on remote filesystems, not more). Oh, and this is fairly recent code, so there won't be anybody relying on the old behavior. So ACK.
On 07.07.2011 20:12, Laine Stump wrote:
On 07/07/2011 12:00 PM, Michal Privoznik wrote:
On 07.07.2011 17:52, Eric Blake wrote:
On 07/07/2011 09:33 AM, Michal Privoznik wrote:
When dynamic ownership is disabled we don't want to chown any files, not just local. Is there more details on a scenario where this was causing an issue? Either a BZ number or a set of steps to reproduce the problem.
--- src/qemu/qemu_driver.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52b7dfd..968865f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2163,11 +2163,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is_reg = true; } else { is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular local file which exists + /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg&& !driver->dynamicOwnership&& - virStorageFileIsSharedFS(path) == 0) { + if (is_reg&& !driver->dynamicOwnership) { The code change looks fine, but without a pointer to a reproducer case proving that it is a bug fix, I'm not sure if this would have unintended consequences.
And I can't think of any reason why it *should* check for local (if anything, we should do *less* changing of ownership on remote filesystems, not more). Oh, and this is fairly recent code, so there won't be anybody relying on the old behavior. So ACK.
Thanks, pushed. Michal
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik