Skip to contentSkip to conversation

xl: Don't output a broken result if error occurs in 'xl info'

PATCH In review Yu Zhiguo 2 participants 3 messages Apr 26, 2010 – Apr 28, 2010

lore
Share & tools
lore

Yu Zhiguo

author
#0 lore raw

PATCH 1/1 xl: Don't output a broken result if error occurs in 'xl info'

If error occurs in 'xl info', we'd better output an error message rather than a broken result.

tools/libxl/xl.c +61 -66
diff -r c87ec146229a -r 3f97bcb08bbd tools/libxl/xl.c
@@ -2642,93 +2642,88 @@
     exit(0);
 }
 
-static void output_xeninfo(void)
+static void output_info(struct utsname *utsinfo, struct libxl_physinfo *physinfo,
+                        const libxl_version_info *verinfo)
 {
-    const libxl_version_info *info;
+    unsigned int i;
     int sched_id;
 
-    info = libxl_get_version_info(&ctx);
     if ((sched_id = libxl_get_sched_id(&ctx)) < 0) {
         fprintf(stderr, "get_sched_id sysctl failed.\n");
         return;
     }
 
-    printf("xen_major              : %d\n", info->xen_version_major);
-    printf("xen_minor              : %d\n", info->xen_version_minor);
-    printf("xen_extra              : %s\n", info->xen_version_extra);
-    printf("xen_caps               : %s\n", info->capabilities);
+    // output utsinfo
+    printf("host                   : %s\n", utsinfo->nodename);
+    printf("release                : %s\n", utsinfo->release);
+    printf("version                : %s\n", utsinfo->version);
+    printf("machine                : %s\n", utsinfo->machine);
+
+    // output physinfo
+    printf("nr_cpus                : %d\n", physinfo->nr_cpus);
+    printf("nr_nodes               : %d\n", physinfo->nr_nodes);
+    printf("cores_per_socket       : %d\n", physinfo->cores_per_socket);
+    printf("threads_per_core       : %d\n", physinfo->threads_per_core);
+    printf("cpu_mhz                : %d\n", physinfo->cpu_khz / 1000);
+    printf("hw_caps                : ");
+    for (i = 0; i < 8; i++)
+        printf("%08x%c", physinfo->hw_cap[i], i < 7 ? ':' : '\n');
+    printf("virt_caps              :");
+    if (physinfo->phys_cap & XEN_SYSCTL_PHYSCAP_hvm)
+        printf(" hvm");
+    if (physinfo->phys_cap & XEN_SYSCTL_PHYSCAP_hvm_directio)
+        printf(" hvm_directio");
+    printf("\n");
+    if (!verinfo)
+        return;
+    i = (1 << 20) / verinfo->pagesize;
+    printf("total_memory           : %llu\n", (long long unsigned int) physinfo->total_pages / i);
+    printf("free_memory            : %llu\n", (long long unsigned int) physinfo->free_pages / i);
+
+    // output verinfo
+    printf("xen_major              : %d\n", verinfo->xen_version_major);
+    printf("xen_minor              : %d\n", verinfo->xen_version_minor);
+    printf("xen_extra              : %s\n", verinfo->xen_version_extra);
+    printf("xen_caps               : %s\n", verinfo->capabilities);
     printf("xen_scheduler          : %s\n",
         sched_id == XEN_SCHEDULER_SEDF ? "sedf" :
         sched_id == XEN_SCHEDULER_CREDIT ? "credit" :
         sched_id == XEN_SCHEDULER_CREDIT2 ? "credit2" : "unknown");
-    printf("xen_pagesize           : %lu\n", info->pagesize);
-    printf("platform_params        : virt_start=0x%lx\n", info->virt_start);
-    printf("xen_changeset          : %s\n", info->changeset);
-    printf("xen_commandline        : %s\n", info->commandline);
-    printf("cc_compiler            : %s\n", info->compiler);
-    printf("cc_compile_by          : %s\n", info->compile_by);
-    printf("cc_compile_domain      : %s\n", info->compile_domain);
-    printf("cc_compile_date        : %s\n", info->compile_date);
+    printf("xen_pagesize           : %lu\n", verinfo->pagesize);
+    printf("platform_params        : virt_start=0x%lx\n", verinfo->virt_start);
+    printf("xen_changeset          : %s\n", verinfo->changeset);
+    printf("xen_commandline        : %s\n", verinfo->commandline);
+    printf("cc_compiler            : %s\n", verinfo->compiler);
+    printf("cc_compile_by          : %s\n", verinfo->compile_by);
+    printf("cc_compile_domain      : %s\n", verinfo->compile_domain);
+    printf("cc_compile_date        : %s\n", verinfo->compile_date);
 
-    return;
-}
-
-static void output_nodeinfo(void)
-{
-    struct utsname utsbuf;
-
-    uname(&utsbuf);
-
-    printf("host                   : %s\n", utsbuf.nodename);
-    printf("release                : %s\n", utsbuf.release);
-    printf("version                : %s\n", utsbuf.version);
-    printf("machine                : %s\n", utsbuf.machine);
-
-    return;
-}
-
-static void output_physinfo(void)
-{
-    struct libxl_physinfo info;
-    const libxl_version_info *vinfo;
-    unsigned int i;
-
-    if (libxl_get_physinfo(&ctx, &info) != 0) {
-        fprintf(stderr, "libxl_physinfo failed.\n");
-        return;
-    }
-
-    printf("nr_cpus                : %d\n", info.nr_cpus);
-    printf("nr_nodes               : %d\n", info.nr_nodes);
-    printf("cores_per_socket       : %d\n", info.cores_per_socket);
-    printf("threads_per_core       : %d\n", info.threads_per_core);
-    printf("cpu_mhz                : %d\n", info.cpu_khz / 1000);
-    printf("hw_caps                : ");
-    for (i = 0; i < 8; i++)
-        printf("%08x%c", info.hw_cap[i], i < 7 ? ':' : '\n');
-    printf("virt_caps              :");
-    if (info.phys_cap & XEN_SYSCTL_PHYSCAP_hvm)
-        printf(" hvm");
-    if (info.phys_cap & XEN_SYSCTL_PHYSCAP_hvm_directio)
-        printf(" hvm_directio");
-    printf("\n");
-    vinfo = libxl_get_version_info(&ctx);
-    i = (1 << 20) / vinfo->pagesize;
-    printf("total_memory           : %lu\n", info.total_pages / i);
-    printf("free_memory            : %lu\n", info.free_pages / i);
+    printf("xend_config_format     : 4\n");
 
     return;
 }
 
 void info(int verbose)
 {
-    output_nodeinfo();
+    struct utsname utsinfo;
+    struct libxl_physinfo physinfo;
+    const libxl_version_info *verinfo;
 
-    output_physinfo();
+    if (uname(&utsinfo) != 0) {
+        fprintf(stderr, "cannot get nodeinfo\n");
+        return;
+    }
+    if (libxl_get_physinfo(&ctx, &physinfo) != 0) {
+        fprintf(stderr, "libxl_physinfo failed.\n");
+        return;
+    }
+    verinfo = libxl_get_version_info(&ctx);
+    if (!verinfo) {
+        fprintf(stderr, "libxl_get_version_info failed.\n");
+        return;
+    }
 
-    output_xeninfo();
-
-    printf("xend_config_format     : 4\n");
+    output_info(&utsinfo, &physinfo, verinfo);
 
     return;
 }
#1 lore raw
Quoted 2 lines · Yu Zhiguo

If error occurs in 'xl info', we'd better output an error message rather than a broken result.

Do you consider an incomplete output as broken? If I got this right, you want to make sure that you output everything or nothing with this patch. Wouldn't it be sufficient to just add the missing check for utsname() and output the other infos regardless? Was there a scenario where broken output was generated? So far I only see missing Xen support or missing privileges to cause errors.

Regards,
Andre.

Quoted 160 lines

diff -r c87ec146229a -r 3f97bcb08bbd tools/libxl/xl.c

tools/libxl/xl.c +61 -66
@@ -2642,93 +2642,88 @@
     exit(0);
 }
 
-static void output_xeninfo(void)
+static void output_info(struct utsname *utsinfo, struct libxl_physinfo *physinfo,
+                        const libxl_version_info *verinfo)
 {
-    const libxl_version_info *info;
+    unsigned int i;
     int sched_id;
 
-    info = libxl_get_version_info(&ctx);
     if ((sched_id = libxl_get_sched_id(&ctx)) < 0) {
         fprintf(stderr, "get_sched_id sysctl failed.\n");
         return;
     }
 
-    printf("xen_major              : %d\n", info->xen_version_major);
-    printf("xen_minor              : %d\n", info->xen_version_minor);
-    printf("xen_extra              : %s\n", info->xen_version_extra);
-    printf("xen_caps               : %s\n", info->capabilities);
+    // output utsinfo
+    printf("host                   : %s\n", utsinfo->nodename);
+    printf("release                : %s\n", utsinfo->release);
+    printf("version                : %s\n", utsinfo->version);
+    printf("machine                : %s\n", utsinfo->machine);
+
+    // output physinfo
+    printf("nr_cpus                : %d\n", physinfo->nr_cpus);
+    printf("nr_nodes               : %d\n", physinfo->nr_nodes);
+    printf("cores_per_socket       : %d\n", physinfo->cores_per_socket);
+    printf("threads_per_core       : %d\n", physinfo->threads_per_core);
+    printf("cpu_mhz                : %d\n", physinfo->cpu_khz / 1000);
+    printf("hw_caps                : ");
+    for (i = 0; i < 8; i++)
+        printf("%08x%c", physinfo->hw_cap[i], i < 7 ? ':' : '\n');
+    printf("virt_caps              :");
+    if (physinfo->phys_cap & XEN_SYSCTL_PHYSCAP_hvm)
+        printf(" hvm");
+    if (physinfo->phys_cap & XEN_SYSCTL_PHYSCAP_hvm_directio)
+        printf(" hvm_directio");
+    printf("\n");
+    if (!verinfo)
+        return;
+    i = (1 << 20) / verinfo->pagesize;
+    printf("total_memory           : %llu\n", (long long unsigned int) physinfo->total_pages / i);
+    printf("free_memory            : %llu\n", (long long unsigned int) physinfo->free_pages / i);
+
+    // output verinfo
+    printf("xen_major              : %d\n", verinfo->xen_version_major);
+    printf("xen_minor              : %d\n", verinfo->xen_version_minor);
+    printf("xen_extra              : %s\n", verinfo->xen_version_extra);
+    printf("xen_caps               : %s\n", verinfo->capabilities);
     printf("xen_scheduler          : %s\n",
         sched_id == XEN_SCHEDULER_SEDF ? "sedf" :
         sched_id == XEN_SCHEDULER_CREDIT ? "credit" :
         sched_id == XEN_SCHEDULER_CREDIT2 ? "credit2" : "unknown");
-    printf("xen_pagesize           : %lu\n", info->pagesize);
-    printf("platform_params        : virt_start=0x%lx\n", info->virt_start);
-    printf("xen_changeset          : %s\n", info->changeset);
-    printf("xen_commandline        : %s\n", info->commandline);
-    printf("cc_compiler            : %s\n", info->compiler);
-    printf("cc_compile_by          : %s\n", info->compile_by);
-    printf("cc_compile_domain      : %s\n", info->compile_domain);
-    printf("cc_compile_date        : %s\n", info->compile_date);
+    printf("xen_pagesize           : %lu\n", verinfo->pagesize);
+    printf("platform_params        : virt_start=0x%lx\n", verinfo->virt_start);
+    printf("xen_changeset          : %s\n", verinfo->changeset);
+    printf("xen_commandline        : %s\n", verinfo->commandline);
+    printf("cc_compiler            : %s\n", verinfo->compiler);
+    printf("cc_compile_by          : %s\n", verinfo->compile_by);
+    printf("cc_compile_domain      : %s\n", verinfo->compile_domain);
+    printf("cc_compile_date        : %s\n", verinfo->compile_date);
 
-    return;
-}
-
-static void output_nodeinfo(void)
-{
-    struct utsname utsbuf;
-
-    uname(&utsbuf);
-
-    printf("host                   : %s\n", utsbuf.nodename);
-    printf("release                : %s\n", utsbuf.release);
-    printf("version                : %s\n", utsbuf.version);
-    printf("machine                : %s\n", utsbuf.machine);
-
-    return;
-}
-
-static void output_physinfo(void)
-{
-    struct libxl_physinfo info;
-    const libxl_version_info *vinfo;
-    unsigned int i;
-
-    if (libxl_get_physinfo(&ctx, &info) != 0) {
-        fprintf(stderr, "libxl_physinfo failed.\n");
-        return;
-    }
-
-    printf("nr_cpus                : %d\n", info.nr_cpus);
-    printf("nr_nodes               : %d\n", info.nr_nodes);
-    printf("cores_per_socket       : %d\n", info.cores_per_socket);
-    printf("threads_per_core       : %d\n", info.threads_per_core);
-    printf("cpu_mhz                : %d\n", info.cpu_khz / 1000);
-    printf("hw_caps                : ");
-    for (i = 0; i < 8; i++)
-        printf("%08x%c", info.hw_cap[i], i < 7 ? ':' : '\n');
-    printf("virt_caps              :");
-    if (info.phys_cap & XEN_SYSCTL_PHYSCAP_hvm)
-        printf(" hvm");
-    if (info.phys_cap & XEN_SYSCTL_PHYSCAP_hvm_directio)
-        printf(" hvm_directio");
-    printf("\n");
-    vinfo = libxl_get_version_info(&ctx);
-    i = (1 << 20) / vinfo->pagesize;
-    printf("total_memory           : %lu\n", info.total_pages / i);
-    printf("free_memory            : %lu\n", info.free_pages / i);
+    printf("xend_config_format     : 4\n");
 
     return;
 }
 
 void info(int verbose)
 {
-    output_nodeinfo();
+    struct utsname utsinfo;
+    struct libxl_physinfo physinfo;
+    const libxl_version_info *verinfo;
 
-    output_physinfo();
+    if (uname(&utsinfo) != 0) {
+        fprintf(stderr, "cannot get nodeinfo\n");
+        return;
+    }
+    if (libxl_get_physinfo(&ctx, &physinfo) != 0) {
+        fprintf(stderr, "libxl_physinfo failed.\n");
+        return;
+    }
+    verinfo = libxl_get_version_info(&ctx);
+    if (!verinfo) {
+        fprintf(stderr, "libxl_get_version_info failed.\n");
+        return;
+    }
 
-    output_xeninfo();
-
-    printf("xend_config_format     : 4\n");
+    output_info(&utsinfo, &physinfo, verinfo);
 
     return;
 }
Signature · 3 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
author
#2 lore raw

PATCH 1/1 xl: sanity check for 'xl info'

Quoted 9 lines · Andre Przywara
Quoted 2 lines · Yu Zhiguo

If error occurs in 'xl info', we'd better output an error message rather than a broken result.

Do you consider an incomplete output as broken? If I got this right, you want to make sure that you output everything or nothing with this patch. Wouldn't it be sufficient to just add the missing check for utsname() and output the other infos regardless? Was there a scenario where broken output was generated? So far I only see missing Xen support or missing privileges to cause errors.

It seems that xm will always output all or nothing, so I think we'd better compatible with xm. But it isn't so important, just add sanity check now.

Regards
Yu Zhiguo

--------------------------------------

Add sanity check for 'xl info'.

tools/libxl/xl.c +15 -10
diff -r c87ec146229a -r c4a684b35c7b tools/libxl/xl.c
@@ -2647,7 +2647,10 @@
     const libxl_version_info *info;
     int sched_id;
 
-    info = libxl_get_version_info(&ctx);
+    if (!(info = libxl_get_version_info(&ctx))) {
+        fprintf(stderr, "libxl_get_version_info failed.\n");
+        return;
+    }
     if ((sched_id = libxl_get_sched_id(&ctx)) < 0) {
         fprintf(stderr, "get_sched_id sysctl failed.\n");
         return;
@@ -2677,12 +2680,12 @@
 {
     struct utsname utsbuf;
 
-    uname(&utsbuf);
-
-    printf("host                   : %s\n", utsbuf.nodename);
-    printf("release                : %s\n", utsbuf.release);
-    printf("version                : %s\n", utsbuf.version);
-    printf("machine                : %s\n", utsbuf.machine);
+    if (uname(&utsbuf) != -1) {
+        printf("host                   : %s\n", utsbuf.nodename);
+        printf("release                : %s\n", utsbuf.release);
+        printf("version                : %s\n", utsbuf.version);
+        printf("machine                : %s\n", utsbuf.machine);
+    }
 
     return;
 }
@@ -2713,9 +2716,11 @@
         printf(" hvm_directio");
     printf("\n");
     vinfo = libxl_get_version_info(&ctx);
-    i = (1 << 20) / vinfo->pagesize;
-    printf("total_memory           : %lu\n", info.total_pages / i);
-    printf("free_memory            : %lu\n", info.free_pages / i);
+    if (vinfo) {
+        i = (1 << 20) / vinfo->pagesize;
+        printf("total_memory           : %lu\n", info.total_pages / i);
+        printf("free_memory            : %lu\n", info.free_pages / i);
+    }
 
     return;
 }