Re: [libvirt] [PATCH] qemu: Don't change ownership of file when appending to it

On 05/24/2011 09:12 AM, Daniel Veillard wrote: > On Tue, May 24, 2011 at 02:54:28PM +0200, Michal Privoznik wrote: >> Saving domain to previously created file changes also its ownership. >> This is certainly not what users want if some conditions are met: >> it is a regular, local file and dynamic_ownership is off. >> --- >> src/qemu/qemu_driver.c | 12 +++++++++++- >> 1 files changed, 11 insertions(+), 1 deletions(-) >> >> @@ -2013,6 +2015,14 @@ 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 >> + * already and dynamic_ownership is off, we don't >> + * want to change it's ownership, just append the data */ >> + if (is_reg && !driver->dynamicOwnership && >> + virStorageFileIsSharedFS(path) == 0) { >> + uid=sb.st_uid; >> + gid=sb.st_gid; The comment is misleading - we aren't using O_APPEND, but O_TRUNC (that is, we are keeping the same inode and file, but rewriting its entire contents, rather than appending to existing contents). How about: s/just append the data/just open it as-is/ > The explaination sounds fine, and patch seems to implement just this, > > ACK, > > but maybe give a 24 hours grace period for others to review it too, as > I'm not 100% sure :-) You've also got my ACK with the comment tweak.