[U-Boot] Rewriting fw_{printenv,getenv} to not use fgets
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'm aware this also gets used as a library, so I'm guessing I need to make linking of getline.c conditional somehow.
Does that make sense? Or should I be attempting it some other way.
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.
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 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 ?
Does that make sense? Or should I be attempting it some other way.
Best regards, Stefano Babic
Hi Stefano
On Thu, Jun 7, 2018 at 8:16 AM Stefano Babic sbabic@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@denx.de =====================================================================
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@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
Hi Stefano
On Thu, Jun 7, 2018 at 9:15 AM Stefano Babic sbabic@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@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.
participants (2)
-
Alex Kiernan -
Stefano Babic