[libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate

[Laine Stump]

On 01/13/2010 09:32 AM, Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serue at us.ibm.com):
>    
>> Quoting Laine Stump (laine at laine.org):
>>      
>>> These functions create a new file or directory with the given
>>> uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by
>>> forking a new process, calling setuid/setgid in the new process, and
>>> then creating the file. This is better than simply calling open then
>>> fchown, because in the latter case, a root-squashing nfs server would
>>> create the new file as user nobody, then refuse to allow fchown.
>>>
>>> If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of
>>> creating the file/dir, then chowning is is used. This gives better
>>> results in cases where the parent directory isn't on a root-squashing
>>> NFS server, but doesn't give permission for the specified uid/gid to
>>> create files. (Note that if the fork/setuid method fails to create the
>>> file due to access privileges, the parent process will make a second
>>> attempt using this simpler method.)
>>>
>>> Return from both of these functions is 0 on success, or the value of
>>> errno if there was a failure.
>>> ---
>>>   src/util/util.c |  247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   src/util/util.h |    9 ++
>>>   2 files changed, 256 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/util/util.c b/src/util/util.c
>>> index 1d493de..1cb29f4 100644
>>> --- a/src/util/util.c
>>> +++ b/src/util/util.c
>>> @@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
>>>       return(0);
>>>   }
>>>
>>> +
>>> +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
>>> +    int fd = -1;
>>> +    int ret = 0;
>>> +
>>> +    if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode))<  0) {
>>> +        ret = errno;
>>> +        virReportSystemError(NULL, errno, _("failed to create file '%s'"),
>>> +                             path);
>>> +        goto error;
>>> +    }
>>> +    if ((getuid() == 0)&&  ((uid != 0) || (gid != 0))) {
>>>        
>> How about checking for CAP_CHOWN instead of getuid()==0?  Otherwise
>> if I try installing this certain ways, virFileCreateSimple() will refuse
>> to try the chown even though it could succeed.
>>      
> I guess for certain netfs's the uid might matter more, so checking for
> either condition - or in fact just trying the fchown, then doing a stat,
> then making sure st.st_{ug}id are correct after the fact - seems useful.
>    

That was actually just a duplication of existing functionality from the 
code that will now call virFileCreate() (right down to the lack of any 
error reporting if you're not running as root and the caller requests 
the file to be created with a different uid).
The same check/chown is done in a couple other places in this patch 
series, so your comment applies to those as well.

If I were to remove the check of current uid, should a failure of chown 
then be reported as an error, or ignored (making it closer to current 
behavior for non-root users)? And does this need to be added to the 
current patch, or can it be considered a follow-on? (I guess the 
question behind that is do people commonly use libvirt in a manner that 
would be affected by this, or is this theorizing about something someone 
*might* do?)