| Message ID | 20230917152108.692767-1-stefano.babic@swupdate.org |
|---|---|
| State | Accepted |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [libubootenv] Add libuboot_namespace_from_dt | expand |
Hi Stefano, thanks for working on this! On 17.09.23 17:21, Stefano Babic wrote: > [Sie erhalten nicht häufig E-Mails von stefano.babic@swupdate.org. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ] > > Move code from fw_printenv to livrary. libuboot_namespace_from_dt looks ^library > into DT and reads the namespace for the bootloader, if any. > > Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> > Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > src/fw_printenv.c | 20 ++++---------------- > src/libuboot.h | 9 ++++++++- > src/uboot_env.c | 17 +++++++++++++++++ > 3 files changed, 29 insertions(+), 17 deletions(-) > > diff --git a/src/fw_printenv.c b/src/fw_printenv.c > index 6118324..6a7a987 100644 > --- a/src/fw_printenv.c > +++ b/src/fw_printenv.c > @@ -77,7 +77,7 @@ int main (int argc, char **argv) { > char *cfgfname = NULL; > char *defenvfile = NULL; > char *scriptfile = NULL; > - char *namespace = NULL; > + const char *namespace = NULL; > int c, i; > int ret = 0; > void *tmp; > @@ -87,9 +87,6 @@ int main (int argc, char **argv) { > bool noheader = false; > bool default_used = false; > struct uboot_version_info *version; > - char dt_namespace[32]; > - size_t dt_ret; > - FILE *fp; > > /* > * As old tool, there is just a tool with symbolic link > @@ -148,20 +145,11 @@ int main (int argc, char **argv) { > exit(1); > } > > + if (!namespace) > + namespace = libuboot_namespace_from_dt(); > + > if (namespace) > ctx = libuboot_get_namespace(ctx, namespace); > - else { > - fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); > - if(fp) { > - dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); > - if (dt_ret) { > - dt_namespace[dt_ret] = 0; > - ctx = libuboot_get_namespace(ctx, dt_namespace); > - } > - > - fclose(fp); > - } > - } > > if (!ctx) { > fprintf(stderr, "Namespace %s not found\n", namespace); > diff --git a/src/libuboot.h b/src/libuboot.h > index 624800a..3ed3244 100644 > --- a/src/libuboot.h > +++ b/src/libuboot.h > @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config); > */ > int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); > > -/** @brief Get ctx from list - this is maintained for compatibility > +/** @brief Get ctx from namespace > * > * @param[in] ctxlist libuboot context array > * @param[in] name name identifier for the single ctx > @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); > */ > struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char *name); > > +/** @brief Look for bootloader namespace from DT > + * > + * @param[in] ctxlist libuboot context array > + * @param[in] name name identifier for the single ctx > + * @return 0 in case of success, else negative value > + */ > +const char *libuboot_namespace_from_dt(void); > > /** @brief Read U-Boot environment configuration from structure > * > diff --git a/src/uboot_env.c b/src/uboot_env.c > index b924c49..34caeab 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -1888,6 +1888,23 @@ struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * > return NULL; > } > > +const char *libuboot_namespace_from_dt(void) > +{ > + FILE *fp; > + char dt_namespace[64]; > + size_t dt_ret; > + > + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); > + if (!fp) > + return NULL; > + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); > + fclose(fp); > + if (!dt_ret) > + return NULL; > + dt_namespace[dt_ret] = 0; > + return strdup(dt_namespace); strdup() does allocate memory that needs to be freed somewhere, right? Or am I missing something? Also wouldn't it be better to allocate a buffer for the string right at the beginning instead of putting the string on the stack first and then copy it to a buffer that is returned to the caller? Thanks Frieder > +} > + > int libuboot_initialize(struct uboot_ctx **out, > struct uboot_env_device *envdevs) { > struct uboot_ctx *ctx; > -- > 2.34.1 >
On 20.09.23 12:43, Frieder Schrempf wrote: > Hi Stefano, > > thanks for working on this! > > On 17.09.23 17:21, Stefano Babic wrote: >> [Sie erhalten nicht häufig E-Mails von stefano.babic@swupdate.org. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ] >> >> Move code from fw_printenv to livrary. libuboot_namespace_from_dt looks > > ^library > >> into DT and reads the namespace for the bootloader, if any. >> >> Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> >> Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> >> --- >> src/fw_printenv.c | 20 ++++---------------- >> src/libuboot.h | 9 ++++++++- >> src/uboot_env.c | 17 +++++++++++++++++ >> 3 files changed, 29 insertions(+), 17 deletions(-) >> >> diff --git a/src/fw_printenv.c b/src/fw_printenv.c >> index 6118324..6a7a987 100644 >> --- a/src/fw_printenv.c >> +++ b/src/fw_printenv.c >> @@ -77,7 +77,7 @@ int main (int argc, char **argv) { >> char *cfgfname = NULL; >> char *defenvfile = NULL; >> char *scriptfile = NULL; >> - char *namespace = NULL; >> + const char *namespace = NULL; >> int c, i; >> int ret = 0; >> void *tmp; >> @@ -87,9 +87,6 @@ int main (int argc, char **argv) { >> bool noheader = false; >> bool default_used = false; >> struct uboot_version_info *version; >> - char dt_namespace[32]; >> - size_t dt_ret; >> - FILE *fp; >> >> /* >> * As old tool, there is just a tool with symbolic link >> @@ -148,20 +145,11 @@ int main (int argc, char **argv) { >> exit(1); >> } >> >> + if (!namespace) >> + namespace = libuboot_namespace_from_dt(); >> + >> if (namespace) >> ctx = libuboot_get_namespace(ctx, namespace); >> - else { >> - fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >> - if(fp) { >> - dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); >> - if (dt_ret) { >> - dt_namespace[dt_ret] = 0; >> - ctx = libuboot_get_namespace(ctx, dt_namespace); >> - } >> - >> - fclose(fp); >> - } >> - } >> >> if (!ctx) { >> fprintf(stderr, "Namespace %s not found\n", namespace); >> diff --git a/src/libuboot.h b/src/libuboot.h >> index 624800a..3ed3244 100644 >> --- a/src/libuboot.h >> +++ b/src/libuboot.h >> @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config); >> */ >> int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); >> >> -/** @brief Get ctx from list - this is maintained for compatibility >> +/** @brief Get ctx from namespace >> * >> * @param[in] ctxlist libuboot context array >> * @param[in] name name identifier for the single ctx >> @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); >> */ >> struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char *name); >> >> +/** @brief Look for bootloader namespace from DT >> + * >> + * @param[in] ctxlist libuboot context array >> + * @param[in] name name identifier for the single ctx >> + * @return 0 in case of success, else negative value >> + */ >> +const char *libuboot_namespace_from_dt(void); >> >> /** @brief Read U-Boot environment configuration from structure >> * >> diff --git a/src/uboot_env.c b/src/uboot_env.c >> index b924c49..34caeab 100644 >> --- a/src/uboot_env.c >> +++ b/src/uboot_env.c >> @@ -1888,6 +1888,23 @@ struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * >> return NULL; >> } >> >> +const char *libuboot_namespace_from_dt(void) >> +{ >> + FILE *fp; >> + char dt_namespace[64]; >> + size_t dt_ret; >> + >> + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >> + if (!fp) >> + return NULL; >> + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); >> + fclose(fp); >> + if (!dt_ret) >> + return NULL; >> + dt_namespace[dt_ret] = 0; >> + return strdup(dt_namespace); > > strdup() does allocate memory that needs to be freed somewhere, right? > Or am I missing something? Yes, my idea (like for context) is that the library allocates the struct, and the caller should free it. > > Also wouldn't it be better to allocate a buffer for the string right at > the beginning instead of putting the string on the stack first and then > copy it to a buffer that is returned to the caller? I will change in this way. Regards, Stefano > > Thanks > Frieder > >> +} >> + >> int libuboot_initialize(struct uboot_ctx **out, >> struct uboot_env_device *envdevs) { >> struct uboot_ctx *ctx; >> -- >> 2.34.1 >>
On 20.09.23 13:07, Stefano Babic wrote: > On 20.09.23 12:43, Frieder Schrempf wrote: >> Hi Stefano, >> >> thanks for working on this! >> >> On 17.09.23 17:21, Stefano Babic wrote: >>> [Sie erhalten nicht häufig E-Mails von stefano.babic@swupdate.org. >>> Weitere Informationen, warum dies wichtig ist, finden Sie unter >>> https://aka.ms/LearnAboutSenderIdentification ] >>> >>> Move code from fw_printenv to livrary. libuboot_namespace_from_dt looks >> >> ^library >> >>> into DT and reads the namespace for the bootloader, if any. >>> >>> Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> >>> Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>> --- >>> src/fw_printenv.c | 20 ++++---------------- >>> src/libuboot.h | 9 ++++++++- >>> src/uboot_env.c | 17 +++++++++++++++++ >>> 3 files changed, 29 insertions(+), 17 deletions(-) >>> >>> diff --git a/src/fw_printenv.c b/src/fw_printenv.c >>> index 6118324..6a7a987 100644 >>> --- a/src/fw_printenv.c >>> +++ b/src/fw_printenv.c >>> @@ -77,7 +77,7 @@ int main (int argc, char **argv) { >>> char *cfgfname = NULL; >>> char *defenvfile = NULL; >>> char *scriptfile = NULL; >>> - char *namespace = NULL; >>> + const char *namespace = NULL; >>> int c, i; >>> int ret = 0; >>> void *tmp; >>> @@ -87,9 +87,6 @@ int main (int argc, char **argv) { >>> bool noheader = false; >>> bool default_used = false; >>> struct uboot_version_info *version; >>> - char dt_namespace[32]; >>> - size_t dt_ret; >>> - FILE *fp; >>> >>> /* >>> * As old tool, there is just a tool with symbolic link >>> @@ -148,20 +145,11 @@ int main (int argc, char **argv) { >>> exit(1); >>> } >>> >>> + if (!namespace) >>> + namespace = libuboot_namespace_from_dt(); >>> + >>> if (namespace) >>> ctx = libuboot_get_namespace(ctx, namespace); >>> - else { >>> - fp = >>> fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>> - if(fp) { >>> - dt_ret = fread(dt_namespace, 1, >>> sizeof(dt_namespace) - 1, fp); >>> - if (dt_ret) { >>> - dt_namespace[dt_ret] = 0; >>> - ctx = libuboot_get_namespace(ctx, >>> dt_namespace); >>> - } >>> - >>> - fclose(fp); >>> - } >>> - } >>> >>> if (!ctx) { >>> fprintf(stderr, "Namespace %s not found\n", namespace); >>> diff --git a/src/libuboot.h b/src/libuboot.h >>> index 624800a..3ed3244 100644 >>> --- a/src/libuboot.h >>> +++ b/src/libuboot.h >>> @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, >>> const char *config); >>> */ >>> int libuboot_read_config_ext(struct uboot_ctx **ctx, const char >>> *config); >>> >>> -/** @brief Get ctx from list - this is maintained for compatibility >>> +/** @brief Get ctx from namespace >>> * >>> * @param[in] ctxlist libuboot context array >>> * @param[in] name name identifier for the single ctx >>> @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx >>> **ctx, const char *config); >>> */ >>> struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, >>> const char *name); >>> >>> +/** @brief Look for bootloader namespace from DT >>> + * >>> + * @param[in] ctxlist libuboot context array >>> + * @param[in] name name identifier for the single ctx >>> + * @return 0 in case of success, else negative value >>> + */ >>> +const char *libuboot_namespace_from_dt(void); >>> >>> /** @brief Read U-Boot environment configuration from structure >>> * >>> diff --git a/src/uboot_env.c b/src/uboot_env.c >>> index b924c49..34caeab 100644 >>> --- a/src/uboot_env.c >>> +++ b/src/uboot_env.c >>> @@ -1888,6 +1888,23 @@ struct uboot_ctx >>> *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * >>> return NULL; >>> } >>> >>> +const char *libuboot_namespace_from_dt(void) >>> +{ >>> + FILE *fp; >>> + char dt_namespace[64]; >>> + size_t dt_ret; >>> + >>> + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>> + if (!fp) >>> + return NULL; >>> + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); >>> + fclose(fp); >>> + if (!dt_ret) >>> + return NULL; >>> + dt_namespace[dt_ret] = 0; >>> + return strdup(dt_namespace); >> >> strdup() does allocate memory that needs to be freed somewhere, right? >> Or am I missing something? > > Yes, my idea (like for context) is that the library allocates the > struct, and the caller should free it. Ok, but above in fw_printenv.c libuboot_namespace_from_dt() is called without free() anywhere, no? > >> >> Also wouldn't it be better to allocate a buffer for the string right at >> the beginning instead of putting the string on the stack first and then >> copy it to a buffer that is returned to the caller? > > I will change in this way.> > Regards, > Stefano > >> >> Thanks >> Frieder >> >>> +} >>> + >>> int libuboot_initialize(struct uboot_ctx **out, >>> struct uboot_env_device *envdevs) { >>> struct uboot_ctx *ctx; >>> -- >>> 2.34.1 >>> >
Hi Frieder, On 20.09.23 13:52, Frieder Schrempf wrote: > On 20.09.23 13:07, Stefano Babic wrote: >> On 20.09.23 12:43, Frieder Schrempf wrote: >>> Hi Stefano, >>> >>> thanks for working on this! >>> >>> On 17.09.23 17:21, Stefano Babic wrote: >>>> [Sie erhalten nicht häufig E-Mails von stefano.babic@swupdate.org. >>>> Weitere Informationen, warum dies wichtig ist, finden Sie unter >>>> https://aka.ms/LearnAboutSenderIdentification ] >>>> >>>> Move code from fw_printenv to livrary. libuboot_namespace_from_dt looks >>> >>> ^library >>> >>>> into DT and reads the namespace for the bootloader, if any. >>>> >>>> Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> >>>> Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>>> --- >>>> src/fw_printenv.c | 20 ++++---------------- >>>> src/libuboot.h | 9 ++++++++- >>>> src/uboot_env.c | 17 +++++++++++++++++ >>>> 3 files changed, 29 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/src/fw_printenv.c b/src/fw_printenv.c >>>> index 6118324..6a7a987 100644 >>>> --- a/src/fw_printenv.c >>>> +++ b/src/fw_printenv.c >>>> @@ -77,7 +77,7 @@ int main (int argc, char **argv) { >>>> char *cfgfname = NULL; >>>> char *defenvfile = NULL; >>>> char *scriptfile = NULL; >>>> - char *namespace = NULL; >>>> + const char *namespace = NULL; >>>> int c, i; >>>> int ret = 0; >>>> void *tmp; >>>> @@ -87,9 +87,6 @@ int main (int argc, char **argv) { >>>> bool noheader = false; >>>> bool default_used = false; >>>> struct uboot_version_info *version; >>>> - char dt_namespace[32]; >>>> - size_t dt_ret; >>>> - FILE *fp; >>>> >>>> /* >>>> * As old tool, there is just a tool with symbolic link >>>> @@ -148,20 +145,11 @@ int main (int argc, char **argv) { >>>> exit(1); >>>> } >>>> >>>> + if (!namespace) >>>> + namespace = libuboot_namespace_from_dt(); >>>> + >>>> if (namespace) >>>> ctx = libuboot_get_namespace(ctx, namespace); >>>> - else { >>>> - fp = >>>> fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>>> - if(fp) { >>>> - dt_ret = fread(dt_namespace, 1, >>>> sizeof(dt_namespace) - 1, fp); >>>> - if (dt_ret) { >>>> - dt_namespace[dt_ret] = 0; >>>> - ctx = libuboot_get_namespace(ctx, >>>> dt_namespace); >>>> - } >>>> - >>>> - fclose(fp); >>>> - } >>>> - } >>>> >>>> if (!ctx) { >>>> fprintf(stderr, "Namespace %s not found\n", namespace); >>>> diff --git a/src/libuboot.h b/src/libuboot.h >>>> index 624800a..3ed3244 100644 >>>> --- a/src/libuboot.h >>>> +++ b/src/libuboot.h >>>> @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, >>>> const char *config); >>>> */ >>>> int libuboot_read_config_ext(struct uboot_ctx **ctx, const char >>>> *config); >>>> >>>> -/** @brief Get ctx from list - this is maintained for compatibility >>>> +/** @brief Get ctx from namespace >>>> * >>>> * @param[in] ctxlist libuboot context array >>>> * @param[in] name name identifier for the single ctx >>>> @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx >>>> **ctx, const char *config); >>>> */ >>>> struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, >>>> const char *name); >>>> >>>> +/** @brief Look for bootloader namespace from DT >>>> + * >>>> + * @param[in] ctxlist libuboot context array >>>> + * @param[in] name name identifier for the single ctx >>>> + * @return 0 in case of success, else negative value >>>> + */ >>>> +const char *libuboot_namespace_from_dt(void); >>>> >>>> /** @brief Read U-Boot environment configuration from structure >>>> * >>>> diff --git a/src/uboot_env.c b/src/uboot_env.c >>>> index b924c49..34caeab 100644 >>>> --- a/src/uboot_env.c >>>> +++ b/src/uboot_env.c >>>> @@ -1888,6 +1888,23 @@ struct uboot_ctx >>>> *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * >>>> return NULL; >>>> } >>>> >>>> +const char *libuboot_namespace_from_dt(void) >>>> +{ >>>> + FILE *fp; >>>> + char dt_namespace[64]; >>>> + size_t dt_ret; >>>> + >>>> + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>>> + if (!fp) >>>> + return NULL; >>>> + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); >>>> + fclose(fp); >>>> + if (!dt_ret) >>>> + return NULL; >>>> + dt_namespace[dt_ret] = 0; >>>> + return strdup(dt_namespace); >>> >>> strdup() does allocate memory that needs to be freed somewhere, right? >>> Or am I missing something? >> >> Yes, my idea (like for context) is that the library allocates the >> struct, and the caller should free it. > > Ok, but above in fw_printenv.c libuboot_namespace_from_dt() is called > without free() anywhere, no? Well, as many tools are doing when getopt() is parsed. The hidden way is to rely on the OS, because the tool runs in one shot and after the process ends, resources are freed automatically by the kernel. This is done by many tools on linux, and same is done here. If you see even scriptfile, defenvfile are not explicitely released. Stefano > >> >>> >>> Also wouldn't it be better to allocate a buffer for the string right at >>> the beginning instead of putting the string on the stack first and then >>> copy it to a buffer that is returned to the caller? >> >> I will change in this way.> >> Regards, >> Stefano >> >>> >>> Thanks >>> Frieder >>> >>>> +} >>>> + >>>> int libuboot_initialize(struct uboot_ctx **out, >>>> struct uboot_env_device *envdevs) { >>>> struct uboot_ctx *ctx; >>>> -- >>>> 2.34.1 >>>> >>
On 20.09.23 13:57, Stefano Babic wrote: > Hi Frieder, > > On 20.09.23 13:52, Frieder Schrempf wrote: >> On 20.09.23 13:07, Stefano Babic wrote: >>> On 20.09.23 12:43, Frieder Schrempf wrote: >>>> Hi Stefano, >>>> >>>> thanks for working on this! >>>> >>>> On 17.09.23 17:21, Stefano Babic wrote: >>>>> [Sie erhalten nicht häufig E-Mails von stefano.babic@swupdate.org. >>>>> Weitere Informationen, warum dies wichtig ist, finden Sie unter >>>>> https://aka.ms/LearnAboutSenderIdentification ] >>>>> >>>>> Move code from fw_printenv to livrary. libuboot_namespace_from_dt >>>>> looks >>>> >>>> ^library >>>> >>>>> into DT and reads the namespace for the bootloader, if any. >>>>> >>>>> Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> >>>>> Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>>>> --- >>>>> src/fw_printenv.c | 20 ++++---------------- >>>>> src/libuboot.h | 9 ++++++++- >>>>> src/uboot_env.c | 17 +++++++++++++++++ >>>>> 3 files changed, 29 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/src/fw_printenv.c b/src/fw_printenv.c >>>>> index 6118324..6a7a987 100644 >>>>> --- a/src/fw_printenv.c >>>>> +++ b/src/fw_printenv.c >>>>> @@ -77,7 +77,7 @@ int main (int argc, char **argv) { >>>>> char *cfgfname = NULL; >>>>> char *defenvfile = NULL; >>>>> char *scriptfile = NULL; >>>>> - char *namespace = NULL; >>>>> + const char *namespace = NULL; >>>>> int c, i; >>>>> int ret = 0; >>>>> void *tmp; >>>>> @@ -87,9 +87,6 @@ int main (int argc, char **argv) { >>>>> bool noheader = false; >>>>> bool default_used = false; >>>>> struct uboot_version_info *version; >>>>> - char dt_namespace[32]; >>>>> - size_t dt_ret; >>>>> - FILE *fp; >>>>> >>>>> /* >>>>> * As old tool, there is just a tool with symbolic link >>>>> @@ -148,20 +145,11 @@ int main (int argc, char **argv) { >>>>> exit(1); >>>>> } >>>>> >>>>> + if (!namespace) >>>>> + namespace = libuboot_namespace_from_dt(); >>>>> + >>>>> if (namespace) >>>>> ctx = libuboot_get_namespace(ctx, namespace); >>>>> - else { >>>>> - fp = >>>>> fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>>>> - if(fp) { >>>>> - dt_ret = fread(dt_namespace, 1, >>>>> sizeof(dt_namespace) - 1, fp); >>>>> - if (dt_ret) { >>>>> - dt_namespace[dt_ret] = 0; >>>>> - ctx = libuboot_get_namespace(ctx, >>>>> dt_namespace); >>>>> - } >>>>> - >>>>> - fclose(fp); >>>>> - } >>>>> - } >>>>> >>>>> if (!ctx) { >>>>> fprintf(stderr, "Namespace %s not found\n", >>>>> namespace); >>>>> diff --git a/src/libuboot.h b/src/libuboot.h >>>>> index 624800a..3ed3244 100644 >>>>> --- a/src/libuboot.h >>>>> +++ b/src/libuboot.h >>>>> @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, >>>>> const char *config); >>>>> */ >>>>> int libuboot_read_config_ext(struct uboot_ctx **ctx, const char >>>>> *config); >>>>> >>>>> -/** @brief Get ctx from list - this is maintained for compatibility >>>>> +/** @brief Get ctx from namespace >>>>> * >>>>> * @param[in] ctxlist libuboot context array >>>>> * @param[in] name name identifier for the single ctx >>>>> @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx >>>>> **ctx, const char *config); >>>>> */ >>>>> struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, >>>>> const char *name); >>>>> >>>>> +/** @brief Look for bootloader namespace from DT >>>>> + * >>>>> + * @param[in] ctxlist libuboot context array >>>>> + * @param[in] name name identifier for the single ctx >>>>> + * @return 0 in case of success, else negative value >>>>> + */ >>>>> +const char *libuboot_namespace_from_dt(void); >>>>> >>>>> /** @brief Read U-Boot environment configuration from structure >>>>> * >>>>> diff --git a/src/uboot_env.c b/src/uboot_env.c >>>>> index b924c49..34caeab 100644 >>>>> --- a/src/uboot_env.c >>>>> +++ b/src/uboot_env.c >>>>> @@ -1888,6 +1888,23 @@ struct uboot_ctx >>>>> *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * >>>>> return NULL; >>>>> } >>>>> >>>>> +const char *libuboot_namespace_from_dt(void) >>>>> +{ >>>>> + FILE *fp; >>>>> + char dt_namespace[64]; >>>>> + size_t dt_ret; >>>>> + >>>>> + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); >>>>> + if (!fp) >>>>> + return NULL; >>>>> + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); >>>>> + fclose(fp); >>>>> + if (!dt_ret) >>>>> + return NULL; >>>>> + dt_namespace[dt_ret] = 0; >>>>> + return strdup(dt_namespace); >>>> >>>> strdup() does allocate memory that needs to be freed somewhere, right? >>>> Or am I missing something? >>> >>> Yes, my idea (like for context) is that the library allocates the >>> struct, and the caller should free it. >> >> Ok, but above in fw_printenv.c libuboot_namespace_from_dt() is called >> without free() anywhere, no? > > Well, as many tools are doing when getopt() is parsed. The hidden way is > to rely on the OS, because the tool runs in one shot and after the > process ends, resources are freed automatically by the kernel. This is > done by many tools on linux, and same is done here. If you see even > scriptfile, defenvfile are not explicitely released. > Ok, if that's how things are done usually, that's fine for me. > >> >>> >>>> >>>> Also wouldn't it be better to allocate a buffer for the string right at >>>> the beginning instead of putting the string on the stack first and then >>>> copy it to a buffer that is returned to the caller? >>> >>> I will change in this way.> >>> Regards, >>> Stefano >>> >>>> >>>> Thanks >>>> Frieder >>>> >>>>> +} >>>>> + >>>>> int libuboot_initialize(struct uboot_ctx **out, >>>>> struct uboot_env_device *envdevs) { >>>>> struct uboot_ctx *ctx; >>>>> -- >>>>> 2.34.1 >>>>> >>> >
diff --git a/src/fw_printenv.c b/src/fw_printenv.c index 6118324..6a7a987 100644 --- a/src/fw_printenv.c +++ b/src/fw_printenv.c @@ -77,7 +77,7 @@ int main (int argc, char **argv) { char *cfgfname = NULL; char *defenvfile = NULL; char *scriptfile = NULL; - char *namespace = NULL; + const char *namespace = NULL; int c, i; int ret = 0; void *tmp; @@ -87,9 +87,6 @@ int main (int argc, char **argv) { bool noheader = false; bool default_used = false; struct uboot_version_info *version; - char dt_namespace[32]; - size_t dt_ret; - FILE *fp; /* * As old tool, there is just a tool with symbolic link @@ -148,20 +145,11 @@ int main (int argc, char **argv) { exit(1); } + if (!namespace) + namespace = libuboot_namespace_from_dt(); + if (namespace) ctx = libuboot_get_namespace(ctx, namespace); - else { - fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); - if(fp) { - dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); - if (dt_ret) { - dt_namespace[dt_ret] = 0; - ctx = libuboot_get_namespace(ctx, dt_namespace); - } - - fclose(fp); - } - } if (!ctx) { fprintf(stderr, "Namespace %s not found\n", namespace); diff --git a/src/libuboot.h b/src/libuboot.h index 624800a..3ed3244 100644 --- a/src/libuboot.h +++ b/src/libuboot.h @@ -67,7 +67,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config); */ int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); -/** @brief Get ctx from list - this is maintained for compatibility +/** @brief Get ctx from namespace * * @param[in] ctxlist libuboot context array * @param[in] name name identifier for the single ctx @@ -75,6 +75,13 @@ int libuboot_read_config_ext(struct uboot_ctx **ctx, const char *config); */ struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char *name); +/** @brief Look for bootloader namespace from DT + * + * @param[in] ctxlist libuboot context array + * @param[in] name name identifier for the single ctx + * @return 0 in case of success, else negative value + */ +const char *libuboot_namespace_from_dt(void); /** @brief Read U-Boot environment configuration from structure * diff --git a/src/uboot_env.c b/src/uboot_env.c index b924c49..34caeab 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -1888,6 +1888,23 @@ struct uboot_ctx *libuboot_get_namespace(struct uboot_ctx *ctxlist, const char * return NULL; } +const char *libuboot_namespace_from_dt(void) +{ + FILE *fp; + char dt_namespace[64]; + size_t dt_ret; + + fp = fopen("/proc/device-tree/chosen/u-boot,env-config", "r"); + if (!fp) + return NULL; + dt_ret = fread(dt_namespace, 1, sizeof(dt_namespace) - 1, fp); + fclose(fp); + if (!dt_ret) + return NULL; + dt_namespace[dt_ret] = 0; + return strdup(dt_namespace); +} + int libuboot_initialize(struct uboot_ctx **out, struct uboot_env_device *envdevs) { struct uboot_ctx *ctx;
Move code from fw_printenv to livrary. libuboot_namespace_from_dt looks into DT and reads the namespace for the bootloader, if any. Signed-off-by: Stefano Babic <stefano.babic@swupdate.org> Suggested-by: Frieder Schrempf <frieder.schrempf@kontron.de> --- src/fw_printenv.c | 20 ++++---------------- src/libuboot.h | 9 ++++++++- src/uboot_env.c | 17 +++++++++++++++++ 3 files changed, 29 insertions(+), 17 deletions(-)