Re: [Qemu-devel] [PATCH QEMU-XEN v4 9/9] xen: make it possible to build without the Xen PV domain builder

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-10-23 at 12:35 +0100, Stefano Stabellini wrote:
> On Fri, 23 Oct 2015, Ian Campbell wrote:
> > On Fri, 2015-10-23 at 12:12 +0100, Stefano Stabellini wrote:
> > > @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then
> > > >    fi
> > > >  fi
> > > >  
> > > > +if test "$xen_pv_domain_build" != "no"; then
> > > > +  if test "$xen_pv_domain_build" = "yes" &&
> > > > +     test "$xen" != "yes"; then
> > > > +      error_exit "User requested Xen PV domain builder support" \
> > > > +		 "which requires Xen support."
> > > > +  fi
> > > > +  xen_pv_domain_build=no
> > > > +fi
> > > 
> > > Can we simplify this to:
> > > 
> > >   if test "$xen_pv_domain_build" = "yes" &&
> > >        test "$xen" != "yes"; then
> > >         error_exit "User requested Xen PV domain builder support" \
> > >   		 "which requires Xen support."
> > >     fi
> > >   fi
> > > 
> > > and move xen_pv_domain_build=no at the beginning of the file?
> > 
> > I think so, I hadn't noticed the precedent for doing so further up in
> > the
> > file.
> > 
> > Just to check, is this still your preference after my earlier reply-to
> > -self
> > explaining why the code above is utter rubbish and proposing a
> > different
> > simplified version?
> 
> The simplified check is OK, but I would still prefer if you moved
> xen_pv_domain_build=no at the beginning of the file.

Ack, I'll do that then.
> 
> 
> > @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine)
> > > >      case XEN_ATTACH:
> > > >          /* nothing to do, xend handles everything */
> > > >          break;
> > > > -    case XEN_CREATE:
> > > > +    case XEN_CREATE: {
> > > > +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD
> > > > +        const char *kernel_filename = machine->kernel_filename;
> > > > +        const char *kernel_cmdline = machine->kernel_cmdline;
> > > > +        const char *initrd_filename = machine->initrd_filename;
> > > >          if (xen_domain_build_pv(kernel_filename, initrd_filename,
> > > >                                  kernel_cmdline) < 0) {
> > > >              fprintf(stderr, "xen pv domain creation failed\n");
> > > >              exit(1);
> > > >          }
> > > > +#else
> > > > +        fprintf(stderr, "xen pv domain creation not supported\n");
> > > > +        exit(1);
> > > > +#endif
> > > >          break;
> > > > +    }
> > > >      case XEN_EMULATE:
> > > >          fprintf(stderr, "xen emulation not implemented (yet)\n");
> > > >          exit(1);
> > > 
> > > Please add a default case with an error and place the XEN_CREATE
> > > entirely within the ifdef CONFIG_XEN_PV_DOMAIN_BUILD.
> > 
> > Will do.
> >