[PATCH] libxl: fix against if condition

[PATCH] libxl: fix against if condition

[ZhouPeng]

[-- Attachment #1.1: Type: text/plain, Size: 682 bytes --]

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

--- a/tools/libxl/libxl_dm.c    Wed Apr 13 14:08:41 2011 +0800
+++ b/tools/libxl/libxl_dm.c    Wed Apr 13 14:20:37 2011 +0800
@@ -197,7 +197,7 @@ static char ** libxl_build_device_model_
         int display = 0;
         const char *listen = "127.0.0.1";

-        if (info->vncpasswd && info->vncpasswd[0]) {
+        if (info->vncpasswd && !info->vncpasswd[0]) {
             assert(!"missing code for supplying vnc password to qemu");
         }
         flexarray_append(dm_args, "-vnc");

-- 
Zhou Peng
Operating System Technology Group
Institute of Software, the Chinese Academy of Sciences (ISCAS)
<ailvpeng25@gmail.com>

[-- Attachment #1.2: Type: text/html, Size: 894 bytes --]

[-- Attachment #2: libxl-against-if-condition.patch --]
[-- Type: text/x-patch, Size: 549 bytes --]

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

--- a/tools/libxl/libxl_dm.c    Wed Apr 13 14:08:41 2011 +0800
+++ b/tools/libxl/libxl_dm.c    Wed Apr 13 14:20:37 2011 +0800
@@ -197,7 +197,7 @@ static char ** libxl_build_device_model_
         int display = 0;
         const char *listen = "127.0.0.1";
 
-        if (info->vncpasswd && info->vncpasswd[0]) {
+        if (info->vncpasswd && !info->vncpasswd[0]) {
             assert(!"missing code for supplying vnc password to qemu");
         }
         flexarray_append(dm_args, "-vnc");


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: fix against if condition

[Ian Jackson]

ZhouPeng writes ("[Xen-devel] [PATCH] libxl: fix against if condition"):
> Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>
> 
> --- a/tools/libxl/libxl_dm.c    Wed Apr 13 14:08:41 2011 +0800
> +++ b/tools/libxl/libxl_dm.c    Wed Apr 13 14:20:37 2011 +0800
> @@ -197,7 +197,7 @@ static char ** libxl_build_device_model_
>          int display = 0;
>          const char *listen = "127.0.0.1";
> 
> -        if (info->vncpasswd && info->vncpasswd[0]) {
> +        if (info->vncpasswd && !info->vncpasswd[0]) {
>              assert(!"missing code for supplying vnc password to qemu");
>          }
>          flexarray_append(dm_args, "-vnc");

Looking just at the diff there, I think the original code is correct.
That is, the assert trips when:
  * a password is supplied (info->vncpasswd != NULL)
  * the password is nonemtpy (strlen(info->vncpasswd) > 0
      which is the same as !!info->vncpasswd[0]
  * we're using a new qemu for which this code has not been implemented

That the failure manifests as an assert might be argued to be
unfortunate, but this is still code under development...

Ian.

Re: [PATCH] libxl: fix against if condition

[ZhouPeng]

[-- Attachment #1.1: Type: text/plain, Size: 1992 bytes --]

2011/4/15 Ian Jackson <Ian.Jackson@eu.citrix.com>

> ZhouPeng writes ("[Xen-devel] [PATCH] libxl: fix against if condition"):
> > Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>
> >
> > --- a/tools/libxl/libxl_dm.c    Wed Apr 13 14:08:41 2011 +0800
> > +++ b/tools/libxl/libxl_dm.c    Wed Apr 13 14:20:37 2011 +0800
> > @@ -197,7 +197,7 @@ static char ** libxl_build_device_model_
> >          int display = 0;
> >          const char *listen = "127.0.0.1";
> >
> > -        if (info->vncpasswd && info->vncpasswd[0]) {
> > +        if (info->vncpasswd && !info->vncpasswd[0]) {
> >              assert(!"missing code for supplying vnc password to qemu");
>
  assert( ) here is missleading, I think.
  In my understanding, assert is offten used to help program debuging,
  telling user the program is runing in exception here and abort it.
  So, I just think "missing code" means "missing passwd code", which tell
user must supply an non-empty vnc passwd,
  if vncpasswd is used in vm cfg file.
  It's hard for me to give assert() an relationship with a comment telling
something in development, :)
  I think an comment like TODO may be more clear,

  Thanks.

> >          }
> >          flexarray_append(dm_args, "-vnc");
>
> Looking just at the diff there, I think the original code is correct.
> That is, the assert trips when:
>  * a password is supplied (info->vncpasswd != NULL)
>  * the password is nonemtpy (strlen(info->vncpasswd) > 0
>      which is the same as !!info->vncpasswd[0]
>  * we're using a new qemu for which this code has not been implemented
>
> That the failure manifests as an assert might be argued to be
> unfortunate, but this is still code under development...
>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>



-- 
Zhou Peng
Operating System Technology Group
Institute of Software, the Chinese Academy of Sciences (ISCAS)
<ailvpeng25@gmail.com>

[-- Attachment #1.2: Type: text/html, Size: 2969 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: fix against if condition

[Ian Jackson]

ZhouPeng writes ("Re: [Xen-devel] [PATCH] libxl: fix against if condition"):
>   assert( ) here is missleading, I think.
>   In my understanding, assert is offten used to help program debuging,

This code is very much under development, and the lack of support for
vnc passwords is pretty serious problem IMO.

>   telling user the program is runing in exception here and abort it.
>   So, I just think "missing code" means "missing passwd code", which tell
> user must supply an non-empty vnc passwd,

No, the user may *not* supply a non-empty password if they want it to
work.

>   I think an comment like TODO may be more clear,

Certainly the code should fail.  If you supply a good patch which
replaces the assert with a properly logged error return I'll apply it.

But it would be better to implement the missing feature.

Ian.