From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH 1/3] libxl:refactor stdvga option support v2
Date: Thu, 28 Jun 2012 06:33:21 +0100
Message-ID: <1340861601.5210.6.camel@dagon.hellion.org.uk>
References: <CAAh7U5M=5F=jJQPskTF_C2Dzmo1A2rbgvhSgCoMeDJnm7J=Q-g@mail.gmail.com>
	<1338983231.32319.65.camel@zakaz.uk.xensource.com>
	<CAAh7U5MPenxZH_bj3h2A+Y=Vko6KMK-nncRfm5nLkifM8jC1DA@mail.gmail.com>
	<1340217533.4998.3.camel@dagon.hellion.org.uk>
	<CAAh7U5NqDOjffUgSxyuYeR6hH=SAYobB=1e91MZc6MEnoSB2xw@mail.gmail.com>
	<1340810891.29172.50.camel@zakaz.uk.xensource.com>
	<CAAh7U5MXH+M-wgCW4dHUsSFsTux_wA=QARR_uZugHyL9rs3ejA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAAh7U5MXH+M-wgCW4dHUsSFsTux_wA=QARR_uZugHyL9rs3ejA@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: ZhouPeng <zpengxen@gmail.com>
Cc: "Xen-Devel (E-mail)" <xen-devel@lists.xensource.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Thu, 2012-06-28 at 02:42 +0100, ZhouPeng wrote:
> On Wed, Jun 27, 2012 at 11:28 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> diff -r 592d15bd4d5e tools/libxl/xl_cmdimpl.c
> >> --- a/tools/libxl/xl_cmdimpl.c  Fri May 18 16:19:21 2012 +0100
> >> +++ b/tools/libxl/xl_cmdimpl.c  Wed Jun 27 20:06:39 2012 +0800
> >> @@ -1256,7 +1256,10 @@ skip_vfb:
> >>  #undef parse_extra_args
> >>
> >>      if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> >> -        xlu_cfg_get_defbool(config, "stdvga", &b_info->u.hvm.stdvga, 0);
> >> +        if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
> >> +            if (l)
> >> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
> >
> > I think this needs to be:
> >          if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
> >                b_info->u.hvm.vga.kind = l ? \
> >                                         LIBXL_VGA_INTERFACE_TYPE_STD : \
> >                                         LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> >
> > so that both "stdvga=0" and "stdvga=1" result in what the user asked for
> > without relying on the libxl default remaining CIRRUS.
> 
> IMHO, this make simple to be complex.
> 
> I think the check  and set-default later in libxl_create.c as a whole is enough.

This is in libxl, as I said above xl should not rely on the default
remaining CIRRUS.

If the user says "stdvga=0" then xl needs to explicitly ask for CIRRUS,
despite the fact that it currently happens to be the default.

If we make the libxl default BLARGLE in the future then stdvga=0 still
needs to mean CIRRUS.

> +        if (!b_info->u.hvm.vga.kind)
> +            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> 
> 'as a whole' here, I means if more vga type added in the future, we
> don't need to set the default one by one, redundantly.