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

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-10-16, Sascha Hauer wrote:
> 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.

FTR: I followed your suggestion, please see the v2.

Regards,
  Marco