| Message ID | 1440516404-4177-5-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/25/15 18:26, Mike Holmes wrote: > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/odp_system_info.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c > index 9e217ff..1919af9 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -95,7 +95,10 @@ static int huge_page_size(void) > while ((dirent = readdir(dir)) != NULL) { > int temp = 0; > > - sscanf(dirent->d_name, "hugepages-%i", &temp); > + if (sscanf(dirent->d_name, "hugepages-%i", &temp) == EOF) { > + ODP_ERR("failed to read hugepages\n"); > + return 0; > + } > __odp_err needs to be set to errno in all cases. scanf can also return 0. So it's better to check that it returns 1 as requested for input. > if (temp > size) > size = temp; > @@ -126,7 +129,10 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t *sysinfo) > if (!mhz) { > pos = strstr(str, "cpu MHz"); > if (pos) { > - sscanf(pos, "cpu MHz : %lf", &mhz); > + if (sscanf(pos, "cpu MHz : %lf", &mhz) == EOF) { > + ODP_ERR("failed to read cpu MHz\n"); > + return -1; > + } > count--; > } > } > @@ -169,13 +175,18 @@ static int cpuinfo_octeon(FILE *file, odp_system_info_t *sysinfo) > double mhz = 0.0; > int model = 0; > int count = 2; > + int result; int ret; (we use ret everywhere in code, not result). > > while (fgets(str, sizeof(str), file) && count > 0) { > if (!mhz) { > pos = strstr(str, "BogoMIPS"); > > if (pos) { > - sscanf(pos, "BogoMIPS : %lf", &mhz); > + result = sscanf(pos, "BogoMIPS : %lf", &mhz); > + if (result == EOF) { > + ODP_ERR("failed to read BogoMIPSn"); > + return -1; > + } > count--; > } > } > @@ -216,7 +227,10 @@ static int cpuinfo_powerpc(FILE *file, odp_system_info_t *sysinfo) > pos = strstr(str, "clock"); > > if (pos) { > - sscanf(pos, "clock : %lf", &mhz); > + if (sscanf(pos, "clock : %lf", &mhz) == EOF) { > + ODP_ERR("failed to read clock"); > + return -1; > + } can you change to the same code in everywhere. If you use ret = scanf in one place, please use it in others same functions. Thanks, Maxim. > count--; > } > }
On 25 August 2015 at 15:06, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 08/25/15 18:26, Mike Holmes wrote: > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> platform/linux-generic/odp_system_info.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index 9e217ff..1919af9 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -95,7 +95,10 @@ static int huge_page_size(void) >> while ((dirent = readdir(dir)) != NULL) { >> int temp = 0; >> - sscanf(dirent->d_name, "hugepages-%i", &temp); >> + if (sscanf(dirent->d_name, "hugepages-%i", &temp) == EOF) >> { >> + ODP_ERR("failed to read hugepages\n"); >> + return 0; >> + } >> >> > __odp_err needs to be set to errno in all cases. > Sure I can set that > > scanf can also return 0. So it's better to check that it returns 1 as > requested for input. > So we will drop EOF, no problem > > > if (temp > size) >> size = temp; >> @@ -126,7 +129,10 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t >> *sysinfo) >> if (!mhz) { >> pos = strstr(str, "cpu MHz"); >> if (pos) { >> - sscanf(pos, "cpu MHz : %lf", &mhz); >> + if (sscanf(pos, "cpu MHz : %lf", &mhz) == >> EOF) { >> + ODP_ERR("failed to read cpu >> MHz\n"); >> + return -1; >> + } >> count--; >> } >> } >> @@ -169,13 +175,18 @@ static int cpuinfo_octeon(FILE *file, >> odp_system_info_t *sysinfo) >> double mhz = 0.0; >> int model = 0; >> int count = 2; >> + int result; >> > int ret; (we use ret everywhere in code, not result). > > > while (fgets(str, sizeof(str), file) && count > 0) { >> if (!mhz) { >> pos = strstr(str, "BogoMIPS"); >> if (pos) { >> - sscanf(pos, "BogoMIPS : %lf", &mhz); >> + result = sscanf(pos, "BogoMIPS : %lf", >> &mhz); >> + if (result == EOF) { >> + ODP_ERR("failed to read >> BogoMIPSn"); >> + return -1; >> + } >> count--; >> } >> } >> @@ -216,7 +227,10 @@ static int cpuinfo_powerpc(FILE *file, >> odp_system_info_t *sysinfo) >> pos = strstr(str, "clock"); >> if (pos) { >> - sscanf(pos, "clock : %lf", &mhz); >> + if (sscanf(pos, "clock : %lf", &mhz) == >> EOF) { >> + ODP_ERR("failed to read clock"); >> + return -1; >> + } >> > > can you change to the same code in everywhere. If you use ret = scanf in > one place, > please use it in others same functions. > It is only used to over come 80 char limit, it complicates the code to have it for no reason elsewhere don’t you think ? > > Thanks, > Maxim. > > count--; >> } >> } >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 08/25/15 22:42, Mike Holmes wrote: > > > On 25 August 2015 at 15:06, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 08/25/15 18:26, Mike Holmes wrote: > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> > --- > platform/linux-generic/odp_system_info.c | 22 > ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index 9e217ff..1919af9 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -95,7 +95,10 @@ static int huge_page_size(void) > while ((dirent = readdir(dir)) != NULL) { > int temp = 0; > - sscanf(dirent->d_name, "hugepages-%i", &temp); > + if (sscanf(dirent->d_name, "hugepages-%i", > &temp) == EOF) { > + ODP_ERR("failed to read hugepages\n"); > + return 0; > + } > > __odp_err needs to be set to errno in all cases. > > > Sure I can set that > > > scanf can also return 0. So it's better to check that it returns 1 > as requested for input. > > > So we will drop EOF, no problem > > > > if (temp > size) > size = temp; > @@ -126,7 +129,10 @@ static int cpuinfo_x86(FILE *file, > odp_system_info_t *sysinfo) > if (!mhz) { > pos = strstr(str, "cpu MHz"); > if (pos) { > - sscanf(pos, "cpu MHz : %lf", > &mhz); > + if (sscanf(pos, "cpu MHz : > %lf", &mhz) == EOF) { > + ODP_ERR("failed to read cpu MHz\n"); > + return -1; > + } > count--; > } > } > @@ -169,13 +175,18 @@ static int cpuinfo_octeon(FILE *file, > odp_system_info_t *sysinfo) > double mhz = 0.0; > int model = 0; > int count = 2; > + int result; > > int ret; (we use ret everywhere in code, not result). > > > while (fgets(str, sizeof(str), file) && count > 0) { > if (!mhz) { > pos = strstr(str, "BogoMIPS"); > if (pos) { > - sscanf(pos, "BogoMIPS : %lf", > &mhz); > + result = sscanf(pos, "BogoMIPS > : %lf", &mhz); > + if (result == EOF) { > + ODP_ERR("failed to read BogoMIPSn"); > + return -1; > + } > count--; > } > } > @@ -216,7 +227,10 @@ static int cpuinfo_powerpc(FILE *file, > odp_system_info_t *sysinfo) > pos = strstr(str, "clock"); > if (pos) { > - sscanf(pos, "clock : %lf", &mhz); > + if (sscanf(pos, "clock : %lf", > &mhz) == EOF) { > + ODP_ERR("failed to read clock"); > + return -1; > + } > > > can you change to the same code in everywhere. If you use ret = > scanf in one place, > please use it in others same functions. > > > It is only used to over come 80 char limit, it complicates the code to > have it for no reason elsewhere don’t you think ? yes, I understand the reason, but it's better to have same style for same code. Just like you fixed that new fixes will be the same. Maxim. > > > Thanks, > Maxim. > > count--; > } > } > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs >
On 25 August 2015 at 15:50, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 08/25/15 22:42, Mike Holmes wrote: > >> >> >> On 25 August 2015 at 15:06, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 08/25/15 18:26, Mike Holmes wrote: >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>> >> >> --- >> platform/linux-generic/odp_system_info.c | 22 >> ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index 9e217ff..1919af9 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -95,7 +95,10 @@ static int huge_page_size(void) >> while ((dirent = readdir(dir)) != NULL) { >> int temp = 0; >> - sscanf(dirent->d_name, "hugepages-%i", &temp); >> + if (sscanf(dirent->d_name, "hugepages-%i", >> &temp) == EOF) { >> + ODP_ERR("failed to read hugepages\n"); >> + return 0; >> + } >> >> __odp_err needs to be set to errno in all cases. >> >> >> Sure I can set that >> >> >> scanf can also return 0. So it's better to check that it returns 1 >> as requested for input. >> >> >> So we will drop EOF, no problem >> >> >> >> if (temp > size) >> size = temp; >> @@ -126,7 +129,10 @@ static int cpuinfo_x86(FILE *file, >> odp_system_info_t *sysinfo) >> if (!mhz) { >> pos = strstr(str, "cpu MHz"); >> if (pos) { >> - sscanf(pos, "cpu MHz : %lf", >> &mhz); >> + if (sscanf(pos, "cpu MHz : >> %lf", &mhz) == EOF) { >> + ODP_ERR("failed to read cpu MHz\n"); >> + return -1; >> + } >> count--; >> } >> } >> @@ -169,13 +175,18 @@ static int cpuinfo_octeon(FILE *file, >> odp_system_info_t *sysinfo) >> double mhz = 0.0; >> int model = 0; >> int count = 2; >> + int result; >> >> int ret; (we use ret everywhere in code, not result). >> >> >> while (fgets(str, sizeof(str), file) && count > 0) { >> if (!mhz) { >> pos = strstr(str, "BogoMIPS"); >> if (pos) { >> - sscanf(pos, "BogoMIPS : %lf", >> &mhz); >> + result = sscanf(pos, "BogoMIPS >> : %lf", &mhz); >> + if (result == EOF) { >> + ODP_ERR("failed to read BogoMIPSn"); >> + return -1; >> + } >> count--; >> } >> } >> @@ -216,7 +227,10 @@ static int cpuinfo_powerpc(FILE *file, >> odp_system_info_t *sysinfo) >> pos = strstr(str, "clock"); >> if (pos) { >> - sscanf(pos, "clock : %lf", &mhz); >> + if (sscanf(pos, "clock : %lf", >> &mhz) == EOF) { >> + ODP_ERR("failed to read clock"); >> + return -1; >> + } >> >> >> can you change to the same code in everywhere. If you use ret = >> scanf in one place, >> please use it in others same functions. >> >> >> It is only used to over come 80 char limit, it complicates the code to >> have it for no reason elsewhere don’t you think ? >> > > yes, I understand the reason, but it's better to have same style for same > code. Just like you fixed that new fixes will be the same. > Why is it better, it is arbitrary, I think it is better to have simple code and you think it is better to have it look the same :) Lets argue over a HO :) > > > Maxim. > > >> >> Thanks, >> Maxim. >> >> count--; >> } >> } >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> >
diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c index 9e217ff..1919af9 100644 --- a/platform/linux-generic/odp_system_info.c +++ b/platform/linux-generic/odp_system_info.c @@ -95,7 +95,10 @@ static int huge_page_size(void) while ((dirent = readdir(dir)) != NULL) { int temp = 0; - sscanf(dirent->d_name, "hugepages-%i", &temp); + if (sscanf(dirent->d_name, "hugepages-%i", &temp) == EOF) { + ODP_ERR("failed to read hugepages\n"); + return 0; + } if (temp > size) size = temp; @@ -126,7 +129,10 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t *sysinfo) if (!mhz) { pos = strstr(str, "cpu MHz"); if (pos) { - sscanf(pos, "cpu MHz : %lf", &mhz); + if (sscanf(pos, "cpu MHz : %lf", &mhz) == EOF) { + ODP_ERR("failed to read cpu MHz\n"); + return -1; + } count--; } } @@ -169,13 +175,18 @@ static int cpuinfo_octeon(FILE *file, odp_system_info_t *sysinfo) double mhz = 0.0; int model = 0; int count = 2; + int result; while (fgets(str, sizeof(str), file) && count > 0) { if (!mhz) { pos = strstr(str, "BogoMIPS"); if (pos) { - sscanf(pos, "BogoMIPS : %lf", &mhz); + result = sscanf(pos, "BogoMIPS : %lf", &mhz); + if (result == EOF) { + ODP_ERR("failed to read BogoMIPSn"); + return -1; + } count--; } } @@ -216,7 +227,10 @@ static int cpuinfo_powerpc(FILE *file, odp_system_info_t *sysinfo) pos = strstr(str, "clock"); if (pos) { - sscanf(pos, "clock : %lf", &mhz); + if (sscanf(pos, "clock : %lf", &mhz) == EOF) { + ODP_ERR("failed to read clock"); + return -1; + } count--; } }
Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/odp_system_info.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)