[libvirt] [PATCHv2 04/15] util: add virCommandSetUID and virCommandSetGID

[Laine Stump]

On 02/12/2013 07:16 PM, Eric Blake wrote:
> On 02/12/2013 01:15 PM, Laine Stump wrote:
>> If a uid and/or gid is specified for a command, it will be set just
>> after the user-supplied post-fork "hook" function is called.
>>
>> The intent is that this can replace user hook functions that set
>> uid/gid. This moves the setting of uid/gid and dropping of
>> capabilities closer to each other, which is important since the two
>> should really be done at the same time (libcapng provides a single
>> function that does both, which we will be unable to use, but want to
>> mimic as closely as possible).
>> ---
>> Change from V1: 
>> * only bypass uid/gid setting if they are -1
>> * cast -1 to ([gu]id_t) when comparing to a [gu]id_t
>> * cast uid and gid to (int) for printing
>>
>>  src/libvirt_private.syms |  2 ++
>>  src/util/vircommand.c    | 29 +++++++++++++++++++++++++++++
>>  src/util/vircommand.h    |  6 +++++-
>>  3 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index b9d45a2..511a686 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -158,12 +158,14 @@ virCommandRun;
>>  virCommandRunAsync;
>>  virCommandSetErrorBuffer;
>>  virCommandSetErrorFD;
>> +virCommandSetGID;
>>  virCommandSetInputBuffer;
>>  virCommandSetInputFD;
>>  virCommandSetOutputBuffer;
>>  virCommandSetOutputFD;
>>  virCommandSetPidFile;
>>  virCommandSetPreExecHook;
>> +virCommandSetUID;
> Is it common enough to set both gid/uid at once, in order to make this a
> single function virCommandSetUIDGID?

Well, at the bottom of the call chain, all three operations (setuid,
setgid, and set/clear capabilities) have to be done in a single
function, but setting capabilities is necessarily a separate function at
the higher level because you need to call it multiple times to set
multiple capabilities, and it seemed like a natural extension to make
setuid and setgid separate functions at this level too; seems to go
along with the general philosophy in virCommand of having many separate
simple calls, rather than a few really complicated ones.

>
>> @@ -605,6 +607,13 @@ virExec(virCommandPtr cmd)
>>             goto fork_error;
>>      }
>>  
>> +    if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1) {
>> +        VIR_DEBUG("Setting child uid:gid to %d:%d",
>> +                  (int)cmd->uid, (int)cmd->gid);
>> +        if (virSetUIDGID(cmd->uid, cmd->gid) < 0)
> In fact, down at a lower layer in the stack, we pass both ids at once.
>
> Hmm, in the chown() case, doing both at once lets you use one syscall
> instead of two; but in the set*id() functions, it's separate syscalls
> for uid vs. gid no matter what we do, so I guess it doesn't really
> matter whether it is two separate calls or one combined call higher up
> in the stack.  But what you have with separate calls works, so I don't
> mind whether you keep it as-is, to save the hassle of rippling a
> combined call through the rest of the series.
>
> ACK.
>