Re: [libvirt] [PATCH v3] qemu: Pass file descriptor when using TPM passthrough

On 11/20/2014 08:08 AM, Stefan Berger wrote: Wow, I've been horribly slow at reviewing this. Do feel free to ping on list if no one seems to notice a patch, to widen the chances of anyone taking a glance at it. I'm still trying to figure out how virCommandReorderFDs() got into the picture (I didn't write that section of the code); when I originally worked on virCommand, the only way to pass fds to the child was in direct positions (same fd in child as in parent), not by remapping one fd in the parent to another position in the child, except for the three standard descriptors. It looks like virCommandReorderFDs was added to allow remapping other fds and populates the LISTEN_FDS environment variable with how many fds were thus mapped. So the two approaches don't really mix. Do we ever use virCommandPassListenFDs() on a virCommand that will also do raw fd passthrough? Maybe the real thing to do is to track at virCommand build-up time that only one of the two passthrough methods can be used, and fail loudly if a programmer tries to do both methods at once. Or maybe we should ALWAYS prepare to remap fds in the child, so that the child receives fds in contiguous order with no gaps. It might simplify the code base to always reorder things, and have the mapping done up front. That is, change the virCommandPassFD() function to return an integer of which next consecutive fd the child will see, or -1 on failure. Callers that then need to alter the command line of the child will have to pay attention to the return value (something a bit different than most virCommand build-up, which intentionally defer error checking to the very end), but it might be worth it. At any rate, this patch needs to be split into at least two - first, a patch to just util/vircommand.c and tests/commandtest.c to tweak the command passing functionality AND test that any new semantics work while no old semantics are broken (except maybe that we now explicitly reject a combination that previously was silently accepted but made no sense), and second the patch to qemu to use the enhancements in virCommand for the TPM feature at hand. Would it make any more sense for this function to do the transfer instead of making the caller do it? Oh, maybe not, since the caller marks the fd for passing once, but then calls multiple helper methods to format multiple parameters to qemu that end up working together to describe the fd used by the child. Here, we should use virReportSystemError and pass along the errno value from the failed open(). The else clause looks redundant with the last few lines of the if clause. Why not just sink it to occur just once after the 'if', as long as the 'if' side does a mere tmpdev=devset? This function is really huge. Yeah, we've not been good at breaking it into chunks in the past, but I think this addition would be nicer if the code inside 'if (def->tpm)' were extracted into a smaller helper function. But if we do that refactoring, the code motion of the existing pre-patch code should be its own patch. Okay, so you are enforcing that a caller can't pick two conflicting fd passing methods on the same virCommand, but my point remains that the testsuite should prove we can hit this error case, to avoid regressions. Or we should redo virCommand to ALWAYS rearrange fds in the child, and make it possible for a caller passing an fd down to know what fd the child will see it as. So, the mapping you are using is that the first fd passed through the child is eventually associated with /dev/fdset/$i, where $i represents which slot of cmd->passfd the descriptor was found in. I know the reason qemu added /dev/fdset/nnn was to allow the possibility of passing in sets of descriptors (such as a read-only and read-write handle both visiting the same file, so in the same fdset), but simplicity argues that unless we need that complexity, always naming our fdsets by the same fd that the child will first use is easier than trying to track the mapping between a parent fd and which order the fd was registered in. So I'm wondering if we change virCommandPassFD to always remap in the child and return an integer (the next consecutive integer starting at 3), then just use '-add-fd set=3,fd=3 ... /dev/fdset/3' is any simpler than trying to have set number unrelated to the fd number. Overall, I can see what you are hoping to do, and it's my fault for taking this long to get back to you on it. As penance, if you like my idea of changing virCommand to always remap child fds into consecutive orders, should I pitch in and help get that part of the patch in good condition? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org