From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v3 32/32] libxl: allow the creation of HVM domains without a device model. Date: Wed, 29 Jul 2015 15:50:51 +0100 Message-ID: <20150729145051.GF5111@zion.uk.xensource.com> References: <1435923310-9019-1-git-send-email-roger.pau@citrix.com> <1435923310-9019-33-git-send-email-roger.pau@citrix.com> <20150728112253.GN5111@zion.uk.xensource.com> <55B8E685.7060109@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZKShH-0005CG-Pf for xen-devel@lists.xenproject.org; Wed, 29 Jul 2015 14:51:07 +0000 Content-Disposition: inline In-Reply-To: <55B8E685.7060109@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Roger Pau =?iso-8859-1?Q?Monn=E9?= Cc: xen-devel@lists.xenproject.org, Ian Campbell , Wei Liu , Ian Jackson , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Wed, Jul 29, 2015 at 04:43:17PM +0200, Roger Pau Monn=E9 wrote: [...] > >> = > >> It is recommended to accept the default value for new guests. If > >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > >> index e9a2d26..a6690cf 100644 > >> --- a/tools/libxl/libxl.c > >> +++ b/tools/libxl/libxl.c > >> @@ -1590,11 +1590,10 @@ void libxl__destroy_domid(libxl__egc *egc, lib= xl__destroy_domid_state *dis) > >> = > >> switch (libxl__domain_type(gc, domid)) { > >> case LIBXL_DOMAIN_TYPE_HVM: > >> - if (!libxl_get_stubdom_id(CTX, domid)) > >> - dm_present =3D 1; > >> - else > >> + if (libxl_get_stubdom_id(CTX, domid)) { > >> dm_present =3D 0; > >> - break; > >> + break; > >> + } > > = > > There is a path that dm_present contains garbage. > = > Please take into account that I've removed the last break in > LIBXL_DOMAIN_TYPE_HVM, so now the LIBXL_DOMAIN_TYPE_HVM case expands > into the LIBXL_DOMAIN_TYPE_PV case. > = Oh yes. Could use /* fall through */ to explicitly state this behaviour to avoid confusion and make Coverity happy. > >> case LIBXL_DOMAIN_TYPE_PV: > >> pid =3D libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/loc= al/domain/%d/image/device-model-pid", domid)); > >> dm_present =3D (pid !=3D NULL); > = > dm_present is set here for both PV and HVM guests without a stubdom. > = > >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > >> index f366a09..1444e40 100644 > >> --- a/tools/libxl/libxl_create.c > >> +++ b/tools/libxl/libxl_create.c > >> @@ -164,6 +164,8 @@ int libxl__domain_build_info_setdefault(libxl__gc = *gc, > >> b_info->u.hvm.bios =3D LIBXL_BIOS_TYPE_ROMBIOS; break; > >> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > >> b_info->u.hvm.bios =3D LIBXL_BIOS_TYPE_SEABIOS; break; > >> + case LIBXL_DEVICE_MODEL_VERSION_NONE: > >> + break; > >> default:return ERROR_INVAL; > >> } > >> = > >> @@ -177,6 +179,8 @@ int libxl__domain_build_info_setdefault(libxl__gc = *gc, > >> if (b_info->u.hvm.bios =3D=3D LIBXL_BIOS_TYPE_ROMBIOS) > >> return ERROR_INVAL; > >> break; > >> + case LIBXL_DEVICE_MODEL_VERSION_NONE: > >> + break; > >> default:abort(); > >> } > >> = > >> @@ -278,6 +282,9 @@ int libxl__domain_build_info_setdefault(libxl__gc = *gc, > >> break; > >> } > >> break; > >> + case LIBXL_DEVICE_MODEL_VERSION_NONE: > >> + b_info->video_memkb =3D 0; > >> + break; > >> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > >> default: > >> switch (b_info->u.hvm.vga.kind) { > >> @@ -945,6 +952,11 @@ static void initiate_domain_create(libxl__egc *eg= c, > >> ret =3D libxl__device_nic_setdefault(gc, &d_config->nics[i], = domid); > >> if (ret) goto error_out; > >> = > >> + /* HVM guests without a device model only have PV nics. */ > >> + if (d_config->b_info.device_model_version =3D=3D > >> + LIBXL_DEVICE_MODEL_VERSION_NONE) > >> + d_config->nics[i].nictype =3D LIBXL_NIC_TYPE_VIF; > >> + > > = > > Better to check if users have asked for emulated NIC and bail. > = > IMHO, I think it would be better to move this to > libxl__device_nic_setdefault and I will do so in the next version. Yes, that's better. Wei.