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

[Gonglei]

On 2014/10/23 20:16, Gerd Hoffmann wrote:

>   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!
> 

Indeed.

> 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.
> 

Yes.

> 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.
> 

OK.

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

Hmm. That's very useful, Thanks a lot :)

Best regards,
-Gonglei