Re: [libvirt] [REPOST PATCH 1/6] storage: Refactor and fix virStorageBackendCreateExecCommand

On 11/03/2015 04:47 AM, Ján Tomko wrote: 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. 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. 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); 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... 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. 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'"