Re: [PATCH 21/26] scripts: imx: add helper to write CSF Blocks command

[Sascha Hauer <sha@pengutronix.de>]

On Mon, Oct 16, 2023 at 09:44:50AM +0200, Marco Felsch wrote:
> Hi Sascha,
> 
> On 23-10-16, Marco Felsch wrote:
> > On 23-10-13, Sascha Hauer wrote:
> > > On Tue, Oct 10, 2023 at 04:33:09PM +0200, Marco Felsch wrote:
> > > > The upcoming commit fixes the HAB support for FlexSPI enabled barebox
> > > > images. This commit prepares the source to keep the diff smaller.
> > > > 
> > > > For the upcoming fix we need a 2nd CSF command sequence which is
> > > > basically the same as the first except for the "Blocks = ...." command.
> > > > Therefore we need to handle the blocks command separately which is done
> > > > by this commit. At the moment there is no functional change.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  scripts/imx/imx.c | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/scripts/imx/imx.c b/scripts/imx/imx.c
> > > > index 933019eba449..acc8424e547d 100644
> > > > --- a/scripts/imx/imx.c
> > > > +++ b/scripts/imx/imx.c
> > > > @@ -296,6 +296,24 @@ static int hab_add_str(struct config_data *data, const char *str)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int hab_add_barebox_blocks(struct config_data *data,
> > > > +				  const char *csf_str,
> > > > +				  const char *flexspi_csf_str)
> > > > +{
> > > > +	int len = strlen(csf_str);
> > > > +
> > > > +	if (data->csf_space < len)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	strcat(data->csf, csf_str);
> > > > +	if (flexspi_csf_str)
> > > > +		strcat(data->flexspi_csf, flexspi_csf_str);
> > > 
> > > Do we need a space check here as well?
> > 
> > To be 100% accurate yes since the strings can have different lengths
> > albeit the difference would be <5 chars. I will rework that, thanks for
> > the review.
> 
> While integrating your review feedback I noticed that my comment was
> wrong. Both strings using the same size limited format string:
> 
>    "Blocks = 0x%08x 0x%08x 0x%08x \"%s\""
> 
> with "%s" always point to data->outfile. So both stings do not differ
> and checking csf_str in enough.

Indeed, so the current way is safe.

However, I wonder if we should rather add a little helper like:

static void *strcata(char *str, const char *add)
{
	str = realloc(str, (str ? strlen(str) : 0) + strlen(add) + 1);
	if (!str)
		return NULL;

	strcat(str, add);

	return str;
}

and get rid oof the fixed size allocation altogether.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |