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

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

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Beside those problems, I also found another issue, see below pls.

Qemu command line:
$ ./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive file=/home/redhat.img,if=none,id=drive-ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file=/mnt/sdb/gonglei/iso/rhel-server-7.0-x86_64-dvd.iso,if=none,id=drive-ide0-0-1 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,bootindex=4 -qmp unix:/tmp/qmp,server,nowait -monitor stdio -vnc 0.0.0.0:10,password,sasl
QEMU 2.1.50 monitor - type 'help' for more information
(qemu) change vnc password 123
(qemu) change vnc abc
error parsing address 'abc'     ---> an intended result because of I set a invalid value 'abc'
(qemu) change vnc password 456
If you want use passwords please enable password auth using '-vnc ${dpy},password'.Could not set password  --> Oops. I shouldn't get this output.
(qemu) 

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. Maybe we should create
a function to save the possible changed property of vnc_display, so that we can recovery
vnc_display when we changed vnc failed.

What's your opinion? Gerd? Thanks,

Gonglei (3):
  vnc: remove superfluous vnc_display_close()
  vnc: fix memory leak at vnc_display_open
  vnc: remove superfluous DisplayState *ds parameter

 include/ui/console.h |  2 +-
 qmp.c                |  2 +-
 ui/vnc.c             | 36 ++++--------------------------------
 vl.c                 |  2 +-
 4 files changed, 7 insertions(+), 35 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] vnc: remove superfluous vnc_display_close()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

vnc_display_close() is called in vnc_display_open().
And in this moment, the ds variable is initilized
at vnc_display_init(). Calling vnc_display_close()
is superfluous.

For another scenario, qmp_change_vnc_listen() pass NULL
into vnc_display_open(), which is not influenced too.

Let's remove it.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 ui/vnc.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 0fe6eff..628a7ba 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2972,36 +2972,6 @@ void vnc_display_init(DisplayState *ds)
     register_displaychangelistener(&vs->dcl);
 }
 
-
-static void vnc_display_close(DisplayState *ds)
-{
-    VncDisplay *vs = vnc_display;
-
-    if (!vs)
-        return;
-    g_free(vs->display);
-    vs->display = NULL;
-    if (vs->lsock != -1) {
-        qemu_set_fd_handler2(vs->lsock, NULL, NULL, NULL, NULL);
-        close(vs->lsock);
-        vs->lsock = -1;
-    }
-#ifdef CONFIG_VNC_WS
-    g_free(vs->ws_display);
-    vs->ws_display = NULL;
-    if (vs->lwebsock != -1) {
-        qemu_set_fd_handler2(vs->lwebsock, NULL, NULL, NULL, NULL);
-        close(vs->lwebsock);
-        vs->lwebsock = -1;
-    }
-#endif /* CONFIG_VNC_WS */
-    vs->auth = VNC_AUTH_INVALID;
-#ifdef CONFIG_VNC_TLS
-    vs->subauth = VNC_AUTH_INVALID;
-    vs->tls.x509verify = 0;
-#endif
-}
-
 int vnc_display_password(DisplayState *ds, const char *password)
 {
     VncDisplay *vs = vnc_display;
@@ -3062,7 +3032,7 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
         error_setg(errp, "VNC display not active");
         return;
     }
-    vnc_display_close(ds);
+
     if (strcmp(display, "none") == 0)
         return;
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/3] vnc: fix memory leak at vnc_display_open

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When using qmp change vnc interface, will leak
memory of vs->display and vs->ws_display (if configed).
Free them before calling g_strdup()/g_strconcat().

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 ui/vnc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 628a7ba..38229a7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3036,6 +3036,7 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
     if (strcmp(display, "none") == 0)
         return;
 
+    g_free(vs->display);
     vs->display = g_strdup(display);
     vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
 
@@ -3083,6 +3084,7 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
                     } else {
                         host = g_strndup(":", 1);
                     }
+                    g_free(vs->ws_display);
                     vs->ws_display = g_strconcat(host, port, NULL);
                     g_free(host);
                     g_free(port);
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/3] vnc: remove superfluous DisplayState *ds parameter

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 include/ui/console.h | 2 +-
 qmp.c                | 2 +-
 ui/vnc.c             | 2 +-
 vl.c                 | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 22ef8ca..8cc48cc 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -328,7 +328,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
 
 /* vnc.c */
 void vnc_display_init(DisplayState *ds);
-void vnc_display_open(DisplayState *ds, const char *display, Error **errp);
+void vnc_display_open(const char *display, Error **errp);
 void vnc_display_add_client(DisplayState *ds, int csock, bool skipauth);
 char *vnc_display_local_addr(DisplayState *ds);
 #ifdef CONFIG_VNC
diff --git a/qmp.c b/qmp.c
index 0b4f131..0c8136e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -368,7 +368,7 @@ void qmp_change_vnc_password(const char *password, Error **errp)
 
 static void qmp_change_vnc_listen(const char *target, Error **errp)
 {
-    vnc_display_open(NULL, target, errp);
+    vnc_display_open(target, errp);
 }
 
 static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
diff --git a/ui/vnc.c b/ui/vnc.c
index 38229a7..8f2f277 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3010,7 +3010,7 @@ char *vnc_display_local_addr(DisplayState *ds)
     return vnc_socket_local_addr("%s:%s", vs->lsock);
 }
 
-void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
+void vnc_display_open(const char *display, Error **errp)
 {
     VncDisplay *vs = vnc_display;
     const char *options;
diff --git a/vl.c b/vl.c
index aee73e1..8ae6eca 100644
--- a/vl.c
+++ b/vl.c
@@ -4318,7 +4318,7 @@ int main(int argc, char **argv, char **envp)
     if (vnc_display) {
         Error *local_err = NULL;
         vnc_display_init(ds);
-        vnc_display_open(ds, vnc_display, &local_err);
+        vnc_display_open(vnc_display, &local_err);
         if (local_err != NULL) {
             error_report("Failed to start VNC server on `%s': %s",
                          vnc_display, error_get_pretty(local_err));
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 3/3] vnc: remove superfluous DisplayState *ds parameter

[Michael Tokarev]

On 10/23/2014 09:39 AM, arei.gonglei@huawei.com wrote:
[]
> --- a/vl.c
> +++ b/vl.c
> @@ -4318,7 +4318,7 @@ int main(int argc, char **argv, char **envp)
>      if (vnc_display) {
>          Error *local_err = NULL;
>          vnc_display_init(ds);
> -        vnc_display_open(ds, vnc_display, &local_err);
> +        vnc_display_open(vnc_display, &local_err);
>          if (local_err != NULL) {

So why do you make the two display-initing functions assymetric?
Why one of them expects a `ds' argument (it is called from just
one place, and this is THE place), while you remove this arg from
another?

Maybe we should remove the ds arg from both (and clean up the
variable in question from main() too -- why it is declared at
the start of main() anyway, instead of this very place like
local_err is?)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/3] vnc: remove superfluous vnc_display_close()

[Michael Tokarev]

On 10/23/2014 09:39 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> vnc_display_close() is called in vnc_display_open().
> And in this moment, the ds variable is initilized
> at vnc_display_init(). Calling vnc_display_close()
> is superfluous.

Maybe the intent was to be able to open/close new vnc
displays in the future?  I think I've seen patches to
allow several vnc displays at once...

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/3] vnc: remove superfluous vnc_display_close()

[Gonglei]

On 2014/10/23 15:09, Michael Tokarev wrote:

> On 10/23/2014 09:39 AM, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> vnc_display_close() is called in vnc_display_open().
>> And in this moment, the ds variable is initilized
>> at vnc_display_init(). Calling vnc_display_close()
>> is superfluous.
> 
> Maybe the intent was to be able to open/close new vnc
> displays in the future?  I think I've seen patches to
> allow several vnc displays at once...
> 

I also saw Gerd's patch series for multiple vnc server instances.
But every vnc server has its own vnc status, they shouldn't
influence with each other. :) This logic is the same, initialize and
close immediately (the close process is superfluous).

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 3/3] vnc: remove superfluous DisplayState *ds parameter

[Gonglei]

On 2014/10/23 15:09, Michael Tokarev wrote:

> On 10/23/2014 09:39 AM, arei.gonglei@huawei.com wrote:
> []
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4318,7 +4318,7 @@ int main(int argc, char **argv, char **envp)
>>      if (vnc_display) {
>>          Error *local_err = NULL;
>>          vnc_display_init(ds);
>> -        vnc_display_open(ds, vnc_display, &local_err);
>> +        vnc_display_open(vnc_display, &local_err);
>>          if (local_err != NULL) {
> 
> So why do you make the two display-initing functions assymetric?
> Why one of them expects a `ds' argument (it is called from just
> one place, and this is THE place), while you remove this arg from
> another?
> 
> Maybe we should remove the ds arg from both (and clean up the
> variable in question from main() too -- why it is declared at
> the start of main() anyway, instead of this very place like
> local_err is?)
> 
Hmm. I notice Gerd's patch had done this clean work:

http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg01618.html

Maybe I should drop this patch. Thanks.

Best regards,
-Gonglei

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

[Gerd Hoffmann]

On Do, 2014-10-23 at 13:39 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Beside those problems, I also found another issue, see below pls.
> 
> Qemu command line:
> $ ./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive file=/home/redhat.img,if=none,id=drive-ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file=/mnt/sdb/gonglei/iso/rhel-server-7.0-x86_64-dvd.iso,if=none,id=drive-ide0-0-1 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,bootindex=4 -qmp unix:/tmp/qmp,server,nowait -monitor stdio -vnc 0.0.0.0:10,password,sasl
> QEMU 2.1.50 monitor - type 'help' for more information
> (qemu) change vnc password 123
> (qemu) change vnc abc
> error parsing address 'abc'     ---> an intended result because of I set a invalid value 'abc'
> (qemu) change vnc password 456
> If you want use passwords please enable password auth using '-vnc ${dpy},password'.Could not set password  --> Oops. I shouldn't get this output.
> (qemu) 
> 
> 
> 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"

cheers,
  Gerd

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

[Gonglei]

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

> On Do, 2014-10-23 at 13:39 +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Beside those problems, I also found another issue, see below pls.
>>
>> Qemu command line:
>> $ ./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive file=/home/redhat.img,if=none,id=drive-ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file=/mnt/sdb/gonglei/iso/rhel-server-7.0-x86_64-dvd.iso,if=none,id=drive-ide0-0-1 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,bootindex=4 -qmp unix:/tmp/qmp,server,nowait -monitor stdio -vnc 0.0.0.0:10,password,sasl
>> QEMU 2.1.50 monitor - type 'help' for more information
>> (qemu) change vnc password 123
>> (qemu) change vnc abc
>> error parsing address 'abc'     ---> an intended result because of I set a invalid value 'abc'
>> (qemu) change vnc password 456
>> If you want use passwords please enable password auth using '-vnc ${dpy},password'.Could not set password  --> Oops. I shouldn't get this output.
>> (qemu) 
>>
>>
>> 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?

Best regards,
-Gonglei

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

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