Xen devel - Mar 2013 - [PATCH] tools/xenconsoled: Initialise static data before use

''fds'' and ''current_array_size'' are used in a
memset() in reset_fds(), and for
the first call to set_fds() before being initialised.

Also initialise nr_fds for sanity sake.

This is another regression introduced by

"Switch from select() to poll() in xenconsoled''s IO loop."
  hg c/s 26405:7359c3122c5d
  git cc5434c933153c4b8812d1df901f8915c22830a8

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Marcus Granado <marcus.granado@citrix.com>

---

This is still not enough to fix the issue of xenconsoled exiting after 384
VMs, but does allow us to reliably reach the 383rd VM.

diff -r 94ece33caae2 -r 803a5869bfb5 tools/console/daemon/io.c
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -70,9 +70,9 @@ static int log_hv_fd = -1;
 
 static xc_gnttab *xcg_handle = NULL;
 
-static struct pollfd  *fds;
-static unsigned int current_array_size;
-static unsigned int nr_fds;
+static struct pollfd  *fds = NULL;
+static unsigned int current_array_size = 0;
+static unsigned int nr_fds = 0;
 
 #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) &
~((1UL<<(_w))-1))

On 07/03/13 18:45, Andrew Cooper wrote:
> ''fds'' and ''current_array_size'' are used
in a memset() in reset_fds(), and for
> the first call to set_fds() before being initialised.
>
> Also initialise nr_fds for sanity sake.
>
> This is another regression introduced by
>
> "Switch from select() to poll() in xenconsoled''s IO
loop."
>   hg c/s 26405:7359c3122c5d
>   git cc5434c933153c4b8812d1df901f8915c22830a8
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Marcus Granado <marcus.granado@citrix.com>
>
> ---
>
> This is still not enough to fix the issue of xenconsoled exiting after 384
> VMs, but does allow us to reliably reach the 383rd VM.
On second thoughts, memset(NULL, 0, 0); is still silly.   I shall resin
tomorrow.

~Andrew
>
> diff -r 94ece33caae2 -r 803a5869bfb5 tools/console/daemon/io.c
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -70,9 +70,9 @@ static int log_hv_fd = -1;
>  
>  static xc_gnttab *xcg_handle = NULL;
>  
> -static struct pollfd  *fds;
> -static unsigned int current_array_size;
> -static unsigned int nr_fds;
> +static struct pollfd  *fds = NULL;
> +static unsigned int current_array_size = 0;
> +static unsigned int nr_fds = 0;
>  
>  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) &
~((1UL<<(_w))-1))
>  
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Andrew Cooper writes ("[PATCH] tools/xenconsoled: Initialise static data
before use"):
> ''fds'' and ''current_array_size'' are used
in a memset() in reset_fds(), and for
> the first call to set_fds() before being initialised.
> 
> Also initialise nr_fds for sanity sake.
These variables have static storage duration and are therefore
initialised to 0.

Ian.

On Thu, 2013-03-07 at 18:54 +0000, Andrew Cooper wrote:
> On 07/03/13 18:45, Andrew Cooper wrote:
> > ''fds'' and ''current_array_size'' are
used in a memset() in reset_fds(), and for
> > the first call to set_fds() before being initialised.
> >
> > Also initialise nr_fds for sanity sake.
> >
> > This is another regression introduced by
> >
> > "Switch from select() to poll() in xenconsoled''s IO
loop."
> >   hg c/s 26405:7359c3122c5d
> >   git cc5434c933153c4b8812d1df901f8915c22830a8
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Marcus Granado <marcus.granado@citrix.com>
> >
> > ---
> >
> > This is still not enough to fix the issue of xenconsoled exiting after
384
> > VMs, but does allow us to reliably reach the 383rd VM.
> 
> On second thoughts, memset(NULL, 0, 0); is still silly.   I shall resin
> tomorrow.
> 
I googled "memset NULL", presumably this is undefined and should be
fix. :-(

You fix here is quite odd, because these static variables should be
initialized to 0 automatically. Is this something related to compiler /
CFLAGS settings that trigger the bug?

I just tested booting up >384 mini-os, with you previous patch applied,
everything worked fine. I''m setting up LVM on my test box to test with
normal Linux guests.

A hint on "128", set_fds expands the array on a multiple of 128.


Wei.
> ~Andrew
> 
> >
> > diff -r 94ece33caae2 -r 803a5869bfb5 tools/console/daemon/io.c
> > --- a/tools/console/daemon/io.c
> > +++ b/tools/console/daemon/io.c
> > @@ -70,9 +70,9 @@ static int log_hv_fd = -1;
> >  
> >  static xc_gnttab *xcg_handle = NULL;
> >  
> > -static struct pollfd  *fds;
> > -static unsigned int current_array_size;
> > -static unsigned int nr_fds;
> > +static struct pollfd  *fds = NULL;
> > +static unsigned int current_array_size = 0;
> > +static unsigned int nr_fds = 0;
> >  
> >  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1)
& ~((1UL<<(_w))-1))
> >  
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>

On 07/03/13 18:59, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/xenconsoled: Initialise static
data before use"):
>> ''fds'' and ''current_array_size'' are
used in a memset() in reset_fds(), and for
>> the first call to set_fds() before being initialised.
>>
>> Also initialise nr_fds for sanity sake.
> These variables have static storage duration and are therefore
> initialised to 0.
>
> Ian.
D''oh - I was not completely sure about this, double checked online and
managed to come up with the wrong answer.

My apologies.

The more concerning point is now "why does this appear to make a
difference"  I shall continue debugging.

~Andrew