[PATCH V3] xl: create VFB for PV guest when VNC is specified

[PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

This replicates a Xend behavior, when you specify:

  vnc=1
  vnclisten=XXXX
  vncpasswd=XXXX

in a PV guest's config file, it creates a VFB for you.

Fixes bug #25.
http://bugs.xenproject.org/xen/bug/25

Reported-by: Konrad Wilk <konrad.wilk@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in V3:
* ARRAY_EXTEND macro
* make parse_top_level_vnc_options a function

Changes in V2:
* use macros to reduce code duplication
* vfb=[] take precedence over top level VNC options
---
 tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bd26bcc..6a22e17 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
     return r;
 }
 
+#define ARRAY_EXTEND(array,count)                                       \
+    do {                                                                \
+        (array) = xrealloc((array),                                     \
+                           sizeof(typeof(*(array))) * ((count) + 1));   \
+        (count) = (count) + 1;                                          \
+    } while (0)
+
 #define LOG(_f, _a...)   dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
 
 static void dolog(const char *file, int line, const char *func, char *fmt, ...)
@@ -686,6 +693,22 @@ static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
     return rc;
 }
 
+static void parse_top_level_vnc_options(XLU_Config *config,
+                                        libxl_defbool *enable,
+                                        char **listen,
+                                        char **passwd,
+                                        int *display,
+                                        libxl_defbool *unused)
+{
+    long l;
+    xlu_cfg_get_defbool(config, "vnc", enable, 0);
+    xlu_cfg_replace_string (config, "vnclisten", listen, 0);
+    xlu_cfg_replace_string (config, "vncpasswd", passwd, 0);
+    if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
+        *display = l;
+    xlu_cfg_get_defbool(config, "vncunused", unused, 0);
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -1357,11 +1380,12 @@ skip_nic:
         fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not supported by xl\n");
     }
 
+    d_config->num_vfbs = 0;
+    d_config->num_vkbs = 0;
+    d_config->vfbs = NULL;
+    d_config->vkbs = NULL;
+
     if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
-        d_config->num_vfbs = 0;
-        d_config->num_vkbs = 0;
-        d_config->vfbs = NULL;
-        d_config->vkbs = NULL;
         while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
             libxl_device_vfb *vfb;
             libxl_device_vkb *vkb;
@@ -1369,13 +1393,13 @@ skip_nic:
             char *buf2 = strdup(buf);
             char *p, *p2;
 
-            d_config->vfbs = (libxl_device_vfb *) realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
-            vfb = d_config->vfbs + d_config->num_vfbs;
+            ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs);
+            vfb = d_config->vfbs + (d_config->num_vfbs - 1);
             libxl_device_vfb_init(vfb);
             vfb->devid = d_config->num_vfbs;
 
-            d_config->vkbs = (libxl_device_vkb *) realloc(d_config->vkbs, sizeof(libxl_device_vkb) * (d_config->num_vkbs + 1));
-            vkb = d_config->vkbs + d_config->num_vkbs;
+            ARRAY_EXTEND(d_config->vkbs, d_config->num_vkbs);
+            vkb = d_config->vkbs + (d_config->num_vkbs - 1);
             libxl_device_vkb_init(vkb);
             vkb->devid = d_config->num_vkbs;
 
@@ -1418,8 +1442,6 @@ skip_nic:
 
 skip_vfb:
             free(buf2);
-            d_config->num_vfbs++;
-            d_config->num_vkbs++;
         }
     }
 
@@ -1608,6 +1630,44 @@ skip_vfb:
 
 #undef parse_extra_args
 
+    /* If we've already got vfb=[] for PV guestthen ignore top level
+     * VNC config. */
+    if (c_info->type == LIBXL_DOMAIN_TYPE_PV && !d_config->num_vfbs) {
+        long vnc_enabled = 0;
+
+        if (!xlu_cfg_get_long (config, "vnc", &l, 0))
+            vnc_enabled = l;
+
+        if (vnc_enabled) {
+            libxl_device_vfb *vfb;
+            libxl_device_vkb *vkb;
+
+            ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs);
+            vfb = d_config->vfbs + (d_config->num_vfbs - 1);
+            libxl_device_vfb_init(vfb);
+            vfb->devid = d_config->num_vfbs;
+
+            ARRAY_EXTEND(d_config->vkbs, d_config->num_vkbs);
+            vkb = d_config->vkbs + (d_config->num_vkbs - 1);
+            libxl_device_vkb_init(vkb);
+            vkb->devid = d_config->num_vkbs;
+
+            parse_top_level_vnc_options(config,
+                                        &vfb->vnc.enable,
+                                        &vfb->vnc.listen,
+                                        &vfb->vnc.passwd,
+                                        &vfb->vnc.display,
+                                        &vfb->vnc.findunused);
+        }
+    } else {
+        parse_top_level_vnc_options(config,
+                                    &b_info->u.hvm.vnc.enable,
+                                    &b_info->u.hvm.vnc.listen,
+                                    &b_info->u.hvm.vnc.passwd,
+                                    &b_info->u.hvm.vnc.display,
+                                    &b_info->u.hvm.vnc.findunused);
+    }
+
     if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
             if (!strcmp(buf, "stdvga")) {
@@ -1622,15 +1682,6 @@ skip_vfb:
             b_info->u.hvm.vga.kind = l ? LIBXL_VGA_INTERFACE_TYPE_STD :
                                          LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
 
-        xlu_cfg_get_defbool(config, "vnc", &b_info->u.hvm.vnc.enable, 0);
-        xlu_cfg_replace_string (config, "vnclisten",
-                                &b_info->u.hvm.vnc.listen, 0);
-        xlu_cfg_replace_string (config, "vncpasswd",
-                                &b_info->u.hvm.vnc.passwd, 0);
-        if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
-            b_info->u.hvm.vnc.display = l;
-        xlu_cfg_get_defbool(config, "vncunused",
-                            &b_info->u.hvm.vnc.findunused, 0);
         xlu_cfg_replace_string (config, "keymap", &b_info->u.hvm.keymap, 0);
         xlu_cfg_get_defbool(config, "sdl", &b_info->u.hvm.sdl.enable, 0);
         xlu_cfg_get_defbool(config, "opengl", &b_info->u.hvm.sdl.opengl, 0);
-- 
1.7.10.4

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Mon, 2013-12-16 at 17:39 +0000, Wei Liu wrote:
> This replicates a Xend behavior, when you specify:
> 
>   vnc=1
>   vnclisten=XXXX
>   vncpasswd=XXXX
> 
> in a PV guest's config file, it creates a VFB for you.
> 
> Fixes bug #25.
> http://bugs.xenproject.org/xen/bug/25
> 
> Reported-by: Konrad Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> ---
> Changes in V3:
> * ARRAY_EXTEND macro
> * make parse_top_level_vnc_options a function
> 
> Changes in V2:
> * use macros to reduce code duplication
> * vfb=[] take precedence over top level VNC options
> ---
>  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index bd26bcc..6a22e17 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> +#define ARRAY_EXTEND(array,count)                                       \
> +    do {                                                                \
> +        (array) = xrealloc((array),                                     \
> +                           sizeof(typeof(*(array))) * ((count) + 1));   \

sizeof(typeof(*array)) == sizeof(*array), doesn't it?

> +        (count) = (count) + 1;                                          \

(count++)

I suppose in a macro like this it is tricky to arrange to only evaluate
the arguments once, since you need to read and write, but I think you
can arrange to evaluate array exactly once instead of 3 times and count
just once.

Perhaps Ian J has some hints.

> @@ -1369,13 +1393,13 @@ skip_nic:
>              char *buf2 = strdup(buf);
>              char *p, *p2;
>  
> -            d_config->vfbs = (libxl_device_vfb *) realloc(d_config->vfbs, sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> -            vfb = d_config->vfbs + d_config->num_vfbs;
> +            ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs);
> +            vfb = d_config->vfbs + (d_config->num_vfbs - 1);

You should make the macro return the new element like I suggested. Apart
from being convenient it avoids having to have this logic, and the
potential for an off-by-one error, repeated in every user.

> @@ -1608,6 +1630,44 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +    /* If we've already got vfb=[] for PV guestthen ignore top level

"guest then"

> +            parse_top_level_vnc_options(config,
> +                                        &vfb->vnc.enable,

This helper can take a libxl_vnc * instead of all of these, individual
fields. Pass it &vfb->vnc or &b_Info->u.hvm.vnc as appropriate.


Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Mon, Dec 16, 2013 at 05:46:04PM +0000, Ian Campbell wrote:
[...]
> > Changes in V2:
> > * use macros to reduce code duplication
> > * vfb=[] take precedence over top level VNC options
> > ---
> >  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 70 insertions(+), 19 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index bd26bcc..6a22e17 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
> >      return r;
> >  }
> >  
> > +#define ARRAY_EXTEND(array,count)                                       \
> > +    do {                                                                \
> > +        (array) = xrealloc((array),                                     \
> > +                           sizeof(typeof(*(array))) * ((count) + 1));   \
> 
> sizeof(typeof(*array)) == sizeof(*array), doesn't it?
> 
> > +        (count) = (count) + 1;                                          \
> 
> (count++)
> 
> I suppose in a macro like this it is tricky to arrange to only evaluate
> the arguments once, since you need to read and write, but I think you
> can arrange to evaluate array exactly once instead of 3 times and count
> just once.
> 

I came up with this. Is it what you asked for?

#define ARRAY_EXTEND(type_ptr,ptr,type_count,count,ret)                 \
    do {                                                                \
        type_ptr **__ptr = &(ptr);                                      \
        type_count *__count = &(count);                                 \
        *__ptr = xrealloc(*__ptr,                                       \
                          sizeof(**__ptr) * (*__count + 1));            \
        (ret) = *__ptr + *__count;                                      \
        (*__count)++;                                                   \
    } while (0)

Wei.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Andrew Cooper]

On 17/12/13 11:28, Wei Liu wrote:
> On Mon, Dec 16, 2013 at 05:46:04PM +0000, Ian Campbell wrote:
> [...]
>>> Changes in V2:
>>> * use macros to reduce code duplication
>>> * vfb=[] take precedence over top level VNC options
>>> ---
>>>  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 70 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>> index bd26bcc..6a22e17 100644
>>> --- a/tools/libxl/xl_cmdimpl.c
>>> +++ b/tools/libxl/xl_cmdimpl.c
>>> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
>>>      return r;
>>>  }
>>>  
>>> +#define ARRAY_EXTEND(array,count)                                       \
>>> +    do {                                                                \
>>> +        (array) = xrealloc((array),                                     \
>>> +                           sizeof(typeof(*(array))) * ((count) + 1));   \
>> sizeof(typeof(*array)) == sizeof(*array), doesn't it?
>>
>>> +        (count) = (count) + 1;                                          \
>> (count++)
>>
>> I suppose in a macro like this it is tricky to arrange to only evaluate
>> the arguments once, since you need to read and write, but I think you
>> can arrange to evaluate array exactly once instead of 3 times and count
>> just once.
>>
> I came up with this. Is it what you asked for?
>
> #define ARRAY_EXTEND(type_ptr,ptr,type_count,count,ret)                 \
>     do {                                                                \
>         type_ptr **__ptr = &(ptr);                                      \
>         type_count *__count = &(count);                                 \
>         *__ptr = xrealloc(*__ptr,                                       \
>                           sizeof(**__ptr) * (*__count + 1));            \
>         (ret) = *__ptr + *__count;                                      \
>         (*__count)++;                                                   \
>     } while (0)
>
> Wei.

Use typeof() rather than passing the types in directly.  It is a GCC
extension, which tries its best not to evaluate its argument.

Alternatively, the __auto_type gcc keyword sould seem to do what you
want as well.

~Andrew

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Tue, Dec 17, 2013 at 11:36:29AM +0000, Andrew Cooper wrote:
> On 17/12/13 11:28, Wei Liu wrote:
> > On Mon, Dec 16, 2013 at 05:46:04PM +0000, Ian Campbell wrote:
> > [...]
> >>> Changes in V2:
> >>> * use macros to reduce code duplication
> >>> * vfb=[] take precedence over top level VNC options
> >>> ---
> >>>  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
> >>>  1 file changed, 70 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >>> index bd26bcc..6a22e17 100644
> >>> --- a/tools/libxl/xl_cmdimpl.c
> >>> +++ b/tools/libxl/xl_cmdimpl.c
> >>> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
> >>>      return r;
> >>>  }
> >>>  
> >>> +#define ARRAY_EXTEND(array,count)                                       \
> >>> +    do {                                                                \
> >>> +        (array) = xrealloc((array),                                     \
> >>> +                           sizeof(typeof(*(array))) * ((count) + 1));   \
> >> sizeof(typeof(*array)) == sizeof(*array), doesn't it?
> >>
> >>> +        (count) = (count) + 1;                                          \
> >> (count++)
> >>
> >> I suppose in a macro like this it is tricky to arrange to only evaluate
> >> the arguments once, since you need to read and write, but I think you
> >> can arrange to evaluate array exactly once instead of 3 times and count
> >> just once.
> >>
> > I came up with this. Is it what you asked for?
> >
> > #define ARRAY_EXTEND(type_ptr,ptr,type_count,count,ret)                 \
> >     do {                                                                \
> >         type_ptr **__ptr = &(ptr);                                      \
> >         type_count *__count = &(count);                                 \
> >         *__ptr = xrealloc(*__ptr,                                       \
> >                           sizeof(**__ptr) * (*__count + 1));            \
> >         (ret) = *__ptr + *__count;                                      \
> >         (*__count)++;                                                   \
> >     } while (0)
> >
> > Wei.
> 
> Use typeof() rather than passing the types in directly.  It is a GCC
> extension, which tries its best not to evaluate its argument.
> 

Oh I thought typeof((count)) always evaluates its argument, good to
know. ;-)

Wei.

> Alternatively, the __auto_type gcc keyword sould seem to do what you
> want as well.
> 
> ~Andrew

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Tue, 2013-12-17 at 11:40 +0000, Wei Liu wrote:
> On Tue, Dec 17, 2013 at 11:36:29AM +0000, Andrew Cooper wrote:
> > On 17/12/13 11:28, Wei Liu wrote:
> > > On Mon, Dec 16, 2013 at 05:46:04PM +0000, Ian Campbell wrote:
> > > [...]
> > >>> Changes in V2:
> > >>> * use macros to reduce code duplication
> > >>> * vfb=[] take precedence over top level VNC options
> > >>> ---
> > >>>  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
> > >>>  1 file changed, 70 insertions(+), 19 deletions(-)
> > >>>
> > >>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >>> index bd26bcc..6a22e17 100644
> > >>> --- a/tools/libxl/xl_cmdimpl.c
> > >>> +++ b/tools/libxl/xl_cmdimpl.c
> > >>> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
> > >>>      return r;
> > >>>  }
> > >>>  
> > >>> +#define ARRAY_EXTEND(array,count)                                       \
> > >>> +    do {                                                                \
> > >>> +        (array) = xrealloc((array),                                     \
> > >>> +                           sizeof(typeof(*(array))) * ((count) + 1));   \
> > >> sizeof(typeof(*array)) == sizeof(*array), doesn't it?
> > >>
> > >>> +        (count) = (count) + 1;                                          \
> > >> (count++)
> > >>
> > >> I suppose in a macro like this it is tricky to arrange to only evaluate
> > >> the arguments once, since you need to read and write, but I think you
> > >> can arrange to evaluate array exactly once instead of 3 times and count
> > >> just once.
> > >>
> > > I came up with this. Is it what you asked for?
> > >
> > > #define ARRAY_EXTEND(type_ptr,ptr,type_count,count,ret)                 \
> > >     do {                                                                \
> > >         type_ptr **__ptr = &(ptr);                                      \
> > >         type_count *__count = &(count);                                 \
> > >         *__ptr = xrealloc(*__ptr,                                       \
> > >                           sizeof(**__ptr) * (*__count + 1));            \
> > >         (ret) = *__ptr + *__count;                                      \
> > >         (*__count)++;                                                   \
> > >     } while (0)
> > >
> > > Wei.
> > 
> > Use typeof() rather than passing the types in directly.  It is a GCC
> > extension, which tries its best not to evaluate its argument.
> > 
> 
> Oh I thought typeof((count)) always evaluates its argument, good to
> know. ;-)

Me too, which is why I didn't suggest it earlier.

In hindsight its main use is probably in making macros only evaluate
once, so if it itself did the evaluation it wouldn't be very useful!

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Tue, Dec 17, 2013 at 11:41:29AM +0000, Ian Campbell wrote:
> On Tue, 2013-12-17 at 11:40 +0000, Wei Liu wrote:
> > On Tue, Dec 17, 2013 at 11:36:29AM +0000, Andrew Cooper wrote:
> > > On 17/12/13 11:28, Wei Liu wrote:
> > > > On Mon, Dec 16, 2013 at 05:46:04PM +0000, Ian Campbell wrote:
> > > > [...]
> > > >>> Changes in V2:
> > > >>> * use macros to reduce code duplication
> > > >>> * vfb=[] take precedence over top level VNC options
> > > >>> ---
> > > >>>  tools/libxl/xl_cmdimpl.c |   89 ++++++++++++++++++++++++++++++++++++----------
> > > >>>  1 file changed, 70 insertions(+), 19 deletions(-)
> > > >>>
> > > >>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > >>> index bd26bcc..6a22e17 100644
> > > >>> --- a/tools/libxl/xl_cmdimpl.c
> > > >>> +++ b/tools/libxl/xl_cmdimpl.c
> > > >>> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
> > > >>>      return r;
> > > >>>  }
> > > >>>  
> > > >>> +#define ARRAY_EXTEND(array,count)                                       \
> > > >>> +    do {                                                                \
> > > >>> +        (array) = xrealloc((array),                                     \
> > > >>> +                           sizeof(typeof(*(array))) * ((count) + 1));   \
> > > >> sizeof(typeof(*array)) == sizeof(*array), doesn't it?
> > > >>
> > > >>> +        (count) = (count) + 1;                                          \
> > > >> (count++)
> > > >>
> > > >> I suppose in a macro like this it is tricky to arrange to only evaluate
> > > >> the arguments once, since you need to read and write, but I think you
> > > >> can arrange to evaluate array exactly once instead of 3 times and count
> > > >> just once.
> > > >>
> > > > I came up with this. Is it what you asked for?
> > > >
> > > > #define ARRAY_EXTEND(type_ptr,ptr,type_count,count,ret)                 \
> > > >     do {                                                                \
> > > >         type_ptr **__ptr = &(ptr);                                      \
> > > >         type_count *__count = &(count);                                 \
> > > >         *__ptr = xrealloc(*__ptr,                                       \
> > > >                           sizeof(**__ptr) * (*__count + 1));            \
> > > >         (ret) = *__ptr + *__count;                                      \
> > > >         (*__count)++;                                                   \
> > > >     } while (0)
> > > >
> > > > Wei.
> > > 
> > > Use typeof() rather than passing the types in directly.  It is a GCC
> > > extension, which tries its best not to evaluate its argument.
> > > 
> > 
> > Oh I thought typeof((count)) always evaluates its argument, good to
> > know. ;-)
> 
> Me too, which is why I didn't suggest it earlier.
> 
> In hindsight its main use is probably in making macros only evaluate
> once, so if it itself did the evaluation it wouldn't be very useful!
> 

So now the macro becomes:

#define ARRAY_EXTEND(ptr,count,ret)                                     \
    do {                                                                \
        typeof((ptr)) *__ptr = &(ptr);                                  \
        typeof((count)) *__count = &(count);                            \
        *__ptr = xrealloc(*__ptr,                                       \
                          sizeof(**__ptr) * (*__count + 1));            \
        (ret) = *__ptr + *__count;                                      \
        (*__count)++;                                                   \
    } while (0)

Thoughts?

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> On Mon, 2013-12-16 at 17:39 +0000, Wei Liu wrote:
> > +        (count) = (count) + 1;                                          \
> 
> (count++)

You mean
  (count)++

> I suppose in a macro like this it is tricky to arrange to only evaluate
> the arguments once, since you need to read and write, but I think you
> can arrange to evaluate array exactly once instead of 3 times and count
> just once.

I think it is fine for a macro to evaluate its arguments as many times
as it feels like.  That's one thing the SHOUTING is there to warn yo
about.

I agree with the rest of Ian C's comments.

Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> On Tue, Dec 17, 2013 at 11:41:29AM +0000, Ian Campbell wrote:
...
> > In hindsight its main use is probably in making macros only evaluate
> > once, so if it itself did the evaluation it wouldn't be very useful!
> 
> So now the macro becomes:
> 
> #define ARRAY_EXTEND(ptr,count,ret)                                     \
>     do {                                                                \
>         typeof((ptr)) *__ptr = &(ptr);                                  \
>         typeof((count)) *__count = &(count);                            \
>         *__ptr = xrealloc(*__ptr,                                       \
>                           sizeof(**__ptr) * (*__count + 1));            \
>         (ret) = *__ptr + *__count;                                      \
>         (*__count)++;                                                   \
>     } while (0)
> 
> Thoughts?

I think all of this complexity to avoid evaluating the array and count
arguments more than once is unnecessary.  Particularly when they're
both going to be updated.

But if you are doing to do that, you need to give your local variables
different names.  Firstly: names starting __ are reserved for the C
implementation and shouldn't be used for anything at all.  Secondly,
prefixing the names with something like "_" isn't sufficient in the
general case and is the wrong habit.  If you need to define a local
variable in a macro you should decorate it with something related to
the name of the macro.  (Hopefully no-one will invoke the same macro
in one of its own arguments.)

Please make the macro return the new array entry as Ian C suggested.

Finally: IMO the macro should probably initialise the new element
itself.  So it will be more than just "ARRAY_EXTEND" because it will
have some knowledge of the kind of array it is, and might need to be
renamed.  Ian C, what do you think ?

Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Tue, Dec 17, 2013 at 03:02:07PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> > On Tue, Dec 17, 2013 at 11:41:29AM +0000, Ian Campbell wrote:
> ...
> > > In hindsight its main use is probably in making macros only evaluate
> > > once, so if it itself did the evaluation it wouldn't be very useful!
> > 
> > So now the macro becomes:
> > 
> > #define ARRAY_EXTEND(ptr,count,ret)                                     \
> >     do {                                                                \
> >         typeof((ptr)) *__ptr = &(ptr);                                  \
> >         typeof((count)) *__count = &(count);                            \
> >         *__ptr = xrealloc(*__ptr,                                       \
> >                           sizeof(**__ptr) * (*__count + 1));            \
> >         (ret) = *__ptr + *__count;                                      \
> >         (*__count)++;                                                   \
> >     } while (0)
> > 
> > Thoughts?
> 
> I think all of this complexity to avoid evaluating the array and count
> arguments more than once is unnecessary.  Particularly when they're
> both going to be updated.
> 
> But if you are doing to do that, you need to give your local variables
> different names.  Firstly: names starting __ are reserved for the C
> implementation and shouldn't be used for anything at all.  Secondly,
> prefixing the names with something like "_" isn't sufficient in the
> general case and is the wrong habit.  If you need to define a local
> variable in a macro you should decorate it with something related to
> the name of the macro.  (Hopefully no-one will invoke the same macro
> in one of its own arguments.)
> 
> Please make the macro return the new array entry as Ian C suggested.
> 

OK.

> Finally: IMO the macro should probably initialise the new element
> itself.  So it will be more than just "ARRAY_EXTEND" because it will
> have some knowledge of the kind of array it is, and might need to be
> renamed.  Ian C, what do you think ?
> 

Huh? Elements are initialized by different funcitons. You mean I need to
add extra argument to the macro to pass in the initilization function?

Wei.

> Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> On Tue, Dec 17, 2013 at 03:02:07PM +0000, Ian Jackson wrote:
> > Finally: IMO the macro should probably initialise the new element
> > itself.  So it will be more than just "ARRAY_EXTEND" because it will
> > have some knowledge of the kind of array it is, and might need to be
> > renamed.  Ian C, what do you think ?
> 
> Huh? Elements are initialized by different funcitons. You mean I need to
> add extra argument to the macro to pass in the initilization function?

Either that, or pass in just the core of the token eg "vkb" and use
token pasting to automatically create the right function name.

But please wait to see what Ian C says as he may disagree with me.

Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Campbell]

On Tue, 2013-12-17 at 16:15 +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> > On Tue, Dec 17, 2013 at 03:02:07PM +0000, Ian Jackson wrote:
> > > Finally: IMO the macro should probably initialise the new element
> > > itself.  So it will be more than just "ARRAY_EXTEND" because it will
> > > have some knowledge of the kind of array it is, and might need to be
> > > renamed.  Ian C, what do you think ?
> > 
> > Huh? Elements are initialized by different funcitons. You mean I need to
> > add extra argument to the macro to pass in the initilization function?
> 
> Either that, or pass in just the core of the token eg "vkb" and use
> token pasting to automatically create the right function name.
> 
> But please wait to see what Ian C says as he may disagree with me.

I'm happy either way. A more helpful macro is good but it is perhaps
getting a bit complex, so I wouldn't blame Wei for wanting to keep it
simpler by doing the init afterwards.

There's also an argument that this should be functionality provided in
helpers generated by the IDL, but that certainly isn't 4.4 material...

Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Tue, Dec 17, 2013 at 04:40:46PM +0000, Ian Campbell wrote:
> On Tue, 2013-12-17 at 16:15 +0000, Ian Jackson wrote:
> > Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> > > On Tue, Dec 17, 2013 at 03:02:07PM +0000, Ian Jackson wrote:
> > > > Finally: IMO the macro should probably initialise the new element
> > > > itself.  So it will be more than just "ARRAY_EXTEND" because it will
> > > > have some knowledge of the kind of array it is, and might need to be
> > > > renamed.  Ian C, what do you think ?
> > > 
> > > Huh? Elements are initialized by different funcitons. You mean I need to
> > > add extra argument to the macro to pass in the initilization function?
> > 
> > Either that, or pass in just the core of the token eg "vkb" and use
> > token pasting to automatically create the right function name.
> > 
> > But please wait to see what Ian C says as he may disagree with me.
> 
> I'm happy either way. A more helpful macro is good but it is perhaps
> getting a bit complex, so I wouldn't blame Wei for wanting to keep it
> simpler by doing the init afterwards.
> 
> There's also an argument that this should be functionality provided in
> helpers generated by the IDL, but that certainly isn't 4.4 material...
> 

OK. Now it looks like this.

#define ARRAY_EXTEND_INIT(ptr,count,name)                               \
    ({                                                                  \
        typeof((ptr)) *_xl_array_ptr = &(ptr);                          \
        typeof((count)) *_xl_array_count = &(count);                    \
        typeof((count)) _xl_array_count_saved = *_xl_array_count;       \
        *_xl_array_ptr = xrealloc(*_xl_array_ptr,                       \
                                  sizeof(**_xl_array_ptr) *             \
                                  (*_xl_array_count + 1));              \
        (*_xl_array_count)++;                                           \
        libxl_device_##name##_init(*_xl_array_ptr + _xl_array_count_saved); \
        (*_xl_array_ptr + _xl_array_count_saved);                       \
    })

It 1) only evaluates its arguments once, 2) initializes the new element
with corresponding function, 3) returns the new element.

Longer local variables names are chosen in the hope that they don't
clash with other names...

Wei.

> Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified"):
> On Tue, Dec 17, 2013 at 04:40:46PM +0000, Ian Campbell wrote:
> > There's also an argument that this should be functionality provided in
> > helpers generated by the IDL, but that certainly isn't 4.4 material...
> 
> OK. Now it looks like this.
> 
> #define ARRAY_EXTEND_INIT(ptr,count,name)                               \
>     ({                                                                  \
>         typeof((ptr)) *_xl_array_ptr = &(ptr);                          \
>         typeof((count)) *_xl_array_count = &(count);                    \
>         typeof((count)) _xl_array_count_saved = *_xl_array_count;       \
>         *_xl_array_ptr = xrealloc(*_xl_array_ptr,                       \
>                                   sizeof(**_xl_array_ptr) *             \
>                                   (*_xl_array_count + 1));              \
>         (*_xl_array_count)++;                                           \
>         libxl_device_##name##_init(*_xl_array_ptr + _xl_array_count_saved); \
>         (*_xl_array_ptr + _xl_array_count_saved);                       \
>     })
> 
> It 1) only evaluates its arguments once, 2) initializes the new element
> with corresponding function, 3) returns the new element.

I still think the bikeshed is the wrong colour and the result is very
ugly, but this will do.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Here's my real opinion:

If you are going to only use "name" once I don't think the token
pasting is justified.  It would be justified if you used "name" to
automatically compute the right expression for ptr and count, but I
can see why you didn't want to do this.

> Longer local variables names are chosen in the hope that they don't
> clash with other names...

I realise you're copying what other people have done, but there is a
lot of cargo cult superstition about what needs to be, and what
doesn't need to be, in these kind of macro local variable names.

The leading _ is pretty pointless.  The "xl" is pointless too because
these are inside the code for xl, so only other code inside xl will
see them.

I would call these variables  array_extend_ptr  et al.

As I've said I think evaluating the arguments multiple times is
better.  After all they have to be lvalues.

So I would do:

 #define ARRAY_EXTEND_INIT(array,count,initfn)                       \
     ({                                                              \
         typeof((count)) array_extend_old_count = (count);           \
         (count)++;                                                  \
         (array) = xrealloc((array), sizeof(*array) * (count));      \
         (initfn)(&(array)[array_extend_old_count]);                 \
         &(array)[array_extend_old_count];                           \
     })

 #define ADD_VDEV(container,things,initfn)                \
     ARRAY_EXTEND_INIT((container)->things,               \
                       (container)->num_##things,         \
                       (initfn))
 ...

     vtpm = ADD_VDEV(d_config, vtpms, libxl_device_vtpm_init);
     vtpm->devid = d_config->num_vtpms;

(not even compiled by me, obviously)

Sadly you can't generate "init" from "kind" because
libxl_device_pci_init but d_config->pcidevs.

Thanks,
Ian.

Re: [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Wei Liu]

On Tue, Dec 17, 2013 at 06:04:58PM +0000, Ian Jackson wrote:
[...]
> As I've said I think evaluating the arguments multiple times is
> better.  After all they have to be lvalues.
> 
> So I would do:
> 
>  #define ARRAY_EXTEND_INIT(array,count,initfn)                       \
>      ({                                                              \
>          typeof((count)) array_extend_old_count = (count);           \
>          (count)++;                                                  \
>          (array) = xrealloc((array), sizeof(*array) * (count));      \
>          (initfn)(&(array)[array_extend_old_count]);                 \
>          &(array)[array_extend_old_count];                           \
>      })
> 

I will take this macro. I don't really have strong opinion on macro
tricks. If you think this is better so be it. The way this macro is
written does affect the core fuctionality of this patch anyway. :-)

Wei.