On 02/08/2013 02:05 PM, Eric Blake wrote:
On 02/07/2013 02:37 PM, Laine Stump wrote:
virCommand was previously calling virSetUIDGID() to change the uid and gid of the child process, then separately calling virSetCapabilities(). This did not work if the desired uid was != 0, since a setuid to anything other than 0 normally clears all capabilities bits.
The solution is to use the new virSetUIDGIDWithCaps(), sending it the uid, gid, and capabilities bits. This will get the new process setup properly.
Since the static functions virSetCapabilities() and virClearCapabilities are no longer called, they have been removed.
NOTE: When combined with "filecap $path-to-qemu sys_rawio", this patch will make CAP_SYS_RAWIO (which is required for passthrough of generic scsi commands to a guest - see commits e8daeeb, 177db08, 397e6a7, and 74e0349) be retained by qemu when necessary. Apparently that capability has been broken for non-root qemu every since it was s/every/ever/
originally added. --- src/util/vircommand.c | 76 ++++++--------------------------------------------- 1 file changed, 8 insertions(+), 68 deletions(-) ACK.
-# else -static int virClearCapabilities(void) -{ -// VIR_WARN("libcap-ng support not compiled in, unable to clear " -// "capabilities"); Odd that we had commented this out previously.
1) Even more odd is that this function wasn't being called from anywhere, but still existed *and* didn't cause a compile error. I had thought that unreferenced static functions always caused a warning (which we promote to an error). 2) There are several places where we call virCommandClearCapabilities() without regard to whether or not WITH_CAPNG is actually set. Systems without WITH_CAPNG would have tons of warnings, which I'm guessing people don't want. If we want to do a warning or error, maybe we could do it at configure time (maybe require people to specifically say "--without-capng" or something).
Should patch 13/15 log any warnings when we are not preserving/clearing capabilities, rather than silently ignoring the capability request?
- if (cmd->uid > 0 || cmd->gid > 0) { - VIR_DEBUG("Setting child uid:gid to %u:%u", cmd->uid, cmd->gid); - if (virSetUIDGID(cmd->uid, cmd->gid) < 0) + /* The steps above may need todo something privileged, so we delay
As long as you are touching this comment, s/todo/to do/ (but you've moved it at least twice in this series, so it depends on how much churn you want on when you finally fix it).
Okay. I'll do it here.