[PATCH] qemu_tpm: Do async IO when starting swtpm emulator

[Michal Prívozník]

On 3/21/22 20:56, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 21, 2022 at 6:59 PM Michal Privoznik <mprivozn at redhat.com
> <mailto:mprivozn at redhat.com>> 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 at redhat.com <mailto:marcandre.lureau at redhat.com>>
>     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