Re: [Xen-devel] [PATCH v3 32/32] libxl: allow the creation of HVM domains without a device model.

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jul 29, 2015 at 04:43:17PM +0200, Roger Pau Monné 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, 
> >> libxl__destroy_domid_state *dis)
> >>  
> >>      switch (libxl__domain_type(gc, domid)) {
> >>      case LIBXL_DOMAIN_TYPE_HVM:
> >> -        if (!libxl_get_stubdom_id(CTX, domid))
> >> -            dm_present = 1;
> >> -        else
> >> +        if (libxl_get_stubdom_id(CTX, domid)) {
> >>              dm_present = 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 = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, 
> >> "/local/domain/%d/image/device-model-pid", domid));
> >>          dm_present = (pid != 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 = LIBXL_BIOS_TYPE_ROMBIOS; break;
> >>              case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >>                  b_info->u.hvm.bios = 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 == 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 = 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 *egc,
> >>          ret = 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 ==
> >> +            LIBXL_DEVICE_MODEL_VERSION_NONE)
> >> +            d_config->nics[i].nictype = 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.

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