Re: [Qemu-devel] [PATCH 0/3] vnc: trivial problem and memory leak fix

[Gerd Hoffmann]

  Hi,

> >> Though we call qmp_change_vnc() failed, the content of global variable 
> >> vnc_display
> >> still will change, such as 'vs->auth = VNC_AUTH_NONE' now. I think this is 
> >> not
> >> reasonable, but I have not good idea to address this issue.
> > 
> > I think the current behavior is fine.  Better be on the safe side (no
> > connects possible) in case "change vnc $display" failed.  You can fix
> > that using "change vnc 0.0.0.0:10,password,sasl"
> 
> Yes. we can do this. But is it reasonable that an API return false,
> but its content changed?

We have:  Call failed -> vnc server not working.  I think that is ok.

You want:  Call failed -> vnc server has previous state.  Reasonable
too, but that is a hard problem.  You can try store everything in
temporary variables while initializing.  On success cleanup VncDisplay
struct and fill with the new data.  On failure leave VncDisplay
untouched.  But note that there are tricky corner cases such as binding
the new listening socket will fail when it is still in use by the old
listening socket.  Or goes on try IPv4 for the new in case the IPv6
address is in use by the old.  Have fun debugging your connection
problems then!

There is some middle ground -- you can try to do parameter parsing
first.  Any failures there are easily catched.  When that succeeds go
call vnc_display_close + reinitialize.  This avoids the problems
outlined above at the cost of not catching all possible failure modes.

If you want try solving this nevertheless I'd suggest to do it on top of
https://www.kraxel.org/cgit/qemu/log/?h=rebase/ui-vnc-next to avoid
conflicts.

Oh, and the switch to QemuOpts for vnc parameter parsing in the branch
might already improve things.

cheers,
  Gerd