Re: [libvirt] [PATCH] qemu: Don't chown files on NFS share if dynamic_ownership is off
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. >>> >> Sure, https://bugzilla.redhat.com/show_bug.cgi?id=716478 > > > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't chown files on NFS share if dynamic_ownership is off
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. Sure, https://bugzilla.redhat.com/show_bug.cgi?id=716478 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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't chown files on NFS share if dynamic_ownership is off
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't chown files on NFS share if dynamic_ownership is off
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 ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list