Patchwork [U-Boot,01/11] fw_env.c: Increase max dev path to 32

login
register
mail settings
Submitter Tom Rini
Date Sept. 26, 2013, 8:27 p.m.
Message ID <1380227287-26057-2-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/278259/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Tom Rini - Sept. 26, 2013, 8:27 p.m.
Currently our limit is too small to allow for /dev/mmcblk0boot0 to work,
for example.  Expand to 32 for future needs.

Signed-off-by: Tom Rini <trini@ti.com>
---
 tools/env/fw_env.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Wolfgang Denk - Oct. 5, 2013, 8:02 p.m.
Dear Tom Rini,

In message <1380227287-26057-2-git-send-email-trini@ti.com> you wrote:
> Currently our limit is too small to allow for /dev/mmcblk0boot0 to work,
> for example.  Expand to 32 for future needs.
> 
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  tools/env/fw_env.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 577ce2d..bf5c552 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -40,7 +40,7 @@
>  	_min1 < _min2 ? _min1 : _min2; })
>  
>  struct envdev_s {
> -	char devname[16];		/* Device name */
> +	char devname[32];		/* Device name */

Do we really need a static size here?  Can we not auto-adjust to the
needed size, say by dynamically allocating the buffer?

Best regards,

Wolfgang Denk
Tom Rini - Oct. 6, 2013, 8:55 p.m.
On Sat, Oct 05, 2013 at 10:02:12PM +0200, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <1380227287-26057-2-git-send-email-trini@ti.com> you wrote:
> > Currently our limit is too small to allow for /dev/mmcblk0boot0 to work,
> > for example.  Expand to 32 for future needs.
> > 
> > Signed-off-by: Tom Rini <trini@ti.com>
> > ---
> >  tools/env/fw_env.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> > index 577ce2d..bf5c552 100644
> > --- a/tools/env/fw_env.c
> > +++ b/tools/env/fw_env.c
> > @@ -40,7 +40,7 @@
> >  	_min1 < _min2 ? _min1 : _min2; })
> >  
> >  struct envdev_s {
> > -	char devname[16];		/* Device name */
> > +	char devname[32];		/* Device name */
> 
> Do we really need a static size here?  Can we not auto-adjust to the
> needed size, say by dynamically allocating the buffer?

Doesn't look like it, without a big change to the parsing code.
Wolfgang Denk - Oct. 7, 2013, 5:47 a.m.
Dear Tom,

In message <20131006205527.GP15917@bill-the-cat> you wrote:
> 
> > Do we really need a static size here?  Can we not auto-adjust to the
> > needed size, say by dynamically allocating the buffer?
> 
> Doesn't look like it, without a big change to the parsing code.

I don't think this requires a big change.  Eventually all it takes is
changing the sscanf() call in get_config() to use a format "%ms"
instead of plain "%s"; form the sscanf() man page:

        ยท An optional 'm' character. This is used with string
          conversions (%s, %c, %[), and relieves the caller of the
          need to allocate a corresponding buffer to hold the input:
          instead, scanf() allocates a buffer of sufficient size, and
          assigns the address of this buffer to the corresponding
          pointer argument, which should be a pointer to a char *
          variable (this variable does not need to be initialized
          before the call). The caller should subsequently free(3)
          this buffer when it is no longer required.

OK, the struct should then of course contain a const char pointer
instead of the buffer itself, but that's also a trivial change.

Best regards,

Wolfgang Denk
Tom Rini - Oct. 7, 2013, 12:06 p.m.
On Mon, Oct 07, 2013 at 07:47:13AM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20131006205527.GP15917@bill-the-cat> you wrote:
> > 
> > > Do we really need a static size here?  Can we not auto-adjust to the
> > > needed size, say by dynamically allocating the buffer?
> > 
> > Doesn't look like it, without a big change to the parsing code.
> 
> I don't think this requires a big change.  Eventually all it takes is
> changing the sscanf() call in get_config() to use a format "%ms"
> instead of plain "%s"; form the sscanf() man page:
> 
>         ?? An optional 'm' character. This is used with string
>           conversions (%s, %c, %[), and relieves the caller of the
>           need to allocate a corresponding buffer to hold the input:
>           instead, scanf() allocates a buffer of sufficient size, and
>           assigns the address of this buffer to the corresponding
>           pointer argument, which should be a pointer to a char *
>           variable (this variable does not need to be initialized
>           before the call). The caller should subsequently free(3)
>           this buffer when it is no longer required.
> 
> OK, the struct should then of course contain a const char pointer
> instead of the buffer itself, but that's also a trivial change.

Well, that's what I get for looking at the code, and not checking man
pages.  Agreed, I'll re-work this part.

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 577ce2d..bf5c552 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -40,7 +40,7 @@ 
 	_min1 < _min2 ? _min1 : _min2; })
 
 struct envdev_s {
-	char devname[16];		/* Device name */
+	char devname[32];		/* Device name */
 	ulong devoff;			/* Device offset */
 	ulong env_size;			/* environment size */
 	ulong erase_size;		/* device erase size */