[PATCH 3/3]: xl: allow long listing of Domain 0

[PATCH 3/3]: xl: allow long listing of Domain 0

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

Hi,

currently xl list aborts when one tries to list -l Domain 0 (either 
explicitly or by listing all domains):
# xl list -l 0
Neither kernel nor bootloader specified

Ignore this error message (which is invalid for Dom0). I haven't found 
an obvious way to check for Dom0 before printing this message, so I 
simply removed the exit() call here.

Regards,
Andre.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: libxl_list_3.patch --]
[-- Type: text/x-patch, Size: 807 bytes --]

diff -r 8992134dcfd0 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Aug 04 19:24:17 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Aug 09 16:03:33 2010 +0200
@@ -454,7 +454,7 @@
         printf("\t\t)\n");
     } else {
         printf("\t\t(linux %d)\n", b_info->hvm);
-        printf("\t\t\t(kernel %s)\n", b_info->kernel.path);
+        printf("\t\t\t(kernel %s)\n", b_info->kernel.path ?: "");
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);
         printf("\t\t)\n");
@@ -706,7 +706,6 @@
 
         if (!b_info->u.pv.bootloader && !b_info->kernel.path) {
             fprintf(stderr, "Neither kernel nor bootloader specified\n");
-            exit(1);
         }
 
         b_info->u.pv.cmdline = cmdline;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 3/3]: xl: allow long listing of Domain 0'

[Stefano Stabellini]

On Mon, 9 Aug 2010, Andre Przywara wrote:
> Hi,
> 
> currently xl list aborts when one tries to list -l Domain 0 (either 
> explicitly or by listing all domains):
> # xl list -l 0
> Neither kernel nor bootloader specified
> 
> Ignore this error message (which is invalid for Dom0). I haven't found 
> an obvious way to check for Dom0 before printing this message, so I 
> simply removed the exit() call here.
> 

I would rather skip dom0 in the list_domains_details loop, I'll apply a
patch that does that.

Re: [PATCH 3/3]: xl: allow long listing of Domain 0'

[Gianni Tedesco]

On Mon, 2010-08-09 at 17:11 +0100, Stefano Stabellini wrote:
> On Mon, 9 Aug 2010, Andre Przywara wrote:
> > Hi,
> > 
> > currently xl list aborts when one tries to list -l Domain 0 (either 
> > explicitly or by listing all domains):
> > # xl list -l 0
> > Neither kernel nor bootloader specified
> > 
> > Ignore this error message (which is invalid for Dom0). I haven't found 
> > an obvious way to check for Dom0 before printing this message, so I 
> > simply removed the exit() call here.
> > 
> 
> I would rather skip dom0 in the list_domains_details loop, I'll apply a
> patch that does that.

FWIW I think that's the wrong fix. The config parser code ought not be
the place to check for such things and should be handled in libxl (or
perhaps elsewhere in xl) with reasonable error message. Seems like a
work-around rather than a fix to not print dom0 info.

Re: [PATCH 3/3]: xl: allow long listing of Domain 0'

[Stefano Stabellini]

On Mon, 9 Aug 2010, Gianni Tedesco (3P) wrote:
> On Mon, 2010-08-09 at 17:11 +0100, Stefano Stabellini wrote:
> > On Mon, 9 Aug 2010, Andre Przywara wrote:
> > > Hi,
> > > 
> > > currently xl list aborts when one tries to list -l Domain 0 (either 
> > > explicitly or by listing all domains):
> > > # xl list -l 0
> > > Neither kernel nor bootloader specified
> > > 
> > > Ignore this error message (which is invalid for Dom0). I haven't found 
> > > an obvious way to check for Dom0 before printing this message, so I 
> > > simply removed the exit() call here.
> > > 
> > 
> > I would rather skip dom0 in the list_domains_details loop, I'll apply a
> > patch that does that.
> 
> FWIW I think that's the wrong fix. The config parser code ought not be
> the place to check for such things and should be handled in libxl (or
> perhaps elsewhere in xl) with reasonable error message. Seems like a
> work-around rather than a fix to not print dom0 info.
> 
> 

I agree, that's why I added a check on dom0 in list_domains_details.
The alternative would be to add a check on domid == 0 in
libxl_userdata_retrieve, but considering that libxl_userdata_retrieve is
supposed to be a generic libxl function, I preferred
list_domains_details.

Re: [PATCH 3/3]: xl: allow long listing of Domain 0'

[Gianni Tedesco]

On Mon, 2010-08-09 at 18:36 +0100, Stefano Stabellini wrote:
> On Mon, 9 Aug 2010, Gianni Tedesco (3P) wrote:
> > On Mon, 2010-08-09 at 17:11 +0100, Stefano Stabellini wrote:
> > > On Mon, 9 Aug 2010, Andre Przywara wrote:
> > > > Hi,
> > > > 
> > > > currently xl list aborts when one tries to list -l Domain 0 (either 
> > > > explicitly or by listing all domains):
> > > > # xl list -l 0
> > > > Neither kernel nor bootloader specified
> > > > 
> > > > Ignore this error message (which is invalid for Dom0). I haven't found 
> > > > an obvious way to check for Dom0 before printing this message, so I 
> > > > simply removed the exit() call here.
> > > > 
> > > 
> > > I would rather skip dom0 in the list_domains_details loop, I'll apply a
> > > patch that does that.
> > 
> > FWIW I think that's the wrong fix. The config parser code ought not be
> > the place to check for such things and should be handled in libxl (or
> > perhaps elsewhere in xl) with reasonable error message. Seems like a
> > work-around rather than a fix to not print dom0 info.
> > 
> > 
> 
> I agree, that's why I added a check on dom0 in list_domains_details.
> The alternative would be to add a check on domid == 0 in
> libxl_userdata_retrieve, but considering that libxl_userdata_retrieve is
> supposed to be a generic libxl function, I preferred
> list_domains_details.

What I mean is that the kernel vs. bootloader check only affects domain
create path right? Therefore the check ought to be deep in that code and
dom0 not skipped in the list_domains_details().

Re: [PATCH 3/3]: xl: allow long listing of Domain 0'

[Ian Campbell]

On Mon, 2010-08-09 at 18:40 +0100, Gianni Tedesco wrote:
> On Mon, 2010-08-09 at 18:36 +0100, Stefano Stabellini wrote:
> > On Mon, 9 Aug 2010, Gianni Tedesco (3P) wrote:
> > > On Mon, 2010-08-09 at 17:11 +0100, Stefano Stabellini wrote:
> > > > On Mon, 9 Aug 2010, Andre Przywara wrote:
> > > > > Hi,
> > > > > 
> > > > > currently xl list aborts when one tries to list -l Domain 0 (either 
> > > > > explicitly or by listing all domains):
> > > > > # xl list -l 0
> > > > > Neither kernel nor bootloader specified
> > > > > 
> > > > > Ignore this error message (which is invalid for Dom0). I haven't found 
> > > > > an obvious way to check for Dom0 before printing this message, so I 
> > > > > simply removed the exit() call here.
> > > > > 
> > > > 
> > > > I would rather skip dom0 in the list_domains_details loop, I'll apply a
> > > > patch that does that.
> > > 
> > > FWIW I think that's the wrong fix. The config parser code ought not be
> > > the place to check for such things and should be handled in libxl (or
> > > perhaps elsewhere in xl) with reasonable error message. Seems like a
> > > work-around rather than a fix to not print dom0 info.
> > > 
> > > 
> > 
> > I agree, that's why I added a check on dom0 in list_domains_details.
> > The alternative would be to add a check on domid == 0 in
> > libxl_userdata_retrieve, but considering that libxl_userdata_retrieve is
> > supposed to be a generic libxl function, I preferred
> > list_domains_details.
> 
> What I mean is that the kernel vs. bootloader check only affects domain
> create path right? Therefore the check ought to be deep in that code and
> dom0 not skipped in the list_domains_details().

As the one who added the check in question to parse_config_data I agree
that it would be better moved into the relevant caller(s).

Ian.