Re: [PATCH 1/2] nodeinfo: Fix code style and some minor bugs.

[Osier Yang <jyang@xxxxxxxxxx>]

On 2012年06月25日 22:43, Peter Krempa wrote:
> This patch cleans up some line breaks and fixes two minor bugs:
>
> - buffer pointer increment to the actual length that should be skipped
> - jump to cleanup section instead of returning -1
> ---
>   src/nodeinfo.c |   31 +++++++++++++++----------------
>   1 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index f7d0cc6..97482d9 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -254,19 +254,19 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>               char *p;
>               unsigned int ui;
>
> -            buf += 9;
> +            buf += 7;
>               while (*buf&&  c_isspace(*buf))
>                   buf++;
>
>               if (*buf != ':' || !buf[1]) {
> -                nodeReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("parsing cpu MHz from cpuinfo"));
> +                nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                _("parsing cpu MHz from cpuinfo"));
>                   goto cleanup;
>               }
>
> -            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0
> +            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0&&
>                   /* Accept trailing fractional part.  */
> -&&  (*p == '\0' || *p == '.' || c_isspace(*p)))
> +                (*p == '\0' || *p == '.' || c_isspace(*p)))
>                   nodeinfo->mhz = ui;
>           }
>
> @@ -279,13 +279,13 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>                   buf++;
>
>               if (*buf != ':' || !buf[1]) {
> -                nodeReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("parsing cpu cores from cpuinfo"));
> -                return -1;
> +                nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                _("parsing cpu cores from cpuinfo"));
> +                goto cleanup;

Instead of chaning "return -1" into "goto cleanup", I think
"goto cleanup" should be changed into "return -1". "sysfs_cpudir"
is NULL anyway before the while loop finished.

>               }
>
> -            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0
> -&&  (*p == '\0' || c_isspace(*p)))
> +            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0&&
> +                (*p == '\0' || c_isspace(*p)))
>                   cpu_cores = ui;
>            }
>   # elif defined(__powerpc__) || \
> @@ -300,14 +300,14 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>                   buf++;
>
>               if (*buf != ':' || !buf[1]) {
> -                nodeReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("parsing cpu MHz from cpuinfo"));
> +                nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                _("parsing cpu MHz from cpuinfo"));
>                   goto cleanup;
>               }
>
> -            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0
> +            if (virStrToLong_ui(buf+1,&p, 10,&ui) == 0&&
>                   /* Accept trailing fractional part.  */
> -&&  (*p == '\0' || *p == '.' || c_isspace(*p)))
> +                (*p == '\0' || *p == '.' || c_isspace(*p)))
>                   nodeinfo->mhz = ui;
>               /* No other interesting infos are available in /proc/cpuinfo.
>                * However, there is a line identifying processor's version,
> @@ -327,8 +327,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>           virReportOOMError();
>           goto cleanup;
>       }
> -    cpudir = opendir(sysfs_cpudir);
> -    if (cpudir == NULL) {
> +    if (!(cpudir = opendir(sysfs_cpudir))) {
>           virReportSystemError(errno, _("cannot opendir %s"), sysfs_cpudir);
>           goto cleanup;
>       }

Others are nice cleanups.

Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list