Re: [PATCH 1/2] qemu: agent: sync once if qemu has serial port event
On 11.03.2020 12:38, Michal Privoznik wrote:
On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
> Sync was introduced in [1] to check for ga presence. This
> check is racy but in the era before serial events are available
> there was not better solution I guess.
Can you elaborate more on the raciness? There should never be more than one thread
talking on the agent monitor.
The race is in another dimension) So sync checks for GA presence, it only waits for 5s so
if
there is no GA the actual command with no timeout won't block. But GA can go down
right
after successful sync so command can block. This is true if there are no serial events
in the latter case agent monitor will be closed when GA go down and command will unblock.
>
> In case we have the events the sync function is different. It allows us
> to flush stateless ga channel from remnants of previous communications.
> But we need to do it only once. Until we get timeout on issued command
> channel state is ok.
>
> [1] qemu_agent: Issue guest-sync prior to every command
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/qemu/qemu_agent.c | 13 ++++++++++++-
> src/qemu/qemu_agent.h | 3 ++-
> src/qemu/qemu_process.c | 3 ++-
> tests/qemumonitortestutils.c | 3 ++-
> 4 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index cd25ef6cd3..2de53efb7a 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -104,6 +104,8 @@ struct _qemuAgent {
> int watch;
> bool running;
> + bool singleSync;
> + bool inSync;
>
I wonder if we can do this with just inSync and not have singleSync. I mean, it looks
like at all places where singleSync is used we have access to qemuCaps so it should be as
easy as:
qemuAgentOpen();
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
if *qemuAgentGuestSync(mon) < 0)
goto error;
mon->inSync = true;
and then qemuagentGuestSync() would be a NOP if inSync is set. But it would also not set
it. But maybe I'm pushing it too far, it's just that I'm confused by the name
'singleSync'. Probably 'hasSingleSync' would be better? The boolean
reflect whether qemu has the VSERPORT_CHANGE capability and thus libvirt knows when GA
connects/disconnects. 'singleSync' just doesn't ring that bell at first.
If we try to sync only on monitor opening then we can't sync if for example command
timeouts and
GA stays connect (thus no serial event and monitor won't be closed). With current
patch we just
try to sync when we need to send next command to GA.
I guess singleSync is better. It just reflects that we don't need to sync before
every
command (in order to avoid hangs) in case there is serial events. Strictly speaking
it is not correct to use sync to check for GA presence because of the race and as to
using it as a means to flush channel it works well without sereal events as well.
Nikolay