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

John Ferlan jferlan at redhat.com
Tue Nov 3 16:49:36 UTC 2015

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

>> 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.


I will send a V3 shortly (for this and the following patch which is also

More information about the libvir-list mailing list