[U-Boot] Rewriting fw_{printenv,getenv} to not use fgets

[Alex Kiernan]

Hi Stefano

On Thu, Jun 7, 2018 at 8:16 AM Stefano Babic <sbabic at denx.de> wrote:
>
> Hi Alex,
>
> On 06/06/2018 18:33, Alex Kiernan wrote:
> > The line length limit that fw_{printenv,setenv} impose (1024
> > characters) has tripped me up twice in as many days, so I figured I'd
> > rewrite it to use getline as we already have that in tools/.
> >
> > But in looking how I structure that change, I've immediately run into
> > questions...
> >
> > Right now I have a horrid hack that just pulls in ../getline.[ch], but
> > I'm thinking I should hoist the build of the environment tools, into
> > tools/Makefile from tools/env/Makefile, move getline.c into tools/lib
> > and then patch the trivial change in to swap from fgets to getline in
> > then.
>
> I do not understand the point: fgets has not such as limit, it simply
> pus bytes into a preallocated buffer. And for scripts (the only point
> where fgets is called), yes, the buffer is 1024. This is the only place
> where fgets() is called.
>

There's two - one is this (where I have a line longer than 1024) and a
second when it's parsing the config file (128 chars), where I overran
it because of a mount point which ends up named according to a
SHA-256. In the second case the long line wasn't detected and in my
case I spent some time before I figured out that the reason
fw_printenv was claiming CRC fail was because it hadn't read the
entire line and thought the environment buffer was shorter than it
actually was.

> On the other side, getline allocates the buffer. In our case, we have
> couple of <var> <value> , and it looks strange we cannot have this under
> control. If you need this, you have very long value for a variable, and
> this makes the script hard to read. It is good practise to split the
> long assignment in more variables to improve maintainance.
>

I'm dealing with migrating from an ancient version of U-Boot, to a new
version and got tripped up trying to manipulate the existing
environment, so it's not under my control at this point. So whilst I'd
like to not have long variables, I'd also like to not change it at the
point of migration, though probably I can just remove the existing
lines before writing things back as I'm not actually touching those
ones.

> >
> > I'm aware this also gets used as a library, so I'm guessing I need to
> > make linking of getline.c conditional somehow.
>
> But getline is part of glibc. Why do you have to copy it ?
>

I wasn't clear that assuming glibc (or musl which also has it) was an
acceptable option. If we can assume getline exists, then the patch is
trivial - that it exists in tools/getline.c made me think that wasn't
a reasonable assumption.

> >
> > Does that make sense? Or should I be attempting it some other way.
> >
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> =====================================================================



-- 
Alex Kiernan