Message ID | E32EC0F1-3CEB-45E7-9CB5-92DB031D4B92@siemens.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [libubootenv,RFC] uboot_env: Emulate %ms in sscanf() | expand |
Hi, Am Mittwoch, 29. November 2023, 21:51:48 CET schrieb 'Storm, Christian' via swupdate: > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > src/uboot_env.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index bb14cab..7d3264e 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx > **ctxlist, const char *config) ctx->size = 0; > rewind(fp); > > - while (getline(&line, &bufsize, fp) != -1) { > + int len; > + while ((len = getline(&line, &bufsize, fp)) != -1) { > /* skip comments */ > if (line[0] == '#') > continue; > > +#if defined(__FreeBSD__) > + /* > + * POSIX.1-2008 introduced the dynamic allocation conversion > + * specifier %m which is not implemented on FreeBSD. > + */ > + tmp = calloc(1, len + 1); > + ret = sscanf(line, "%s %lli %zx %zx %lx %d", > + tmp, > +#else > + (void)len; > ret = sscanf(line, "%ms %lli %zx %zx %lx %d", > &tmp, > +#endif > &dev->offset, > &dev->envsize, > &dev->sectorsize, > @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx > **ctxlist, const char *config) /* > * At least name offset and size should be set > */ > - if (ret < 3 || !tmp) > + if (ret < 3) { > + if (tmp) { > + free(tmp); > + } Usually free() should accept NULL pointers and do nothing in this case, so the "if" could be probably omitted. Same for the change below. Best regards, Michael > continue; > + } > > /* > * If size is set but zero, entry is wrong > */ > if (!dev->envsize) { > + if (tmp) { > + free(tmp); > + } > retval = -EINVAL; > break; > }
Hi Michael, >> Signed-off-by: Christian Storm <christian.storm@siemens.com> >> --- >> src/uboot_env.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/src/uboot_env.c b/src/uboot_env.c >> index bb14cab..7d3264e 100644 >> --- a/src/uboot_env.c >> +++ b/src/uboot_env.c >> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx >> **ctxlist, const char *config) ctx->size = 0; >> rewind(fp); >> >> - while (getline(&line, &bufsize, fp) != -1) { >> + int len; >> + while ((len = getline(&line, &bufsize, fp)) != -1) { >> /* skip comments */ >> if (line[0] == '#') >> continue; >> >> +#if defined(__FreeBSD__) >> + /* >> + * POSIX.1-2008 introduced the dynamic allocation > conversion >> + * specifier %m which is not implemented on FreeBSD. >> + */ >> + tmp = calloc(1, len + 1); >> + ret = sscanf(line, "%s %lli %zx %zx %lx %d", >> + tmp, >> +#else >> + (void)len; >> ret = sscanf(line, "%ms %lli %zx %zx %lx %d", >> &tmp, >> +#endif >> &dev->offset, >> &dev->envsize, >> &dev->sectorsize, >> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx >> **ctxlist, const char *config) /* >> * At least name offset and size should be set >> */ >> - if (ret < 3 || !tmp) >> + if (ret < 3) { >> + if (tmp) { >> + free(tmp); >> + } > > Usually free() should accept NULL pointers and do nothing in this case, so the > "if" could be probably omitted. > Same for the change below. Emphasis should be on "usually" :) That depends on the libc implementation... Either way, I did this consciously for this reason and, more importantly, for being explicit that there maybe is something to free() which is more apparent this way. But I don't care too much if you prefer a shorter version... Kind regards, Christian
Hi Christian, Am Donnerstag, 30. November 2023, 15:42:41 CET schrieb Storm, Christian: > Hi Michael, > > >> Signed-off-by: Christian Storm <christian.storm@siemens.com> > >> --- > >> src/uboot_env.c | 23 +++++++++++++++++++++-- > >> 1 file changed, 21 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/uboot_env.c b/src/uboot_env.c > >> index bb14cab..7d3264e 100644 > >> --- a/src/uboot_env.c > >> +++ b/src/uboot_env.c > >> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx > >> **ctxlist, const char *config) ctx->size = 0; > >> rewind(fp); > >> > >> - while (getline(&line, &bufsize, fp) != -1) { > >> + int len; > >> + while ((len = getline(&line, &bufsize, fp)) != -1) { > >> /* skip comments */ > >> if (line[0] == '#') > >> continue; > >> > >> +#if defined(__FreeBSD__) > >> + /* > >> + * POSIX.1-2008 introduced the dynamic allocation > > > > conversion > > > >> + * specifier %m which is not implemented on FreeBSD. > >> + */ > >> + tmp = calloc(1, len + 1); > >> + ret = sscanf(line, "%s %lli %zx %zx %lx %d", > >> + tmp, > >> +#else > >> + (void)len; > >> ret = sscanf(line, "%ms %lli %zx %zx %lx %d", > >> &tmp, > >> +#endif > >> &dev->offset, > >> &dev->envsize, > >> &dev->sectorsize, > >> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx > >> **ctxlist, const char *config) /* > >> * At least name offset and size should be set > >> */ > >> - if (ret < 3 || !tmp) > >> + if (ret < 3) { > >> + if (tmp) { > >> + free(tmp); > >> + } > > > > Usually free() should accept NULL pointers and do nothing in this case, so > > the "if" could be probably omitted. > > Same for the change below. > > Emphasis should be on "usually" :) That depends on the libc > implementation... Either way, I did this consciously for this reason and, > more importantly, for being explicit that there maybe is something to > free() which is more apparent this way. I also remember these days, especially on older embedded systems/compilers... but AFAIK this is defined in the C standard (or POSIX or whatever) and thus having a check for something what is expected behavior is considered bad practise on my personal side and code reviewers should nowadays be aware of this - you might have another opinion so please don't feel offended :-) And I'm especially not the person who decides here. Kind regards, Michael > > But I don't care too much if you prefer a shorter version... > > > Kind regards, > Christian
Hi, On 30.11.23 18:23, Michael Heimpold wrote: > Hi Christian, > > Am Donnerstag, 30. November 2023, 15:42:41 CET schrieb Storm, Christian: >> Hi Michael, >> >>>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>>> --- >>>> src/uboot_env.c | 23 +++++++++++++++++++++-- >>>> 1 file changed, 21 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/uboot_env.c b/src/uboot_env.c >>>> index bb14cab..7d3264e 100644 >>>> --- a/src/uboot_env.c >>>> +++ b/src/uboot_env.c >>>> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx >>>> **ctxlist, const char *config) ctx->size = 0; >>>> rewind(fp); >>>> >>>> - while (getline(&line, &bufsize, fp) != -1) { >>>> + int len; >>>> + while ((len = getline(&line, &bufsize, fp)) != -1) { >>>> /* skip comments */ >>>> if (line[0] == '#') >>>> continue; >>>> >>>> +#if defined(__FreeBSD__) >>>> + /* >>>> + * POSIX.1-2008 introduced the dynamic allocation >>> >>> conversion >>> >>>> + * specifier %m which is not implemented on FreeBSD. >>>> + */ >>>> + tmp = calloc(1, len + 1); >>>> + ret = sscanf(line, "%s %lli %zx %zx %lx %d", >>>> + tmp, >>>> +#else >>>> + (void)len; >>>> ret = sscanf(line, "%ms %lli %zx %zx %lx %d", >>>> &tmp, >>>> +#endif >>>> &dev->offset, >>>> &dev->envsize, >>>> &dev->sectorsize, >>>> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx >>>> **ctxlist, const char *config) /* >>>> * At least name offset and size should be set >>>> */ >>>> - if (ret < 3 || !tmp) >>>> + if (ret < 3) { >>>> + if (tmp) { >>>> + free(tmp); >>>> + } >>> >>> Usually free() should accept NULL pointers and do nothing in this case, so >>> the "if" could be probably omitted. >>> Same for the change below. >> >> Emphasis should be on "usually" :) That depends on the libc >> implementation... Either way, I did this consciously for this reason and, >> more importantly, for being explicit that there maybe is something to >> free() which is more apparent this way. > > I also remember these days, especially on older embedded systems/compilers... ..right... > but AFAIK this is defined in the C standard (or POSIX or whatever) and > thus having a check for something what is expected behavior is considered > bad practise on my personal side and code reviewers should nowadays be aware > of this - you might have another opinion so please don't feel offended :-) > And I'm especially not the person who decides here. Sure we have not the same behavior in all code, but we have already in some cases decided in previous patch to remove the check because we rely on free() behavior. Best regards, Stefano > > Kind regards, > Michael >> >> But I don't care too much if you prefer a shorter version... >> >> >> Kind regards, >> Christian > > > >
diff --git a/src/uboot_env.c b/src/uboot_env.c index bb14cab..7d3264e 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx **ctxlist, const char *config) ctx->size = 0; rewind(fp); - while (getline(&line, &bufsize, fp) != -1) { + int len; + while ((len = getline(&line, &bufsize, fp)) != -1) { /* skip comments */ if (line[0] == '#') continue; +#if defined(__FreeBSD__) + /* + * POSIX.1-2008 introduced the dynamic allocation conversion + * specifier %m which is not implemented on FreeBSD. + */ + tmp = calloc(1, len + 1); + ret = sscanf(line, "%s %lli %zx %zx %lx %d", + tmp, +#else + (void)len; ret = sscanf(line, "%ms %lli %zx %zx %lx %d", &tmp, +#endif &dev->offset, &dev->envsize, &dev->sectorsize, @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx **ctxlist, const char *config) /* * At least name offset and size should be set */ - if (ret < 3 || !tmp) + if (ret < 3) { + if (tmp) { + free(tmp); + } continue; + } /* * If size is set but zero, entry is wrong */ if (!dev->envsize) { + if (tmp) { + free(tmp); + } retval = -EINVAL; break; }
Signed-off-by: Christian Storm <christian.storm@siemens.com> --- src/uboot_env.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)