Re: [PATCH v3 1/3] xen: add warning infrastructure

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Jun 22, 2016 at 09:35:51AM -0600, Jan Beulich wrote:
> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote:
> > @@ -1582,6 +1583,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  
> >      init_constructors();
> >  
> > +    warning_print();
> > +
> >      console_endboot();
> 
> What about an ARM equivalent? Perhaps put this in console_endboot()?
> 

Fine with putting this in console_endboot.

> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -59,6 +59,7 @@ obj-y += vm_event.o
> >  obj-y += vmap.o
> >  obj-y += vsprintf.o
> >  obj-y += wait.o
> > +obj-y += warning.o
> 
> And perhaps better to put the new code into the console code too?
> In any event if you want to keep this in a separate file, considering
> that all of it is code/data contributions to .init.*, this wants to be
> 

I will keep a separate file, and ...

> obj-bin-y += warning.init.o
> 

... use this.

> so that namely string literals get moved to .init.* too.
> 
> > --- /dev/null
> > +++ b/xen/common/warning.c
> > @@ -0,0 +1,50 @@
> > +#include <xen/delay.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/softirq.h>
> > +#include <xen/warning.h>
> > +
> > +#define WARNING_ARRAY_SIZE 20
> > +static unsigned int __initdata nr_warnings;
> > +static const char __initdata *warnings[WARNING_ARRAY_SIZE];
> 
> The __initdata belongs after the *.
> 

Fixed.

> > +void __init warning_add(const char *warning)
> > +{
> > +    if ( nr_warnings >= WARNING_ARRAY_SIZE )
> > +        panic("Too many pieces of warning text.");
> 
> panic() seems a bit harsh, but this shouldn't trigger anyway.
> 
> > +    warnings[nr_warnings] = warning;
> > +    nr_warnings++;
> > +}
> > +
> > +void __init warning_print(void)
> > +{
> > +    unsigned int i, j;
> > +
> > +    if ( !nr_warnings )
> > +        return;
> 
> Perhaps a single instance of the ***** separators could be printed
> here ...
> 
> > +    for ( i = 0; i < nr_warnings; i++ )
> > +        printk("%s", warnings[i]);
> 
> ... and here, avoiding each caller to add such?
> 

The warning text (multiple lines) is added as one single string, which
means we can't trivially add leading stars at the beginning of each
line.

I don't feel like arguing over how the text would look like, so if
something like:

    ************************************************
    WARNING 1
    ************************************************
    WARNING 2
    ************************************************
    ...
    ************************************************
    WARNING N
    ************************************************

is ok to you, I'm fine with using that format, too.

Wei.

> Jan
> 

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