On 3/21/22 20:56, Marc-André Lureau wrote: > Hi > > On Mon, Mar 21, 2022 at 6:59 PM Michal Privoznik <mprivozn@xxxxxxxxxx > <mailto:mprivozn@xxxxxxxxxx>> wrote: > > When vTPM is secured via virSecret libvirt passes the secret > value via an FD when swtpm is started (arguments --key and > --migration-key). The writing of the secret into the FDs is > handled via virCommand, specifically qemu_tpm calls > virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a > thread to handle writing into the FD via > virCommandDoAsyncIOHelper. But the thread is not created unless > VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix > it, virCommandDoAsyncIO() must be called. > > The credit goes to Marc-André Lureau > <marcandre.lureau@xxxxxxxxxx <mailto:marcandre.lureau@xxxxxxxxxx>> > who has done all the debugging and > proposed fix in the bugzilla. > > > (thanks for the credit :) > > Wouldn't it make sense to return an error if SendBuffers is used without > AsyncIO then? Or just enable AsyncIO as necessary? (beware, I am not > very familiar with virCommand API. I don't know what this would imply) Yeah, I could try to make virCommand error out, but at any of those functions. Thing is, it's actually vitCommandDaemonize() that's troublemaker here. For SendBuffers to work, virCommandProcessIO() has to run and there are two places where it's called: 1) for non-daemonized processes it's called directly from virCommandRun(), 2) for daemonized processes it's called from the intermediary child. Therefore, SendBuffers() can work even without AsyncIO(). Anyway, the idea behind virCommand is that caller doesn't have to check for retval of individual virCommand* calls as they do that itself as the very first thing - virCommandHasError() - and turn themselves into NOP if there was an error earlier. This allows us to write shorter functions, e.g.: cmd = virComandNew("/usr/bin/binary"); virCommandSomething(cmd); virCommandSomethingElse(cmd); virCommandSomethingThatFails(cmd); /* no error reported here */ rc = virCommandRun(cmd, NULL); /* it's only here that the error from earlier is propagated and rc is set to -1 */ In general it's okay to swap virCommand calls, unless appending arguments (which would then be swapped too). > > Also it would be nice to cover that "behaviour" in a test (even better > if we could cover the swtpm setup & start handling too, although I > realize this is more work!) I'm not exactly sure what to test here. We don't have a test suite for this area of the code. I mean, we do have a test that covers virCommand and I can definitely add a test case there, but it runs only a small & dumb binary of ours. But at least we would have proper example of use somewhere. Let me post that in v2. Thanks! Michal
