[libvirt] [PATCH] qemu: Don't change ownership of file when appending to it
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(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db299a2..4538cf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1963,6 +1963,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, bool is_reg = false; unsigned long long offset; int fd = -1; + uid_t uid = getuid(); + gid_t gid = getgid(); memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -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; + } } offset = sizeof(header) + header.xml_len; @@ -2048,7 +2058,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, } else { if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR, - getuid(), getgid(), 0)) < 0) { + uid, gid, 0)) < 0) { /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the -- 1.7.5.rc3
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(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db299a2..4538cf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1963,6 +1963,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, bool is_reg = false; unsigned long long offset; int fd = -1; + uid_t uid = getuid(); + gid_t gid = getgid();
memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -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; + } }
offset = sizeof(header) + header.xml_len; @@ -2048,7 +2058,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, } else { if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR, - getuid(), getgid(), 0)) < 0) { + uid, gid, 0)) < 0) { /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the
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 :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 24.05.2011 17:17, Eric Blake wrote:
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.
Fixed & pushed. Thanks.
participants (4)
-
Daniel Veillard -
Eric Blake -
Michal Privoznik -
Michal Prívozník