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.

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.