From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38954)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <prvs=7311588c8=Ian.Campbell@citrix.com>)
	id 1ZpaOB-00073Z-PT
	for qemu-devel@nongnu.org; Fri, 23 Oct 2015 07:20:07 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <prvs=7311588c8=Ian.Campbell@citrix.com>)
	id 1ZpaO7-0007L7-IG
	for qemu-devel@nongnu.org; Fri, 23 Oct 2015 07:20:03 -0400
Received: from smtp.citrix.com ([66.165.176.63]:17158 helo=SMTP02.CITRIX.COM)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <prvs=7311588c8=Ian.Campbell@citrix.com>)
	id 1ZpaO7-0007Kl-DE
	for qemu-devel@nongnu.org; Fri, 23 Oct 2015 07:19:59 -0400
Message-ID: <1445599197.2374.127.camel@citrix.com>
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 23 Oct 2015 12:19:57 +0100
In-Reply-To: <alpine.DEB.2.02.1510231209070.15801@kaball.uk.xensource.com>
References: <1445440941.9563.163.camel@citrix.com>
	<1445441038-25903-1-git-send-email-ian.campbell@citrix.com>
	<1445441038-25903-10-git-send-email-ian.campbell@citrix.com>
	<alpine.DEB.2.02.1510231209070.15801@kaball.uk.xensource.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH QEMU-XEN v4 9/9] xen: make it possible to
 build without the Xen PV domain builder
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org

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?

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