Re: [Xen-devel] [PATCH v5 05/25] xen/arm: check for multiboot nodes only under /chosen

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Julien Grall wrote:
> On 10/26/18 10:27 PM, Julien Grall wrote:
> > Hi,
> > 
> > On 10/26/18 10:12 PM, Stefano Stabellini wrote:
> > > On Fri, 26 Oct 2018, Julien Grall wrote:
> > > > Hi Stefano,
> > > > 
> > > > On 10/23/18 3:02 AM, Stefano Stabellini wrote:
> > > > > Make sure to only look for multiboot compatible nodes only under
> > > > > /chosen, not under any other paths (depth <= 3).
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v5:
> > > > > - add patch
> > > > > - add check on return value of fdt_get_path
> > > > > ---
> > > > >    xen/arch/arm/bootfdt.c | 13 ++++++++++---
> > > > >    1 file changed, 10 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > > > index 8eba42c..a314aca 100644
> > > > > --- a/xen/arch/arm/bootfdt.c
> > > > > +++ b/xen/arch/arm/bootfdt.c
> > > > > @@ -173,7 +173,14 @@ static void __init process_multiboot_node(const
> > > > > void
> > > > > *fdt, int node,
> > > > >        bootmodule_kind kind;
> > > > >        paddr_t start, size;
> > > > >        const char *cmdline;
> > > > > -    int len;
> > > > > +    int len = sizeof("/chosen");
> > > > 
> > > > NIT, I am not convince you win anything with that trick. strlen could be
> > > > optimized by the compiler (we use __builtin_strlen on Arm64).
> > > 
> > > I could use strlen if you prefer, but given that the string is known at
> > > compile time, it should hopefully be resolved to "8" at compile time
> > > using sizeof. What's wrong with it?
> > 
> > __builtin_strlen should get optimized at compile-time. Although, I don't
> > seem to be the case when I tried. Not sure why.
> > 
> > Never mind then.
> 
> Actually I just tried to use __builtin_stlren rather than strlen. And
> GCC is computing the size at compiler time.
> 
> Somehow, I thought arm64 was using __builtin_strlen but it seems to implement
> there own. Most likely we would want to use __builtin_strlen wor constant
> string. But that's another story.
> 
> In that case, I would prefer if you use __builtin_strlen. This is easier to
> understand over sizeof.

sizeof should be easier than __builtin_strlen for most: sizeof is part
of the C language, and it is taught in C classes. I expect everybody
should know what it is. While __builtin_strlen is a compiler construct,
which is certainly not for beginners. I think it is strange you find it
easier, probably a side of effect of years of Xen/kernel programming :-)

I would rather keep it as is.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel