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

[Alex Kiernan]

Hi Stefano

On Thu, Jun 7, 2018 at 9:15 AM Stefano Babic <sbabic at denx.de> wrote:
>
> Hi Alex,
>
> On 07/06/2018 10:07, Alex Kiernan wrote:
> > 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.
>
> ok
>
> > 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.
>
> Understood, thanks.
>
> >
> >> 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.
> >
>
> Right.
>
> >>>
> >>> 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.
>
> The tools fw_{printenv,getenv} run in user space and they are using a
> standard environment, that is you can assume that glibc | musl is
> linked. In fact, you see other function calls that are part of libc.
>
> To let you better understand, just fw_env_main.c is not part of the
> library.
>

Thanks, that helps clarify.

-- 
Alex Kiernan