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

[Stefano Babic]

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.

> 
>>>
>>> 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
=====================================================================