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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


> @@ -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.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel