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

Tue Nov 3 13:17:17 UTC 2015

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 at 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'"