Re: [Xen-devel] [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH v3 14/14] libxl: build a device tree for ARM 
guests"):
> On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> > > +    if (fdt_size) {
> > > +        fdt_size <<= 1;
> > > +        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> > > +    } else {
> > > +        fdt_size = 4096;
> > > +    }
> > 
> > Can we rule out this loop getting out of control ?
> 
> The FDT macro has a check and error if it gets too big. I'll see about
> moving the <<= into the macro, which I suspect will seem more natural
> with a loop called "retry".

Oh, please don't do that.  I much prefer to keep anything out of a
macro that doesn't have to be there.

The fdt_size increase is part of the loop control.  Now you can't put
it inside the for(;;) brackets but putting it at the top of the loop
will make it nice and obvious.

Personally I normally do something like this
     fdt_size += 4096;
     fdt_size <<= 2;
which avoids the if (fdt_size).  The only downside is that the
answers aren't uniformly powers of 2.

> > Please don't key off NDEBUG.  libxl ought never to be compiled
> > with NDEBUG.  I don't see why you can't just have this code present
> > all the time.
> 
> I wasn't entirely sure about keeping it at all, I wrote it for my own
> debugging purposes. I'm happy with it unconditional if you are.

Sure.

> > The style in this function is anomalous:
> > 
> >   * spaces inside ( )
> > 
> >   * use of
> >       if ((ret = blah) !=0 )
> >     which hides the meat in the if and has !=0 clobber, instead of
> >       rc = blah;
> >       if (rc) goto out;
> > 
> >   * use of "ret" for a libxl error code, rather than rc.
> > 
> > But I won't insist on you fixing it.  Adding an extra occurrence is
> > probably better than having one call which is different to all the
> > others.
> 
> I agree.

Thanks,
Ian.

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