[libvirt] [REPOST PATCH 0/6] Have 'buildVol' callers to clean up after themselves
Since some time and changes have occurred since the original posting, I figured it would be just easier to updated and repost the remainder of the series: http://www.redhat.com/archives/libvir-list/2015-October/msg00312.html Where the patches 1-3 are v2's of the original patches 8 & 9: http://www.redhat.com/archives/libvir-list/2015-October/msg00515.html http://www.redhat.com/archives/libvir-list/2015-October/msg00516.html http://www.redhat.com/archives/libvir-list/2015-October/msg00518.html Note that patches 4-6 have been ACK'd already as patches 10-12, it's just that without the previous patches it doesn't make much sense to push them alone. John Ferlan (6): storage: Refactor and fix virStorageBackendCreateExecCommand storage: Cleanup failures virStorageBackendCreateExecCommand storage: Cleanup failures in virStorageBackendCreateRaw storage: Pull volume removal from pool in storageVolDeleteInternal Revert "storage: Prior to creating a volume, refresh the pool" storage: On 'buildVol' failure don't delete the volume src/storage/storage_backend.c | 40 ++++++++++++++++++++++++++------------- src/storage/storage_backend.h | 10 ++++++++++ src/storage/storage_driver.c | 44 +++++++++++++++++++++---------------------- 3 files changed, 59 insertions(+), 35 deletions(-) -- 2.1.0
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails. So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + virCommandSetUmask(cmd, 0777 - mode); - if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */ + if (virFileExists(vol->target.path)) + goto cleanup; } } - /* don't change uid/gid if we retry */ + /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); + virCommandSetUmask(cmd, 0); - if (!filecreated) { - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - if (stat(vol->target.path, &st) < 0) { - virReportSystemError(errno, - _("failed to create %s"), vol->target.path); - goto cleanup; - } + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(errno, + _("failed to create %s"), vol->target.path); + goto cleanup; } uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid -- 2.1.0
On Wed, Oct 28, 2015 at 07:54:48AM -0400, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
Correctly - we should fail if we cannot create the file with the requested permissions. Or is the problem with chmod failing even though the file already has the right mode? Can't we just skip the chmod when the modes match, or -1 was requested, as we do for uid/gid?
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);
This assignment is now done twice in this function.
+ virCommandSetUmask(cmd, 0777 - mode);
Dealing with bits, I'd rather use xor.
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */
We should not ignore the explicitly requested uid/gid/mode just because we're in a NETFS pool. Also, this overwrites 'ret' from the default -1, without jumping to cleanup in all cases. Jan
On 11/03/2015 04:47 AM, Ján Tomko wrote:
On Wed, Oct 28, 2015 at 07:54:48AM -0400, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
Correctly - we should fail if we cannot create the file with the requested permissions.
The NETFS root-squash create file code will try both - which is what this follows. Look at the code prior to my changes - if the virCommandRun and the stat() inside the NETFS succeeds, then we skip the virCommandRun in the other path (even though it needlessly sets UID and GID in cmd). Next, if we have a successful NETFS creation, then the chown never happens since each will go to the ":" of the ternary as the virCommandRun would have been run under the provided UID/GID. The chmod is run in both cases, which is where the problem is if we have the pesky root squash environment. By not providing one at creation, we default to whatever nfs 'chooses' for that and we cannot change it using chmod() since nfs blocks that. So the "change" is essentially that the NETFS code will set the mode bits (via umask) during the virCommandRun which is done under the uid, gid, & umask provided just like would be done in our other root squash cases. If any of that fails, then we fallback to the non root squash mechanism where we try to set uid, gid, and mode.
Or is the problem with chmod failing even though the file already has the right mode? Can't we just skip the chmod when the modes match, or -1 was requested, as we do for uid/gid?
We document (in formatstorage) that if mode is not provided, then we default to 0600. So not attempting a change if mode == -1 isn't right. When I was first going through this - I considered checking for matching mode, but cannot recall why I chose not to. I can modify the check to be "if (mode != st.st_mode && chmod...)" in order to avoid the unnecessary chmod to the same mode bits.
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);
This assignment is now done twice in this function.
Hmm. right, so I'll move it to the top, e.g. mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);
+ virCommandSetUmask(cmd, 0777 - mode);
Dealing with bits, I'd rather use xor.
OK - xor isn't one of those unary operations that I use all that often. I will change this to: virCommandSetUmask(cmd, 0777 ^ mode); I also could use ACCESSPERMS instead of 0777; however, it seems the majority of code uses 0777. My fear in using ACCESSPERMS is some platform doesn't provide it and some build fails...
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */
We should not ignore the explicitly requested uid/gid/mode just because we're in a NETFS pool.
Not sure I understand the comment. If virCommandRun fails, then we fallback to the other methodology. If for some reason virCommandRun succeeds, but the file doesn't exist, then we fallback to the other mechanism (the difference in this being use of 'access' instead of 'stat' in order to determine that) which will attempt the chown & chmod on the created file.
Also, this overwrites 'ret' from the default -1, without jumping to cleanup in all cases.
Hmm. right - so I'll change it to look like: if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, if the file was created * then we are done */ if (virFileExists(vol->target.path)) { ret = 0; goto cleanup; } } I was trying to save a ret = 0; step, but sure if ret = 0 and !virFileExists, then a subsequent jump to cleanup would return the wrong value. Do you wish to see the adjusted patch? Just the diffs between this and the changes or a diffs between the changes and top of tree? John FWIW: I probably should have carried over the response/comment from the initial adjustment (w/r/t the new virCommandSetUmask): Just to be clear - this fixes a bug in the existing code which cannot create 'qcow2' file in an NFS pool that has root squash enabled. The failure "currently" is : virsh vol-create-from bz1253609 qemu-img.xml test.img --inputpool bz1253609 error: Failed to create vol from qemu-img.xml error: cannot set mode of '/tmp/netfs-pool/qemu-img-test.img' to 0644: Operation not permitted Assuming that 'test.img' was created using : virsh vol-create bz1253609 vol.xml The qemu-img.xml is: <volume> <name>qemu-img-test.img</name> <source> </source> <capacity>4048576000</capacity> <allocation>0</allocation> <target> <format type='qcow2'/> <permissions> <mode>0644</mode> <owner>107</owner> <group>107</group> <label>system_u:object_r:nfs_t:s0</label> </permissions> </target> </volume> it fails it other ways if <mode> and/or <owner>/<group> are not supplied. FWIW: The 'vol.xml' file differs from the above by only the "type='raw'"
On Tue, Nov 03, 2015 at 08:17:17AM -0500, John Ferlan wrote:
On 11/03/2015 04:47 AM, Ján Tomko wrote:
On Wed, Oct 28, 2015 at 07:54:48AM -0400, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
Correctly - we should fail if we cannot create the file with the requested permissions.
The NETFS root-squash create file code will try both - which is what this follows. Look at the code prior to my changes - if the virCommandRun and the stat() inside the NETFS succeeds, then we skip the virCommandRun in the other path (even though it needlessly sets UID and GID in cmd).
Next, if we have a successful NETFS creation, then the chown never happens since each will go to the ":" of the ternary as the virCommandRun would have been run under the provided UID/GID.
I thought the point of squashing was that the files can be owned by different users and groups that created them - even if we successfully ran the command as uid:gid, we check if it matches st_uid:st_gid.
The chmod is run in both cases, which is where the problem is if we have the pesky root squash environment. By not providing one at creation, we default to whatever nfs 'chooses' for that and we cannot change it using chmod() since nfs blocks that.
If root squash blocks chmod, we should skip it if the mode of the created file already matches the requested mode.
So the "change" is essentially that the NETFS code will set the mode bits (via umask) during the virCommandRun which is done under the uid, gid, & umask provided just like would be done in our other root squash cases.
It seems there is no mount/export to set the umask, so setting it this way should work unless the creation tool uses a different mode than 0777.
If any of that fails, then we fallback to the non root squash mechanism where we try to set uid, gid, and mode.
Or is the problem with chmod failing even though the file already has the right mode? Can't we just skip the chmod when the modes match, or -1 was requested, as we do for uid/gid?
We document (in formatstorage) that if mode is not provided, then we default to 0600. So not attempting a change if mode == -1 isn't right.
When I was first going through this - I considered checking for matching mode, but cannot recall why I chose not to. I can modify the check to be
"if (mode != st.st_mode && chmod...)"
in order to avoid the unnecessary chmod to the same mode bits.
st_mode also contains the file type that needs to be masked out: if (mode != (st.st_mode & S_IRWXUGO) && ...
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
+ virCommandSetUmask(cmd, 0777 - mode);
Dealing with bits, I'd rather use xor.
OK - xor isn't one of those unary operations that I use all that often. I will change this to:
virCommandSetUmask(cmd, 0777 ^ mode);
I also could use ACCESSPERMS instead of 0777; however, it seems the majority of code uses 0777. My fear in using ACCESSPERMS is some platform doesn't provide it and some build fails...
We already use S_IRWXUGO in virStorageBackendUpdateVolTargetInfoFD elsewhere in this file.
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */
We should not ignore the explicitly requested uid/gid/mode just because we're in a NETFS pool.
Not sure I understand the comment. If virCommandRun fails, then we fallback to the other methodology. If for some reason virCommandRun succeeds, but the file doesn't exist, then we fallback to the other mechanism (the difference in this being use of 'access' instead of 'stat' in order to determine that) which will attempt the chown & chmod on the created file.
I was wondering about a case where virCommandRun succeeds, the file exists but has different properties than we expected. Leaving the existing checks for owner and user would guard us from that, but if you're sure that can't happen (and we don't try to pretend to work with all_squash), let's skip the checks.
Also, this overwrites 'ret' from the default -1, without jumping to cleanup in all cases.
Hmm. right - so I'll change it to look like:
if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, if the file was created * then we are done */ if (virFileExists(vol->target.path)) { ret = 0; goto cleanup; } }
I was trying to save a ret = 0; step, but sure if ret = 0 and !virFileExists, then a subsequent jump to cleanup would return the wrong value.
Or just return 0, since there's nothing to clean up at this point.
Do you wish to see the adjusted patch? Just the diffs between this and the changes or a diffs between the changes and top of tree?
I prefer to see an adjusted patch in a separate mail, that way I can easily apply it. Not to mention the diffs to diffs would take more screen space than the whole patch.
John
FWIW:
I probably should have carried over the response/comment from the initial adjustment (w/r/t the new virCommandSetUmask):
Just to be clear - this fixes a bug in the existing code which cannot create 'qcow2' file in an NFS pool that has root squash enabled. The failure "currently" is :
Is there a public bug that could be mentioned in the commit message? Jan
On 11/03/2015 10:51 AM, Ján Tomko wrote:
On Tue, Nov 03, 2015 at 08:17:17AM -0500, John Ferlan wrote:
On 11/03/2015 04:47 AM, Ján Tomko wrote:
On Wed, Oct 28, 2015 at 07:54:48AM -0400, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
Correctly - we should fail if we cannot create the file with the requested permissions.
The NETFS root-squash create file code will try both - which is what this follows. Look at the code prior to my changes - if the virCommandRun and the stat() inside the NETFS succeeds, then we skip the virCommandRun in the other path (even though it needlessly sets UID and GID in cmd).
Next, if we have a successful NETFS creation, then the chown never happens since each will go to the ":" of the ternary as the virCommandRun would have been run under the provided UID/GID.
I thought the point of squashing was that the files can be owned by different users and groups that created them - even if we successfully ran the command as uid:gid, we check if it matches st_uid:st_gid.
In a root squashed environment, if a "root" client created files, they would have what the nfs server defined for owner/group (via anonuid and anongid which both default to 65534). The run under some other owner/group is as a forked process which does the setreuid in virSetUIDGID - the path to get there is a bit longer in virCommandRun than virFileOpenForked.
The chmod is run in both cases, which is where the problem is if we have the pesky root squash environment. By not providing one at creation, we default to whatever nfs 'chooses' for that and we cannot change it using chmod() since nfs blocks that.
If root squash blocks chmod, we should skip it if the mode of the created file already matches the requested mode.
OK - that's possible, but for the root-squash perhaps it wasn't clear it's not necessary now since virCommandRun can use the 'umask' instead in order to create the file with the "right" mode bits.
So the "change" is essentially that the NETFS code will set the mode bits (via umask) during the virCommandRun which is done under the uid, gid, & umask provided just like would be done in our other root squash cases.
It seems there is no mount/export to set the umask, so setting it this way should work unless the creation tool uses a different mode than 0777.
If any of that fails, then we fallback to the non root squash mechanism where we try to set uid, gid, and mode.
Or is the problem with chmod failing even though the file already has the right mode? Can't we just skip the chmod when the modes match, or -1 was requested, as we do for uid/gid?
We document (in formatstorage) that if mode is not provided, then we default to 0600. So not attempting a change if mode == -1 isn't right.
When I was first going through this - I considered checking for matching mode, but cannot recall why I chose not to. I can modify the check to be
"if (mode != st.st_mode && chmod...)"
in order to avoid the unnecessary chmod to the same mode bits.
st_mode also contains the file type that needs to be masked out: if (mode != (st.st_mode & S_IRWXUGO) && ...
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
+ virCommandSetUmask(cmd, 0777 - mode);
Dealing with bits, I'd rather use xor.
OK - xor isn't one of those unary operations that I use all that often. I will change this to:
virCommandSetUmask(cmd, 0777 ^ mode);
I also could use ACCESSPERMS instead of 0777; however, it seems the majority of code uses 0777. My fear in using ACCESSPERMS is some platform doesn't provide it and some build fails...
We already use S_IRWXUGO in virStorageBackendUpdateVolTargetInfoFD elsewhere in this file.
Perfect - I had just searched on "0777"
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */
We should not ignore the explicitly requested uid/gid/mode just because we're in a NETFS pool.
Not sure I understand the comment. If virCommandRun fails, then we fallback to the other methodology. If for some reason virCommandRun succeeds, but the file doesn't exist, then we fallback to the other mechanism (the difference in this being use of 'access' instead of 'stat' in order to determine that) which will attempt the chown & chmod on the created file.
I was wondering about a case where virCommandRun succeeds, the file exists but has different properties than we expected. Leaving the existing checks for owner and user would guard us from that, but if you're sure that can't happen (and we don't try to pretend to work with all_squash), let's skip the checks.
I'm still not certain how virCommandRun succeeds, but the file doesn't exist. Although, now I see your point. The v3 will follow the original logic more closely and remove that "chance" that something doesn't work as expected.
Also, this overwrites 'ret' from the default -1, without jumping to cleanup in all cases.
Hmm. right - so I'll change it to look like:
if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, if the file was created * then we are done */ if (virFileExists(vol->target.path)) { ret = 0; goto cleanup; } }
I was trying to save a ret = 0; step, but sure if ret = 0 and !virFileExists, then a subsequent jump to cleanup would return the wrong value.
Or just return 0, since there's nothing to clean up at this point.
Do you wish to see the adjusted patch? Just the diffs between this and the changes or a diffs between the changes and top of tree?
I prefer to see an adjusted patch in a separate mail, that way I can easily apply it. Not to mention the diffs to diffs would take more screen space than the whole patch.
Just making sure - some are fine with a git am of the diffs while others prefer a git am of the entire patch. I'm just asking so I don't assume anything
John
FWIW:
I probably should have carried over the response/comment from the initial adjustment (w/r/t the new virCommandSetUmask):
Just to be clear - this fixes a bug in the existing code which cannot create 'qcow2' file in an NFS pool that has root squash enabled. The failure "currently" is :
Is there a public bug that could be mentioned in the commit message?
There was no public bug on this. It was self found when trying to "test" what opaque path was causing the code to go into the NETFS if condition. Reverse engineered a bug I suppose based on recent learnings of root squashed environments. John I will send a V3 shortly (for this and the following patch which is also affected)
Currently the code does not handle the NFS root squash environment properly since if the file gets created, then the subsequent chmod will fail in a root squash environment where we're creating a file in the pool with qemu tools, such as seen via: $ virsh vol-create-from $pool $file.xml file.img --inputpool $pool assuming $file.xml is creating a file of "<format type='qcow2'"> from an existing file.img in the pool of "<format type='raw'>". This patch will utilize the virCommandSetUmask when creating the file in the NETFS pool. The virCommandSetUmask API was added in commit id '0e1a1a8c4', which was after the original code was developed in commit id 'e1f27784' to attempt to handle the root squash environment. Also, rather than blindly attempting to chmod, check to see if the st_mode bits from the stat match what we're trying to set and only make the chmod if they don't. Also, a slight adjustment to the fallback algorithm to move the virCommandSetUID/virCommandSetGID inside the if (!filecreated) since they're only useful if we need to attempt to create the file again. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Change over v2: - Revert back to the original style of using filecreated in order to determine whether or not to run the second virCommandRun instead of checking status and virFileExists - This means the 'stat' will be run and the subsequent chown and chmod could be run, but more than likely won't since the values should match. - Use of S_IRWXUGO instead of 0777 - Compare the current st_mode against the expected mode and only call chmod if we need to change. src/storage/storage_backend.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..2ba6e27 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -677,7 +677,9 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, struct stat st; gid_t gid; uid_t uid; - mode_t mode; + mode_t mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode); bool filecreated = false; int ret = -1; @@ -690,6 +692,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + virCommandSetUmask(cmd, S_IRWXUGO ^ mode); if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, check if the file was created */ @@ -698,11 +701,12 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - /* don't change uid/gid if we retry */ - virCommandSetUID(cmd, -1); - virCommandSetGID(cmd, -1); - if (!filecreated) { + /* don't change uid/gid/mode if we retry */ + virCommandSetUID(cmd, -1); + virCommandSetGID(cmd, -1); + virCommandSetUmask(cmd, 0); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; if (stat(vol->target.path, &st) < 0) { @@ -725,9 +729,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, goto cleanup; } - mode = (vol->target.perms->mode == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); - if (chmod(vol->target.path, mode) < 0) { + if (mode != (st.st_mode & S_IRWXUGO) && + chmod(vol->target.path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), vol->target.path, mode); -- 2.1.0
On Tue, Nov 03, 2015 at 12:09:48PM -0500, John Ferlan wrote:
Currently the code does not handle the NFS root squash environment properly since if the file gets created, then the subsequent chmod will fail in a root squash environment where we're creating a file in the pool with qemu tools, such as seen via:
$ virsh vol-create-from $pool $file.xml file.img --inputpool $pool
assuming $file.xml is creating a file of "<format type='qcow2'"> from an existing file.img in the pool of "<format type='raw'>".
This patch will utilize the virCommandSetUmask when creating the file in the NETFS pool. The virCommandSetUmask API was added in commit id '0e1a1a8c4', which was after the original code was developed in commit id 'e1f27784' to attempt to handle the root squash environment.
Also, rather than blindly attempting to chmod, check to see if the st_mode bits from the stat match what we're trying to set and only make the chmod if they don't.
Also, a slight adjustment to the fallback algorithm to move the virCommandSetUID/virCommandSetGID inside the if (!filecreated) since they're only useful if we need to attempt to create the file again.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
ACK Jan
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 037d6d7..487c914 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,6 +678,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; + bool filecreated = false; int ret = -1; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -708,6 +709,9 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if (virCommandRun(cmd, NULL) < 0) goto cleanup; + + filecreated = true; + if (stat(vol->target.path, &st) < 0) { virReportSystemError(errno, _("failed to create %s"), vol->target.path); @@ -739,6 +743,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0; cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path); return ret; } -- 2.1.0
On Wed, Oct 28, 2015 at 07:54:49AM -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 037d6d7..487c914 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,6 +678,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; + bool filecreated = false;
This variable was removed in the previous patch, reordering them would remove the need for that. Jan
On 11/03/2015 04:13 AM, Ján Tomko wrote:
On Wed, Oct 28, 2015 at 07:54:49AM -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 037d6d7..487c914 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,6 +678,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; + bool filecreated = false;
This variable was removed in the previous patch, reordering them would remove the need for that.
I just chose to fix a bug first, then continue with applying the sequence of the original series. Personally I would prefer to squash the two together... Is the order all that precludes an ACK? It doesn't really matter to me. John
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should remove the file. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Changes over v2: - The 'filecreated' was kept from the previous patch, but we still need to set it once virCommandRun and stat() agree the file was created. - Use virFileRemove and not just unlink in order to delete the file. If this was a root squash created file, then the only way to remove it properly is via virFileRemove src/storage/storage_backend.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2ba6e27..15470e8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -714,6 +714,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, _("failed to create %s"), vol->target.path); goto cleanup; } + filecreated = true; } uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid @@ -740,6 +741,9 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0; cleanup: + if (ret < 0 && filecreated) + virFileRemove(vol->target.path, vol->target.perms->uid, + vol->target.perms->gid); return ret; } -- 2.1.0
On Tue, Nov 03, 2015 at 12:12:31PM -0500, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should remove the file.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
ACK Jan
After successfully returning from virFileOpenAs, if subsequent calls fail, then we need to remove the file since our caller expects that failures after creation will remove the created file. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 487c914..8dcb1dc 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -485,6 +485,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; + bool created = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | VIR_STORAGE_VOL_CREATE_REFLINK, @@ -531,6 +532,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } + created = true; if (vol->target.nocow) { #ifdef __linux__ @@ -557,6 +559,10 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, ret = -1; cleanup: + if (ret < 0 && created) + ignore_value(virFileRemove(vol->target.path, + vol->target.perms->uid, + vol->target.perms->gid)); VIR_FORCE_CLOSE(fd); return ret; } -- 2.1.0
On Wed, Oct 28, 2015 at 07:54:50AM -0400, John Ferlan wrote:
After successfully returning from virFileOpenAs, if subsequent calls fail, then we need to remove the file since our caller expects that failures after creation will remove the created file.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+)
ACK Jan
Create a helper function to remove volume from the pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0aa2d6e..292ed9e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1619,6 +1619,25 @@ storagePoolLookupByTargetPath(virConnectPtr conn, } +static void +storageVolRemoveFromPool(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + size_t i; + + for (i = 0; i < pool->volumes.count; i++) { + if (pool->volumes.objs[i] == vol) { + VIR_INFO("Deleting volume '%s' from storage pool '%s'", + vol->name, pool->def->name); + virStorageVolDefFree(vol); + + VIR_DELETE_ELEMENT(pool->volumes.objs, i, pool->volumes.count); + break; + } + } +} + + static int storageVolDeleteInternal(virStorageVolPtr obj, virStorageBackendPtr backend, @@ -1627,7 +1646,6 @@ storageVolDeleteInternal(virStorageVolPtr obj, unsigned int flags, bool updateMeta) { - size_t i; int ret = -1; if (!backend->deleteVol) { @@ -1651,16 +1669,7 @@ storageVolDeleteInternal(virStorageVolPtr obj, } } - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i] == vol) { - VIR_INFO("Deleting volume '%s' from storage pool '%s'", - vol->name, pool->def->name); - virStorageVolDefFree(vol); - - VIR_DELETE_ELEMENT(pool->volumes.objs, i, pool->volumes.count); - break; - } - } + storageVolRemoveFromPool(pool, vol); ret = 0; cleanup: -- 2.1.0
This reverts commit fdda37608a6e22406fbdfe4ac0c573a96a8d0417. This commit only manages a symptom of finding a buildRet failure where a volume was not listed in the pool, but someone created the volume outside of libvirt in the pool being managed by libvirt. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 292ed9e..31b7095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1805,15 +1805,6 @@ storageVolCreateXML(virStoragePoolPtr obj, if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0) goto cleanup; - /* While not perfect, refresh the list of volumes in the pool and - * then check that the incoming name isn't already in the pool. - */ - if (backend->refreshPool) { - virStoragePoolObjClearVols(pool); - if (backend->refreshPool(obj->conn, pool) < 0) - goto cleanup; - } - if (virStorageVolDefFindByName(pool, voldef->name)) { virReportError(VIR_ERR_STORAGE_VOL_EXIST, _("'%s'"), voldef->name); -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1233003 Commit id 'fdda3760' only managed a symptom where it was possible to create a file in a pool without libvirt's knowledge, so it was reverted. The real fix is to have all the createVol API's which actually create a volume (disk, logical, zfs) and the buildVol API's which handle the real creation of some volume file (fs, rbd, sheepdog) manage deleting any volume which they create when there is some sort of error in processing the volume. This way the onus isn't left up to the storage_driver to determine whether the buildVol failure was due to some failure as a result of adjustments made to the volume after creation such as getting sizes, changing ownership, changing volume protections, etc. or simple a failure in creation. Without needing to consider that the volume has to be removed, the buildVol failure path only needs to remove the volume from the pool. This way if a creation failed due to duplicate name, libvirt wouldn't remove a volume that it didn't create in the pool target. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.h | 10 ++++++++++ src/storage/storage_driver.c | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 39c00b1..96b5f39 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -48,6 +48,16 @@ typedef int (*virStorageBackendStopPool)(virConnectPtr conn, typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags); + +/* A 'buildVol' backend must remove any volume created on error since + * the storage driver does not distinguish whether the failure is due + * to failure to create the volume, to reserve any space necessary for + * the volume, to get data about the volume, to change it's accessibility, + * etc. This avoids issues arising from a creation failure due to some + * external action which created a volume of the same name that libvirt + * was not aware of between checking the pool and the create attempt. It + * also avoids extra round trips to just delete a file. + */ typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 31b7095..ee8b263 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1869,8 +1869,8 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--; if (buildret < 0) { - storageVolDeleteInternal(volobj, backend, pool, voldef, - 0, false); + /* buildVol handles deleting volume on failure */ + storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup; } -- 2.1.0
participants (2)
-
John Ferlan -
Ján Tomko