Re: [libvirt] [PATCH 1/6] extend usb controller model to support xen pvusb

[Chun Yan Liu]

>>> On 6/14/2016 at 11:29 PM, in message
<75f3d94f-19d3-b466-781f-abb283063...@laine.org>, Laine Stump 
wrote: 
> On 06/14/2016 01:02 AM, Jim Fehlig wrote: 
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c 
> >> >index 490260f..5c3de83 100644 
> >> >--- a/src/qemu/qemu_command.c 
> >> >+++ b/src/qemu/qemu_command.c 
> >> >@@ -133,6 +133,10 @@ VIR_ENUM_IMPL(qemuControllerModelUSB,  
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, 
> >> >"vt82c686b-usb-uhci", 
> >> >"pci-ohci", 
> >> >"nec-usb-xhci", 
> >> >+  "pvusb1", 
> >> >+  "pvusb2", 
> >> >+  "qusb1", 
> >> >+  "qusb2", 
> >> >"none"); 
> > It seems odd that these need to be added to qemu_command.c. But sadly, I'm  
> not 
> > familiar with how USB controllers are handled in the qemu driver to give  
> much 
> > useful feedback at this time. I can certainly start investigating that, but 
> >  
> in 
> > the meantime I've added Laine to the cc list. He has done quite a bit of  
> work in 
> > this area in the past and might have a few minutes free to comment on these 
> >  
> changes. 
>  
>  
> This enum exists because the model strings recognized/stored by  
> libvirt's XML parser are slightly different from the exact name of the  
> devices on the qemu commandline. For example, if you put "ehci" in the  
> XML, the device that qemu uses is called "usb-ehci". Internally (in the  
> domain object) it is stored as an enum value, and when we create the  
> qemu commandline we use qemuControllerModelUSBTypeToString(model) to get  
> the name of the device. 
>  
>  
> This points out a couple of things: 
>  
>  
> 1) the VIR_ENUM_IMPL for qemuControllerModelUSB and  
> virDomainontrollerModelUSB must be kept in sync (this is partly enforced  
> by the VIR_ENUM_IMPL() macro, which checks that the array of strings has  
> as many entries as VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST). 

Yes, that's why we have to update qemuControllerModelUSB too.

>  
>  
> 2) If new models are added that aren't supported in a particular  
> hypervisor, the post-parse validation for that hypervisor should check  
> for those models and flag them as errors. 

Right. I'll add a check in qemu post-parse.

Thanks,
Chunyan

>  
>  
>  
>  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/6] extend usb controller model to support xen pvusb

[Chun Yan Liu]

>>> On 6/14/2016 at 01:27 PM, in message <575f95c4.4080...@suse.com>, Juergen 
>>> Gross
 wrote: 
> On 14/06/16 07:02, Jim Fehlig wrote: 
> > On 06/12/2016 10:53 AM, Chunyan Liu wrote: 
> >> According to libxl implementation, it supports pvusb 
> >> controller of version 1.1 and version 2.0, and it 
> >> supports two types of backend, 'pvusb' (dom0 backend) 
> >> and 'qusb' (qemu backend). 
> >  
> > IIUC, the pvusb backend has not gained any traction in the kernel and will  
> not 
> > be accepted upstream. Adding Juergen to confirm. If that is the case, I  
> think it 
> > should be dropped. 
>  
> Correct. Upstream Linux doesn't have a pvusb backend and it seems 
> unlikely it will have one.

OK. If that will never happen, I will drop pvusb1 and pvusb2 models. It's easy.

- Chunyan 
>  
> You are talking about dropping it from libvirt, right? 
>  
> >  
> >> 
> >> To match libxl support, extend usb controller schema 
> >> to support more models: pvusb1 (pvusb, version 1.1), 
> >> pvusb2 (pvusb, version 2.0), qusb1 (qusb, version 1.1) 
> >> and 'qusb2' (qusb version 2.0). 
> >> 
> >> Signed-off-by: Chunyan Liu  
> >> --- 
> >>  docs/formatdomain.html.in | 6 +- 
> >>  docs/schemas/domaincommon.rng | 4  
> >>  src/conf/domain_conf.c| 4  
> >>  src/conf/domain_conf.h| 4  
> >>  src/qemu/qemu_command.c   | 4  
> >>  5 files changed, 21 insertions(+), 1 deletion(-) 
> >> 
> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in 
> >> index fb3ec5e..1492915 100644 
> >> --- a/docs/formatdomain.html.in 
> >> +++ b/docs/formatdomain.html.in 
> >> @@ -3063,7 +3063,11 @@ 
> >>  A usb controller has an optional attribute 
> >>  model, which is one of "piix3-uhci", "piix4-uhci", 
> >>  "ehci", "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", 
> >> -"vt82c686b-uhci", "pci-ohci" or "nec-xhci".  Additionally, 
> >> +"vt82c686b-uhci", "pci-ohci", "nec-xhci", "pvusb1" (xen pvusb 
> >> +with dom0 backend, version 1.1), "pvusb2" (xen pvusb with dom0 
> >> +backend, version 2.0), "qusb1" (xen pvusb with qemu backend, 
> >> +version 1.1) or "qusb2" (xen pvusb with qemu backend, version  
> 2.0). 
> >> +Additionally, 
> >>   since 0.10.0, if the USB bus needs to 
> >>   be explicitly disabled for the guest, model='none' 
> >>   may be used.  Since 1.0.5, no default 
> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng 
> >> index 02078d7..5afa15d 100644 
> >> --- a/docs/schemas/domaincommon.rng 
> >> +++ b/docs/schemas/domaincommon.rng 
> >> @@ -1768,6 +1768,10 @@ 
> >>pci-ohci 
> >>nec-xhci 
> >>none 
> >> +  pvusb1 
> >> +  pvusb2 
> >> +  qusb1 
> >> +  qusb2 
> >>   
> >> 
> >>   
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c 
> >> index 10e61da..ff3285f 100644 
> >> --- a/src/conf/domain_conf.c 
> >> +++ b/src/conf/domain_conf.c 
> >> @@ -356,6 +356,10 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,  
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, 
> >>"vt82c686b-uhci", 
> >>"pci-ohci", 
> >>"nec-xhci", 
> >> +  "pvusb1", 
> >> +  "pvusb2", 
> >> +  "qusb1", 
> >> +  "qusb2", 
> >>"none") 
> >>   
> >>  VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, 
> >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h 
> >> index 3792562..6733b2d 100644 
> >> --- a/src/conf/domain_conf.h 
> >> +++ b/src/conf/domain_conf.h 
> >> @@ -675,6 +675,10 @@ typedef enum { 
> >>  VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI, 
> >>  VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI, 
> >>  VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI, 
> >> +VIR_DOMAIN_CONTROLLER_MODEL_USB_PVUSB1, 
> >> +VIR_DOMAIN_CONTROLLER_MODEL_USB_PVUSB2, 
> >> +VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1, 
> >> +VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2, 
> >>  VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE, 
> >>   
> >>  VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST 
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c 
> >> index 490260f..5c3de83 100644 
> >> --- a/src/qemu/qemu_command.c 
> >> +++ b/src/qemu/qemu_command.c 
> >> @@ -133,6 +133,10 @@ VIR_ENUM_IMPL(qemuControllerModelUSB,  
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, 
> >>"vt82c686b-usb-uhci", 
> >>"pci-ohci", 
> >>"nec-usb-xhci", 
> >> +  "pvusb1", 
> >> +  "pvusb2", 
> >> +  "qusb1", 
> >> +  "qusb2", 
> >>"none"); 
> >  
> > It seems odd that these need to be added to qemu_command.c. But sadly, I'm  
> not 
> > familiar with how USB controllers are handled in the 

[libvirt] [RFC] extend USB controller 'model' to support xen pvusb

[Chun Yan Liu]

Hi, List,

Currently libxl supports pvusb type USB controller hotplug and in creation time,
I'd like to add the support in libvirt libxl driver too. According to current 
USB
controller schema, we need to extend 'model' to support xen PVUSB type. Since
libxl support USB controller of version 1.1 and version 2.0, we may add two
models: "pvusb1" (pvusb type, version 1.1) and "pvusb2" (pvusb type,
version 2.0).

Do you have any opinions on that? If it is agreed, I'll try to implement. 

Thanks,
Chunyan


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 2/4] libxl: support hotplug USB host device

[Chun Yan Liu]

>>> On 5/24/2016 at 03:23 AM, in message <57435894.9080...@suse.com>, Jim Fehlig
<jfeh...@suse.com> wrote: 

> On 05/23/2016 12:51 AM, Chun Yan Liu wrote: 
> > 
> > Yes, I think it's OK. And another place needs to be updated, since now it's 
> > not only pci device, but also could be usb device, so the error message 
> > should be updated. 
> >  if (virDomainHostdevFind(vmdef, hostdev, ) >= 0) { 
> > -pcisrc = >source.subsys.u.pci; 
> >  virReportError(VIR_ERR_OPERATION_FAILED, 
> > -   _("target pci device %.4x:%.2x:%.2x.%.1x\ 
> > -  already exists"), 
> > -   pcisrc->addr.domain, pcisrc->addr.bus, 
> > -   pcisrc->addr.slot, pcisrc->addr.function); 
> > +   _("device already exists in domain  
> configuration")); 
> >  return -1; 
> >  } 
>  
> Nice catch. I've squashed in the below diff, changing the error code and  
> message 
> to match the qemu driver. 

That's better. Thanks very much!

- Chunyan

>  
> Regards, 
> Jim 
>  
>  
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> index 5b6f87a..4d7124d 100644 
> --- a/src/libxl/libxl_driver.c 
> +++ b/src/libxl/libxl_driver.c 
> @@ -3293,7 +3293,6 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, 
> virDomainDeviceDefPtr dev) 
>  virDomainNetDefPtr net; 
>  virDomainHostdevDefPtr hostdev; 
>  virDomainHostdevDefPtr found; 
> -virDomainHostdevSubsysPCIPtr pcisrc; 
>  char mac[VIR_MAC_STRING_BUFLEN]; 
>   
>  switch (dev->type) { 
> @@ -3336,12 +3335,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, 
> virDomainDeviceDefPtr dev) 
>  } 
>   
>  if (virDomainHostdevFind(vmdef, hostdev, ) >= 0) { 
> -pcisrc = >source.subsys.u.pci; 
> -virReportError(VIR_ERR_OPERATION_FAILED, 
> -   _("target pci device %.4x:%.2x:%.2x.%.1x\ 
> -  already exists"), 
> -   pcisrc->addr.domain, pcisrc->addr.bus, 
> -   pcisrc->addr.slot, pcisrc->addr.function); 
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s", 
> +   _("device is already in the domain  
> configuration")); 
>  return -1; 
>  } 
>   
>  
>  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 2/4] libxl: support hotplug USB host device

[Chun Yan Liu]

>>> On 5/21/2016 at 05:25 AM, in message <573f80e3.4070...@suse.com>, Jim Fehlig
 wrote: 
> On 05/20/2016 02:25 PM, Jim Fehlig wrote: 
> > On 05/20/2016 04:32 AM, Joao Martins wrote: 
> >> On 05/19/2016 09:14 AM, Chunyan Liu wrote: 
> >>> Support hot attach/detach a USB host device to guest. 
> >>> Curretnly libxl only supports xen PV guest, and only 
> > s/Curretnly/Currently/ 
> > 
> >>> supports specifying USB host device by 'bus number' 
> >>> and 'device number'. 
> >>> 
> >>> For example: 
> >>>  usb.xml: 
> >>>  
> >>>
> >>>  
> >>>
> >>>  
> >>>  #xl attach-device dom usb.xml 
> >>>  #xl detach-device dom usb.xml 
> >>> 
> >>> Signed-off-by: Chunyan Liu  
> >>> --- 
> >>> Changes: 
> >>>   * add LIBXL_HAVE_PVUSB check 
> >>>   * fix Jim's comments 
> >>> 
> >>>  src/libxl/libxl_driver.c | 136  
> ++- 
> >>>  1 file changed, 135 insertions(+), 1 deletion(-) 
> >>> 
> >>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> >>> index 960673f..a171efe 100644 
> >>> --- a/src/libxl/libxl_driver.c 
> >>> +++ b/src/libxl/libxl_driver.c 
> >>> @@ -3028,6 +3028,56 @@ 
> >>> libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr  
> driver, 
> >>>  return ret; 
> >>>  } 
> >>>   
> >>> +#ifdef LIBXL_HAVE_PVUSB 
> >>> +static int 
> >>> +libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, 
> >>> +   virDomainObjPtr vm, 
> >>> +   virDomainHostdevDefPtr hostdev) 
> >>> +{ 
> >>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> >>> +libxl_device_usbdev usbdev; 
> >>> +virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> >>> +int ret = -1; 
> >>> + 
> >>> +libxl_device_usbdev_init(); 
> >>> + 
> >>> +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || 
> >>> +hostdev->source.subsys.type != 
> >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> >>> +goto cleanup; 
> >>> + 
> >>> +if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) 
> >>> +goto cleanup; 
> >>> + 
> >>> +if (virHostdevPrepareUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> >>> +vm->def->name, , 1, 0) < 0) 
> >>> +goto cleanup; 
> >>> + 
> >>> +if (libxlMakeUSB(hostdev, ) < 0) 
> >>> +goto reattach; 
> >>> + 
> >>> +if (libxl_device_usbdev_add(cfg->ctx, vm->def->id, , 0) < 0) 
> >>> { 
> >>> +virReportError(VIR_ERR_INTERNAL_ERROR, 
> >>> +   _("libxenlight failed to attach usb device  
> Busnum:%3x, Devnum:%3x"), 
> >>> +   hostdev->source.subsys.u.usb.bus, 
> >>> +   hostdev->source.subsys.u.usb.device); 
> >>> +goto reattach; 
> >>> +} 
> >>> + 
> >>> +vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; 
> >>> +ret = 0; 
> >>> +goto cleanup; 
> >>> + 
> >>> + reattach: 
> >>> +virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> >>> + vm->def->name, , 1); 
> >>> + 
> >>> + cleanup: 
> >>> +virObjectUnref(cfg); 
> >>> +libxl_device_usbdev_dispose(); 
> >>> +return ret; 
> >>> +} 
> >>> +#endif 
> >>> + 
> >>>  static int 
> >>>  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, 
> >>>  virDomainObjPtr vm, 
> >>> @@ -3046,6 +3096,13 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr  
> driver, 
> >>>  return -1; 
> >>>  break; 
> >>>   
> >>> +#ifdef LIBXL_HAVE_PVUSB 
> >>> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: 
> >>> +if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) 
> >>> +return -1; 
> >>> +break; 
> >>> +#endif 
> >>> + 
> >>>  default: 
> >>>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, 
> >>> _("hostdev subsys type '%s' not supported"), 
> >>> @@ -3271,7 +3328,9 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr 
> >>> vmdef,  
> virDomainDeviceDefPtr dev) 
> >>>  case VIR_DOMAIN_DEVICE_HOSTDEV: 
> >>>  hostdev = dev->data.hostdev; 
> >>>   
> >>> -if (hostdev->source.subsys.type !=  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> >>> +if (hostdev->source.subsys.type != 
> >>> +(VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI || 
> >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) 
> >> Is this conditional correct when LIBXL_HAVE_PVUSB isn't there? 
> > I think there are two problems here. First, (VIR_DOMAIN_DEVICE_DISK || 
> > VIR_DOMAIN_DEVICE_LEASE) evaluates to 1, so essentially this is equivalent  
> to 
> > 
> > if  (hostdev->source.subsys.type != 1) 

Oh, my god. Your are right. Stupid mistake. 

> > 
> > Second, I agree that we don't want to add a USB device to the config if  
> libxl 
> > cant support it. 
>  
> Something like the below diff? 

Yes, I think it's OK. And 

Re: [libvirt] [PATCH V2 2/4] libxl: support hotplug USB host device

[Chun Yan Liu]

>>> On 5/20/2016 at 06:32 PM, in message <573ee7c3.7000...@oracle.com>, Joao
Martins  wrote: 
> On 05/19/2016 09:14 AM, Chunyan Liu wrote: 
> > Support hot attach/detach a USB host device to guest. 
> > Curretnly libxl only supports xen PV guest, and only 
> > supports specifying USB host device by 'bus number' 
> > and 'device number'. 
> >  
> > For example: 
> >  usb.xml: 
> >  
> >
> >  
> >
> >  
> >  #xl attach-device dom usb.xml 
> >  #xl detach-device dom usb.xml 
> >  
> > Signed-off-by: Chunyan Liu  
> > --- 
> > Changes: 
> >   * add LIBXL_HAVE_PVUSB check 
> >   * fix Jim's comments 
> >  
> >  src/libxl/libxl_driver.c | 136  
> ++- 
> >  1 file changed, 135 insertions(+), 1 deletion(-) 
> >  
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 960673f..a171efe 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -3028,6 +3028,56 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >  return ret; 
> >  } 
> >   
> > +#ifdef LIBXL_HAVE_PVUSB 
> > +static int 
> > +libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +   virDomainObjPtr vm, 
> > +   virDomainHostdevDefPtr hostdev) 
> > +{ 
> > +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> > +libxl_device_usbdev usbdev; 
> > +virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> > +int ret = -1; 
> > + 
> > +libxl_device_usbdev_init(); 
> > + 
> > +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || 
> > +hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> > +goto cleanup; 
> > + 
> > +if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) 
> > +goto cleanup; 
> > + 
> > +if (virHostdevPrepareUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +vm->def->name, , 1, 0) < 0) 
> > +goto cleanup; 
> > + 
> > +if (libxlMakeUSB(hostdev, ) < 0) 
> > +goto reattach; 
> > + 
> > +if (libxl_device_usbdev_add(cfg->ctx, vm->def->id, , 0) < 0) { 
> > +virReportError(VIR_ERR_INTERNAL_ERROR, 
> > +   _("libxenlight failed to attach usb device  
> Busnum:%3x, Devnum:%3x"), 
> > +   hostdev->source.subsys.u.usb.bus, 
> > +   hostdev->source.subsys.u.usb.device); 
> > +goto reattach; 
> > +} 
> > + 
> > +vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; 
> > +ret = 0; 
> > +goto cleanup; 
> > + 
> > + reattach: 
> > +virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > + vm->def->name, , 1); 
> > + 
> > + cleanup: 
> > +virObjectUnref(cfg); 
> > +libxl_device_usbdev_dispose(); 
> > +return ret; 
> > +} 
> > +#endif 
> > + 
> >  static int 
> >  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, 
> >  virDomainObjPtr vm, 
> > @@ -3046,6 +3096,13 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr  
> driver, 
> >  return -1; 
> >  break; 
> >   
> > +#ifdef LIBXL_HAVE_PVUSB 
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: 
> > +if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) 
> > +return -1; 
> > +break; 
> > +#endif 
> > + 
> >  default: 
> >  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, 
> > _("hostdev subsys type '%s' not supported"), 
> > @@ -3271,7 +3328,9 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef,  
> virDomainDeviceDefPtr dev) 
> >  case VIR_DOMAIN_DEVICE_HOSTDEV: 
> >  hostdev = dev->data.hostdev; 
> >   
> > -if (hostdev->source.subsys.type !=  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +if (hostdev->source.subsys.type != 
> > +(VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI || 
> > + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) 
> Is this conditional correct when LIBXL_HAVE_PVUSB isn't there? 

No harm without LIBXL_HAVE_PVUSB, it can be handled. But if you want
to use it to reboot the domain, the real hotplug into the system will be failed
without LIBXL_HAVE_PVUSB. So, if our purpose is: any PVUSB function should
be forbidden without LIBXL_HAVE_PVUSB, we need to add a check; otherwise,
it's OK. How do you think?

- Chunyan

>  
> >  return -1; 
> >   
> >  if (virDomainHostdevFind(vmdef, hostdev, ) >= 0) { 
> > @@ -3389,6 +3448,76 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >  return ret; 
> >  } 
> >   
> > +#ifdef LIBXL_HAVE_PVUSB 
> > +static int 
> > +libxlDomainDetachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +   virDomainObjPtr vm, 
> > +   

Re: [libvirt] [PATCH] libxl: add .domainInterfaceAddresses

[Chun Yan Liu]

>>> On 5/17/2016 at 11:46 PM, in message
<2fa0cb6f-ea83-d6f3-18f8-51a671574...@laine.org>, Laine Stump <la...@laine.org>
wrote: 
> On 05/16/2016 06:05 PM, Jim Fehlig wrote: 
> > Chun Yan Liu wrote: 
> >>>>> On 5/14/2016 at 07:47 AM, in message <5736677d.8030...@suse.com>, Jim 
> >>>>> Fehlig 
> >> <jfeh...@suse.com> wrote: 
> >>> On 05/13/2016 12:21 AM, Chunyan Liu wrote: 
> >>>> Add .domainInterfaceAddresses so that user can have a way to 
> >>>> get domain interface address by 'virsh domifaddr'. Currently 
> >>>> it only supports '--source lease'. 
> >>>> 
> >>>> Signed-off: Chunyan Liu <cy...@suse.com> 
> >>>> --- 
> >>>>   src/libxl/libxl_driver.c | 140 
> >>> +++ 
> >>>>   1 file changed, 140 insertions(+) 
> >>>> 
> >>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> >>>> index 062d6f8..f2bd6fa 100644 
> >>>> --- a/src/libxl/libxl_driver.c 
> >>>> +++ b/src/libxl/libxl_driver.c 
> >>>> @@ -5425,6 +5425,145 @@ static int 
> >>>> libxlNodeGetSecurityModel(virConnectPtr 
> >>> conn, 
> >>>>   return 0; 
> >>>>   } 
> >>>> 
> >>>> +static int 
> >>>> +libxlGetDHCPInterfaces(virDomainPtr dom, 
> >>>> +   virDomainObjPtr vm, 
> >>>> +   virDomainInterfacePtr **ifaces) 
> >>>> +{ 
> >>>> +int rv = -1; 
> >>>> +int n_leases = 0; 
> >>>> +size_t i, j; 
> >>>> +size_t ifaces_count = 0; 
> >>>> +virNetworkPtr network = NULL; 
> >>>> +char macaddr[VIR_MAC_STRING_BUFLEN]; 
> >>>> +virDomainInterfacePtr iface = NULL; 
> >>>> +virNetworkDHCPLeasePtr *leases = NULL; 
> >>>> +virDomainInterfacePtr *ifaces_ret = NULL; 
> >>>> + 
> >>>> +if (!dom->conn->networkDriver || 
> >>>> +!dom->conn->networkDriver->networkGetDHCPLeases) { 
> >>>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 
> >>>> +   _("Network driver does not support DHCP lease 
> >>> query")); 
> >>>> +return -1; 
> >>>> +} 
> >>>> + 
> >>>> +for (i = 0; i < vm->def->nnets; i++) { 
> >>>> +if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) 
> >>>> +continue; 
> >>>> + 
> >>>> +virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); 
> >>>> +virObjectUnref(network); 
> >>>> +network = virNetworkLookupByName(dom->conn, 
> >>>> +  
> vm->def->nets[i]->data.network.name); 
> >>>> + 
> >>>> +if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, 
> >>>> +, 0)) < 0) 
> >>>> +goto error; 
> >>>> + 
> >>>> +if (n_leases) { 
> >>>> +if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) 
> >>>> +goto error; 
> >>>> + 
> >>>> +if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) 
> >>>> +goto error; 
> >>>> + 
> >>>> +iface = ifaces_ret[ifaces_count - 1]; 
> >>>> +/* Assuming each lease corresponds to a separate IP */ 
> >>>> +iface->naddrs = n_leases; 
> >>>> + 
> >>>> +if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) 
> >>>> +goto error; 
> >>>> + 
> >>>> +if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) 
> >>>> +goto cleanup; 
> >>>> + 
> >>>> +if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) 
> >>>> +goto cleanup; 
> >>>> +} 
> >>>> + 
> >>>> +for (j = 0; j < n_leases; j++) { 
> >>>> +virNetworkDHCPLeasePtr lea

Re: [libvirt] [PATCH 1/2] xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl

[Chun Yan Liu]

>>> On 5/16/2016 at 06:14 PM, in message <57399d91.1030...@oracle.com>, Joao
Martins  wrote: 

>  
> On 05/13/2016 05:54 PM, Jim Fehlig wrote: 
> > On 05/13/2016 06:59 AM, Joao Martins wrote: 
> >> 
> >> On 05/12/2016 09:55 PM, Jim Fehlig wrote: 
> >>> Joao Martins wrote: 
>  On 05/12/2016 12:54 AM, Jim Fehlig wrote: 
> > On 04/21/2016 05:10 AM, Chunyan Liu wrote: 
> >> According to current xl.cfg docs and xl codes, it uses type=vif 
> >> instead of type=netfront. 
> >> 
> >> Currently after domxml-to-native, libvirt xml model=netfront will be 
> >> converted to xl type=netfront. This has no problem before, xen codes 
> >> for a long time just check type=ioemu, if not, set type to _VIF. 
> >> 
> >> Since libxl uses parse_nic_config to avoid duplicate codes, it 
> >> compares 'type=vif' and 'type=ioemu' for valid parameters, others 
> >> are considered as invalid, thus we have problem with type=netfront 
> >> in xl config file. 
> >>  #xl create sles12gm-hvm.orig 
> >>  Parsing config from sles12gm-hvm.orig 
> >>  Invalid parameter `type'. 
> >> 
> >> Correct the convertion in libvirt, so that it matchs libxl codes 
> >> and also xl.cfg. 
> >> 
> >> Signed-off-by: Chunyan Liu  
> >> --- 
> >>  src/xenconfig/xen_common.c | 38 
> >> -- 
> >>  src/xenconfig/xen_common.h |  7 --- 
> >>  src/xenconfig/xen_xl.c |  4 ++-- 
> >>  src/xenconfig/xen_xm.c |  8  
> >>  4 files changed, 34 insertions(+), 23 deletions(-) 
> >> 
> >> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c 
> >> index e1d9cf6..f54d6b6 100644 
> >> --- a/src/xenconfig/xen_common.c 
> >> +++ b/src/xenconfig/xen_common.c 
> >> @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr 
> >> def) 
> >>  return -1; 
> >>  } 
> >>   
> >> - 
> >>  static int 
> >> -xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> >> +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char  
> *vif_typename) 
> >>  { 
> >>  char *script = NULL; 
> >>  virDomainNetDefPtr net = NULL; 
> >> @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> >>  VIR_STRDUP(net->model, model) < 0) 
> >>  goto cleanup; 
> >>   
> >> -if (!model[0] && type[0] && STREQ(type, "netfront") && 
> >> +if (!model[0] && type[0] && STREQ(type, vif_typename) && 
> >>  VIR_STRDUP(net->model, "netfront") < 0) 
> >>  goto cleanup; 
> >>   
> >> @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, 
> >> virDomainDefPtr  
> def, virCapsPtr caps) 
> >>   
> >>  /* 
> >>   * A convenience function for parsing all config common to both XM 
> >> and XL 
> >> + * 
> >> + * vif_typename: type name for a paravirtualized network could 
> >> + * be different for xm and xl. For xm, it uses type=netfront; 
> >> + * for xl, it uses type=vif. So, for xm, should pass "netfront"; 
> >> + * for xl, should pass "vif". 
> >>   */ 
> >>  int 
> >>  xenParseConfigCommon(virConfPtr conf, 
> >>   virDomainDefPtr def, 
> >> - virCapsPtr caps) 
> >> + virCapsPtr caps, 
> >> + const char *vif_typename) 
> > One thing I didn't recall when suggesting this approach is that  
> xenParseVif() is 
> > called in xenParseConfigCommon(). I was thinking it was called from 
> > xen_{xl,xm}.c and the extra parameter would only be added to the 
> > xen{Format,Parse}Vif functions. I don't particularly like seeing the 
> > device 
> > specific parameter added to the common functions, but wont object if 
> > others  
> are 
> > fine with it. Any other opinions on that? Joao? 
>  That's a good point - probably we can avoid it by using 
>  xen{Format,Parse}Vif (with the signature change Chunyan proposes)  
> individually 
>  on xenParseXM and xenParseXL. 
> >>> Nod. 
> >>> 
>  And there wouldn't be any xenParseConfigCommon 
>  with device-specific parameters (as vif being one of the many devices 
>  that  
> the 
>  routine is handling). The vif config is the same between xm and xl, with 
>   
> the 
>  small difference wrt to the validation on xen libxl side - so having in 
>  xen_common.c makes sense. 
> >>> Nod again :-). 
> >>> 
> > And one reason I wont object is that the alternative (calling 
> > xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since 
> > all  
> the 
> > tests/{xl,xm}configdata/ files would need to be adjusted. 
>  Hm, perhaps I fail to see what the large change would be. We would keep 
>  

Re: [libvirt] [PATCH 1/2] xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl

[Chun Yan Liu]

>>> On 5/14/2016 at 12:54 AM, in message <573606ab.4080...@suse.com>, Jim Fehlig
 wrote: 
> On 05/13/2016 06:59 AM, Joao Martins wrote: 
> > 
> > On 05/12/2016 09:55 PM, Jim Fehlig wrote: 
> >> Joao Martins wrote: 
> >>> On 05/12/2016 12:54 AM, Jim Fehlig wrote: 
>  On 04/21/2016 05:10 AM, Chunyan Liu wrote: 
> > According to current xl.cfg docs and xl codes, it uses type=vif 
> > instead of type=netfront. 
> > 
> > Currently after domxml-to-native, libvirt xml model=netfront will be 
> > converted to xl type=netfront. This has no problem before, xen codes 
> > for a long time just check type=ioemu, if not, set type to _VIF. 
> > 
> > Since libxl uses parse_nic_config to avoid duplicate codes, it 
> > compares 'type=vif' and 'type=ioemu' for valid parameters, others 
> > are considered as invalid, thus we have problem with type=netfront 
> > in xl config file. 
> >  #xl create sles12gm-hvm.orig 
> >  Parsing config from sles12gm-hvm.orig 
> >  Invalid parameter `type'. 
> > 
> > Correct the convertion in libvirt, so that it matchs libxl codes 
> > and also xl.cfg. 
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/xenconfig/xen_common.c | 38 -- 
> >  src/xenconfig/xen_common.h |  7 --- 
> >  src/xenconfig/xen_xl.c |  4 ++-- 
> >  src/xenconfig/xen_xm.c |  8  
> >  4 files changed, 34 insertions(+), 23 deletions(-) 
> > 
> > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c 
> > index e1d9cf6..f54d6b6 100644 
> > --- a/src/xenconfig/xen_common.c 
> > +++ b/src/xenconfig/xen_common.c 
> > @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr 
> > def) 
> >  return -1; 
> >  } 
> >   
> > - 
> >  static int 
> > -xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> > +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char  
> *vif_typename) 
> >  { 
> >  char *script = NULL; 
> >  virDomainNetDefPtr net = NULL; 
> > @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> >  VIR_STRDUP(net->model, model) < 0) 
> >  goto cleanup; 
> >   
> > -if (!model[0] && type[0] && STREQ(type, "netfront") && 
> > +if (!model[0] && type[0] && STREQ(type, vif_typename) && 
> >  VIR_STRDUP(net->model, "netfront") < 0) 
> >  goto cleanup; 
> >   
> > @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, 
> > virDomainDefPtr  
> def, virCapsPtr caps) 
> >   
> >  /* 
> >   * A convenience function for parsing all config common to both XM and 
> > XL 
> > + * 
> > + * vif_typename: type name for a paravirtualized network could 
> > + * be different for xm and xl. For xm, it uses type=netfront; 
> > + * for xl, it uses type=vif. So, for xm, should pass "netfront"; 
> > + * for xl, should pass "vif". 
> >   */ 
> >  int 
> >  xenParseConfigCommon(virConfPtr conf, 
> >   virDomainDefPtr def, 
> > - virCapsPtr caps) 
> > + virCapsPtr caps, 
> > + const char *vif_typename) 
>  One thing I didn't recall when suggesting this approach is that  
> xenParseVif() is 
>  called in xenParseConfigCommon(). I was thinking it was called from 
>  xen_{xl,xm}.c and the extra parameter would only be added to the 
>  xen{Format,Parse}Vif functions. I don't particularly like seeing the 
>  device 
>  specific parameter added to the common functions, but wont object if 
>  others  
> are 
>  fine with it. Any other opinions on that? Joao? 
> >>> That's a good point - probably we can avoid it by using 
> >>> xen{Format,Parse}Vif (with the signature change Chunyan proposes)  
> individually 
> >>> on xenParseXM and xenParseXL. 
> >> Nod. 
> >> 
> >>> And there wouldn't be any xenParseConfigCommon 
> >>> with device-specific parameters (as vif being one of the many devices 
> >>> that  
> the 
> >>> routine is handling). The vif config is the same between xm and xl, with  
> the 
> >>> small difference wrt to the validation on xen libxl side - so having in 
> >>> xen_common.c makes sense. 
> >> Nod again :-). 
> >> 
>  And one reason I wont object is that the alternative (calling 
>  xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since 
>  all  
> the 
>  tests/{xl,xm}configdata/ files would need to be adjusted. 
> >>> Hm, perhaps I fail to see what the large change would be. We would keep 
> >>> the  
> same 
> >>> interface (i.e. model=netfront as valid on libvirt-side and converting to 
> >>> type="vif" where applicable (libxl)) then the xml and .cfg won't change. 
> >>> 

Re: [libvirt] [PATCH] libxl: add .domainInterfaceAddresses

[Chun Yan Liu]

>>> On 5/14/2016 at 07:47 AM, in message <5736677d.8030...@suse.com>, Jim Fehlig
 wrote: 
> On 05/13/2016 12:21 AM, Chunyan Liu wrote: 
> > Add .domainInterfaceAddresses so that user can have a way to 
> > get domain interface address by 'virsh domifaddr'. Currently 
> > it only supports '--source lease'. 
> > 
> > Signed-off: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_driver.c | 140  
> +++ 
> >  1 file changed, 140 insertions(+) 
> > 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 062d6f8..f2bd6fa 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -5425,6 +5425,145 @@ static int libxlNodeGetSecurityModel(virConnectPtr  
> conn, 
> >  return 0; 
> >  } 
> >   
> > +static int 
> > +libxlGetDHCPInterfaces(virDomainPtr dom, 
> > +   virDomainObjPtr vm, 
> > +   virDomainInterfacePtr **ifaces) 
> > +{ 
> > +int rv = -1; 
> > +int n_leases = 0; 
> > +size_t i, j; 
> > +size_t ifaces_count = 0; 
> > +virNetworkPtr network = NULL; 
> > +char macaddr[VIR_MAC_STRING_BUFLEN]; 
> > +virDomainInterfacePtr iface = NULL; 
> > +virNetworkDHCPLeasePtr *leases = NULL; 
> > +virDomainInterfacePtr *ifaces_ret = NULL; 
> > + 
> > +if (!dom->conn->networkDriver || 
> > +!dom->conn->networkDriver->networkGetDHCPLeases) { 
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 
> > +   _("Network driver does not support DHCP lease  
> query")); 
> > +return -1; 
> > +} 
> > + 
> > +for (i = 0; i < vm->def->nnets; i++) { 
> > +if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) 
> > +continue; 
> > + 
> > +virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); 
> > +virObjectUnref(network); 
> > +network = virNetworkLookupByName(dom->conn, 
> > + 
> > vm->def->nets[i]->data.network.name); 
> > + 
> > +if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, 
> > +, 0)) < 0) 
> > +goto error; 
> > + 
> > +if (n_leases) { 
> > +if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) 
> > +goto error; 
> > + 
> > +if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) 
> > +goto error; 
> > + 
> > +iface = ifaces_ret[ifaces_count - 1]; 
> > +/* Assuming each lease corresponds to a separate IP */ 
> > +iface->naddrs = n_leases; 
> > + 
> > +if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) 
> > +goto error; 
> > + 
> > +if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) 
> > +goto cleanup; 
> > + 
> > +if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) 
> > +goto cleanup; 
> > +} 
> > + 
> > +for (j = 0; j < n_leases; j++) { 
> > +virNetworkDHCPLeasePtr lease = leases[j]; 
> > +virDomainIPAddressPtr ip_addr = >addrs[j]; 
> > + 
> > +if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0) 
> > +goto cleanup; 
> > + 
> > +ip_addr->type = lease->type; 
> > +ip_addr->prefix = lease->prefix; 
> > +} 
> > + 
> > +for (j = 0; j < n_leases; j++) 
> > +virNetworkDHCPLeaseFree(leases[j]); 
> > + 
> > +VIR_FREE(leases); 
> > +} 
> > + 
> > +*ifaces = ifaces_ret; 
> > +ifaces_ret = NULL; 
> > +rv = ifaces_count; 
> > + 
> > + cleanup: 
> > +virObjectUnref(network); 
> > +if (leases) { 
> > +for (i = 0; i < n_leases; i++) 
> > +virNetworkDHCPLeaseFree(leases[i]); 
> > +} 
> > +VIR_FREE(leases); 
> > + 
> > +return rv; 
> > + 
> > + error: 
> > +if (ifaces_ret) { 
> > +for (i = 0; i < ifaces_count; i++) 
> > +virDomainInterfaceFree(ifaces_ret[i]); 
> > +} 
> > +VIR_FREE(ifaces_ret); 
> > + 
> > +goto cleanup; 
> > +} 
>  
> It's unfortunate this is a copy-paste from the qemu driver. The code is not 
> trivial and any bug fixes in one copy could be missed in the other. A lot of  
> the 
> function is domain related, so probably can't be abstracted further to the 
> network code. Have you considered approaches that allow the drivers to share 
> this code?

Well, it can be extracted and placed in bridge_driver.c as 
networkGetDHCPInterfaces,
but I don't know if that is acceptable?

Chunyan

>  
> Regards, 
> Jim 
>  
>  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl

[Chun Yan Liu]

>>> On 5/14/2016 at 12:58 AM, in message <5736079b.5050...@suse.com>, Jim Fehlig
 wrote: 
> On 05/13/2016 07:08 AM, Joao Martins wrote: 
> > I am not sure what's the etiquette in these cases but I sent you some  
> patches 
> > that fixes the tests and makes some adjustments to help out (all  
> changelog-ed). 
> > There were a couple of failures lately regarding vram defaults and what not 
> >  
> so 
> > some of the tests would fail as vram defaults would be wrongly calculated.  
> Jim 
> > sent a series fixing that which is Acked already but still to be pushed. 
>  
> I've pushed those patches now. I think we've finally resolved all the  
> default 
> vram issues. 

Joao, I applied your updated patch series, it looks good. Maybe you can send
out to mailing list?

Chunyan

>  
> Regards, 
> Jim 
>  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] libxl: support hotplug USB host device

[Chun Yan Liu]

>>> On 5/13/2016 at 07:20 AM, in message <57350fa5.3070...@suse.com>, Jim Fehlig
 wrote: 
> Chunyan Liu wrote: 
> > Support hot attach/detach a USB host device to guest. 
> > Curretnly libxl only supports xen PV guest, and only 
>  
> s/Curretnly/Currently/ 
>  
> > supports specifying USB host device by 'bus number' 
> > and 'device number'. 
> >  
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_driver.c | 130  
> ++- 
> >  1 file changed, 129 insertions(+), 1 deletion(-) 
> >  
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 18a0891..8900644 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -3024,6 +3024,55 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >  } 
> >   
> >  static int 
> > +libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +   virDomainObjPtr vm, 
> > +   virDomainHostdevDefPtr hostdev) 
> > +{ 
> > +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> > +libxl_device_usbdev usbdev; 
> > +virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> > +int ret = -1; 
> > + 
> > +libxl_device_usbdev_init(); 
>  
> Move the init below the check for hostdev->mode and 
> hostdev->source.subsys.type, 
> otherwise it is not disposed if the check fails. 

Yes, will update.

>  
> > + 
> > +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || 
> > +hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> > +return ret; 
> > + 
> > +if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) 
> > +goto cleanup; 
> > + 
> > +if (virHostdevPrepareUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +vm->def->name, , 1, 0) < 0) 
> > +goto cleanup; 
> > + 
> > +if (libxlMakeUSB(hostdev, ) < 0) 
> > +goto error; 
> > + 
> > +if (libxl_device_usbdev_add(cfg->ctx, vm->def->id, , 0) < 0) { 
> > +virReportError(VIR_ERR_INTERNAL_ERROR, 
> > +   _("libxenlight failed to attach usb device  
> Busnum:%3x, Devnum:%3x"), 
> > +   hostdev->source.subsys.u.usb.bus, 
> > +   hostdev->source.subsys.u.usb.device); 
> > +goto error; 
> > +} 
> > + 
> > +vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; 
> > +ret = 0; 
> > +goto cleanup; 
> > + 
> > + error: 
>  
> I think 'reattach' is a better name for this label. 

Will update.

>  
> > +virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > + vm->def->name, , 1); 
> > + 
> > + cleanup: 
> > +virObjectUnref(cfg); 
> > +libxl_device_usbdev_dispose(); 
> > +return ret; 
> > +} 
> > + 
> > + 
> > +static int 
> >  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, 
> >  virDomainObjPtr vm, 
> >  virDomainHostdevDefPtr hostdev) 
> > @@ -3041,6 +3090,11 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr  
> driver, 
> >  return -1; 
> >  break; 
> >   
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: 
> > +if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) 
> > +return -1; 
> > +break; 
> > + 
> >  default: 
> >  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, 
> > _("hostdev subsys type '%s' not supported"), 
> > @@ -3266,7 +3320,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef,  
> virDomainDeviceDefPtr dev) 
> >  case VIR_DOMAIN_DEVICE_HOSTDEV: 
> >  hostdev = dev->data.hostdev; 
> >   
> > -if (hostdev->source.subsys.type !=  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +if (hostdev->source.subsys.type != 
> > +(VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB ||  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)) 
>  
> if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB 
>  
> || 
> hostdev->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
>  
> >  return -1; 
> >   
> >  if (virDomainHostdevFind(vmdef, hostdev, ) >= 0) { 
> > @@ -3385,6 +3440,76 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >  } 
> >   
> >  static int 
> > +libxlDomainDetachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +   virDomainObjPtr vm, 
> > +   virDomainHostdevDefPtr hostdev) 
> > +{ 
> > +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> > +virDomainHostdevSubsysPtr subsys = >source.subsys; 
> > +virDomainHostdevSubsysUSBPtr usbsrc = >u.usb; 
> > +virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> > +libxl_device_usbdev usbdev; 
> > +

Re: [libvirt] [PATCH 1/2] libxl: support creating guest with USB hostdev

[Chun Yan Liu]

>>> On 5/13/2016 at 06:56 AM, in message <57350a36.3070...@suse.com>, Jim Fehlig
 wrote: 
> Chunyan Liu wrote: 
> > Support creating guest with USB host device in config file. 
> > Currently libxl only supports xen PV guest, and only supports 
> > specifying USB host device by 'bus number' and 'device number'. 
>  
> It would be nice to have an example of xl.cfg(5) usbdev= configuration and  
> the 
> corresponding domXML snippet. Actually, the example would be better placed  
> in 
> the commit message for the new patch needed for converting domXML <-> 
> xl.cfg(5) 
> USB config. 
>  
> >  
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_conf.c   | 71  
>  
> >  src/libxl/libxl_conf.h   |  3 ++ 
> >  src/libxl/libxl_domain.c |  4 +-- 
> >  src/libxl/libxl_driver.c |  2 +- 
> >  4 files changed, 77 insertions(+), 3 deletions(-) 
> >  
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c 
> > index 30f2ce9..3b69cbf 100644 
> > --- a/src/libxl/libxl_conf.c 
> > +++ b/src/libxl/libxl_conf.c 
> > @@ -1848,6 +1848,74 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr  
> cfg, 
> >  } 
> >   
> >  int 
> > +libxlMakeUSB(virDomainHostdevDefPtr hostdev, libxl_device_usbdev *usbdev) 
> > +{ 
> > +virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb; 
> > + 
> > +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) 
> > +return -1; 
> > +if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> > +return -1; 
> > + 
> > +if (usbsrc->bus <= 0 || usbsrc->device <= 0) { 
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", 
> > +   _("libxenlight supports only USB device " 
> > + "specified by busnum:devnum")); 
> > +return -1; 
> > +} 
> > + 
> > +usbdev->u.hostdev.hostbus = usbsrc->bus; 
> > +usbdev->u.hostdev.hostaddr = usbsrc->device; 
> > + 
> > +return 0; 
> > +} 
> > + 
> > +static int 
> > +libxlMakeUSBList(virDomainDefPtr def, libxl_domain_config *d_config) 
> > +{ 
> > +virDomainHostdevDefPtr *l_hostdevs = def->hostdevs; 
> > +size_t nhostdevs = def->nhostdevs; 
> > +size_t nusbdevs = 0; 
> > +libxl_device_usbdev *x_usbdevs; 
> > +size_t i, j; 
> > + 
> > +if (nhostdevs == 0) 
> > +return 0; 
> > + 
> > +if (VIR_ALLOC_N(x_usbdevs, nhostdevs) < 0) 
> > +return -1; 
> > + 
> > +for (i = 0, j = 0; i < nhostdevs; i++) { 
> > +if (l_hostdevs[i]->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) 
> > +continue; 
> > +if (l_hostdevs[i]->source.subsys.type !=  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> > +continue; 
> > + 
> > +libxl_device_usbdev_init(_usbdevs[j]); 
> > + 
> > +if (libxlMakeUSB(l_hostdevs[i], _usbdevs[j]) < 0) 
> > +goto error; 
> > + 
> > +nusbdevs++; 
> > +j++; 
> > +} 
> > + 
> > +VIR_SHRINK_N(x_usbdevs, nhostdevs, nhostdevs - nusbdevs); 
> > +d_config->usbdevs = x_usbdevs; 
> > +d_config->num_usbdevs = nusbdevs; 
> > + 
> > +return 0; 
> > + 
> > + error: 
> > +for (i = 0; i < nhostdevs; i++) 
> > +libxl_device_usbdev_dispose(_usbdevs[i]); 
>  
> It looks like it is possible to call dispose on elements that did not have a 
> corresponding init. 

Oh, my fault, should be "i   
> > + 
> > +VIR_FREE(x_usbdevs); 
> > +return -1; 
> > +} 
> > + 
> > + 
> > +int 
> >  libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev) 
> >  { 
> >  virDomainHostdevSubsysPCIPtr pcisrc = >source.subsys.u.pci; 
> > @@ -2078,6 +2146,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr  
> graphicsports, 
> >  if (libxlMakePCIList(def, d_config) < 0) 
> >  return -1; 
> >   
> > +if (libxlMakeUSBList(def, d_config) < 0) 
> > +return -1; 
> > + 
> >  /* 
> >   * Now that any potential VFBs are defined, update the build info with 
> >   * the data of the primary display. Some day libxl might implicitely  
> do 
> > diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h 
> > index 24e2911..e3ccca1 100644 
> > --- a/src/libxl/libxl_conf.h 
> > +++ b/src/libxl/libxl_conf.h 
> > @@ -192,6 +192,9 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, 
> >  int 
> >  libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev); 
> >   
> > +int 
> > +libxlMakeUSB(virDomainHostdevDefPtr hostdev, libxl_device_usbdev *usbdev); 
> > + 
> >  virDomainXMLOptionPtr 
> >  libxlCreateXMLConf(void); 
> >   
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index 14a900c..4272dda 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -728,7 +728,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, 
> >  virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> >   
> >  

Re: [libvirt] [PATCH 1/2] xenFormatNet: correct `type=netfront' to 'type=vif' to match libxl

[Chun Yan Liu]

>>> On 5/12/2016 at 11:00 PM, in message <57349a91.50...@oracle.com>, Joao 
>>> Martins
 wrote: 
> On 05/12/2016 12:54 AM, Jim Fehlig wrote: 
> > On 04/21/2016 05:10 AM, Chunyan Liu wrote: 
> >> According to current xl.cfg docs and xl codes, it uses type=vif 
> >> instead of type=netfront. 
> >> 
> >> Currently after domxml-to-native, libvirt xml model=netfront will be 
> >> converted to xl type=netfront. This has no problem before, xen codes 
> >> for a long time just check type=ioemu, if not, set type to _VIF. 
> >> 
> >> Since libxl uses parse_nic_config to avoid duplicate codes, it 
> >> compares 'type=vif' and 'type=ioemu' for valid parameters, others 
> >> are considered as invalid, thus we have problem with type=netfront 
> >> in xl config file. 
> >>  #xl create sles12gm-hvm.orig 
> >>  Parsing config from sles12gm-hvm.orig 
> >>  Invalid parameter `type'. 
> >> 
> >> Correct the convertion in libvirt, so that it matchs libxl codes 
> >> and also xl.cfg. 
> >> 
> >> Signed-off-by: Chunyan Liu  
> >> --- 
> >>  src/xenconfig/xen_common.c | 38 -- 
> >>  src/xenconfig/xen_common.h |  7 --- 
> >>  src/xenconfig/xen_xl.c |  4 ++-- 
> >>  src/xenconfig/xen_xm.c |  8  
> >>  4 files changed, 34 insertions(+), 23 deletions(-) 
> >> 
> >> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c 
> >> index e1d9cf6..f54d6b6 100644 
> >> --- a/src/xenconfig/xen_common.c 
> >> +++ b/src/xenconfig/xen_common.c 
> >> @@ -801,9 +801,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def) 
> >>  return -1; 
> >>  } 
> >>   
> >> - 
> >>  static int 
> >> -xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> >> +xenParseVif(virConfPtr conf, virDomainDefPtr def, const char  
> *vif_typename) 
> >>  { 
> >>  char *script = NULL; 
> >>  virDomainNetDefPtr net = NULL; 
> >> @@ -942,7 +941,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) 
> >>  VIR_STRDUP(net->model, model) < 0) 
> >>  goto cleanup; 
> >>   
> >> -if (!model[0] && type[0] && STREQ(type, "netfront") && 
> >> +if (!model[0] && type[0] && STREQ(type, vif_typename) && 
> >>  VIR_STRDUP(net->model, "netfront") < 0) 
> >>  goto cleanup; 
> >>   
> >> @@ -1042,11 +1041,17 @@ xenParseGeneralMeta(virConfPtr conf, 
> >> virDomainDefPtr  
> def, virCapsPtr caps) 
> >>   
> >>  /* 
> >>   * A convenience function for parsing all config common to both XM and XL 
> >> + * 
> >> + * vif_typename: type name for a paravirtualized network could 
> >> + * be different for xm and xl. For xm, it uses type=netfront; 
> >> + * for xl, it uses type=vif. So, for xm, should pass "netfront"; 
> >> + * for xl, should pass "vif". 
> >>   */ 
> >>  int 
> >>  xenParseConfigCommon(virConfPtr conf, 
> >>   virDomainDefPtr def, 
> >> - virCapsPtr caps) 
> >> + virCapsPtr caps, 
> >> + const char *vif_typename) 
> >  
> > One thing I didn't recall when suggesting this approach is that  
> xenParseVif() is 
> > called in xenParseConfigCommon(). I was thinking it was called from 
> > xen_{xl,xm}.c and the extra parameter would only be added to the 
> > xen{Format,Parse}Vif functions. I don't particularly like seeing the device 
> > specific parameter added to the common functions, but wont object if others 
> >  
> are 
> > fine with it. Any other opinions on that? Joao? 
> That's a good point - probably we can avoid it by using 
> xen{Format,Parse}Vif (with the signature change Chunyan proposes)  
> individually 
> on xenParseXM and xenParseXL. And there wouldn't be any xenParseConfigCommon 
> with device-specific parameters (as vif being one of the many devices that  
> the 
> routine is handling). The vif config is the same between xm and xl, with the 
> small difference wrt to the validation on xen libxl side - so having in 
> xen_common.c makes sense. 
>  
> > And one reason I wont object is that the alternative (calling 
> > xen{Format,Parse}Vif from xen_{xl,xm}.c) is a rather large change since all 
> >  
> the 
> > tests/{xl,xm}configdata/ files would need to be adjusted. 
> Hm, perhaps I fail to see what the large change would be. We would keep the  
> same 
> interface (i.e. model=netfront as valid on libvirt-side and converting to 
> type="vif" where applicable (libxl)) then the xml and .cfg won't change.  

In fact at first I changed codes as Jim suggested, didn't change 
xenParseConfigCommon(), but extract xen{Format,Parse}Vif out from
xenParseConfigCommon() and called from xen_{xl,xm}.c directly. But that
will change the order device appears in .cfg or xml. So existing xml and
.cfg will fail many times.

(I spent quite a bit of time to update xml and .cfg to make all of them correct,
 still some not pass. That's why finally I updated xenParseConfigCommon()).

> Furthermore, we 

Re: [libvirt] [PATCH V4 4/6] libxl: support creating domain with VF assignment from a pool

[Chun Yan Liu]

>>> On 3/29/2016 at 08:05 AM, in message <56f9c6d0.5000...@suse.com>, Jim Fehlig
 wrote: 
> On 03/21/2016 02:11 AM, Chunyan Liu wrote: 
> > Add codes to support creating domain with network defition of assigning 
> > SRIOV VF from a pool. 
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_domain.c | 50  
>  
> >  tests/Makefile.am|  3 +++ 
> >  2 files changed, 53 insertions(+) 
> > 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index c8d09b1..d11bf3a 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -36,6 +36,7 @@ 
> >  #include "virtime.h" 
> >  #include "locking/domain_lock.h" 
> >  #include "xen_common.h" 
> > +#include "network/bridge_driver.h" 
> >   
> >  #define VIR_FROM_THIS VIR_FROM_LIBXL 
> >   
> > @@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, 
> >  if (net->ifname && 
> >  STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) 
> >  VIR_FREE(net->ifname); 
> > + 
> > +/* cleanup actual device */ 
> > +virDomainNetRemoveHostdev(vm->def, net); 
> > +networkReleaseActualDevice(vm->def, net); 
> >  } 
> >  } 
> >   
> > @@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def,  
> libxl_domain_config *d_config) 
> >  } 
> >  } 
> >   
> > +static int 
> > +libxlNetworkPrepareDevices(virDomainDefPtr def) 
> > +{ 
> > +int ret = -1; 
> > +size_t i; 
> > + 
> > +for (i = 0; i < def->nnets; i++) { 
> > +virDomainNetDefPtr net = def->nets[i]; 
> > +int actualType; 
> > + 
> > +/* If appropriate, grab a physical device from the configured 
> > + * network's pool of devices, or resolve bridge device name 
> > + * to the one defined in the network definition. 
> > + */ 
> > +if (networkAllocateActualDevice(def, net) < 0) 
> > +goto cleanup; 
> > + 
> > +actualType = virDomainNetGetActualType(net); 
> > +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && 
> > +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { 
> > +/* Each type='hostdev' network device must also have a 
> > + * corresponding entry in the hostdevs array. For netdevs 
> > + * that are hardcoded as type='hostdev', this is already 
> > + * done by the parser, but for those allocated from a 
> > + * network / determined at runtime, we need to do it 
> > + * separately. 
> > + */ 
> > +virDomainHostdevDefPtr hostdev =  
> virDomainNetGetActualHostdev(net); 
> > +virDomainHostdevSubsysPCIPtr pcisrc =  
> >source.subsys.u.pci; 
> > + 
> > +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && 
> > +hostdev->source.subsys.type ==  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; 
> > + 
> > +if (virDomainHostdevInsert(def, hostdev) < 0) 
> > +goto cleanup; 
> > +} 
> > +} 
> > +ret = 0; 
> > + cleanup: 
> > +return ret; 
>  
> Nothing to cleanup here. I think we should just return -1 on failure paths  
> and 0 
> on success. 
>  
> > +} 
> >   
> >  /* 
> >   * Start a domain through libxenlight. 
> > @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto cleanup; 
>  
> I accessed a machine to test this series and have found a few problems. 
>  
> 1. When attaching an SR-IOV vf from a pool, the attach appears successful. 
>I see that xen's pciback driver is bound to the device in the host. I 
>didn't see the device in the guest (could be a guest problem), 

For pv guest, from testing, it is shown in guest. For HVM, seems
xl pci-attach has the same result.

> nor in 
>dumpxml or /var/run/libvirt/libxl/.xml.

Ah, there is a mistake during a review change to original patch 1/6.
Didn't notice that earlier.

In libxlDomainAttachNetDevice:
The if (!ret) is needed, maybe cleanup is not proper.
Since for actual type is hostdev case, after a successful
libxlDomainAttachHostDevice, we need to
update vm->def->nets too.

  cleanup:
 libxl_device_nic_dispose();
- out:
-if (!ret)
+if (!ret) {
 vm->def->nets[vm->def->nnets++] = net;
+} else {
+virDomainNetRemoveHostdev(vm->def, net);
+networkReleaseActualDevice(vm->def, net);
+}

Similar for libxDomainDetachNetDevice cleanup:
Original is still needed.

  cleanup:
 libxl_device_nic_dispose();
-if (!ret)
+if (!ret) {
+networkReleaseActualDevice(vm->def, detach);
 

Re: [libvirt] [PATCH V4 6/6] libxlDomainStart: correct cleanup after failure

[Chun Yan Liu]

>>> On 3/29/2016 at 09:00 AM, in message <56f9d3c0.7020...@suse.com>, Jim Fehlig
 wrote: 
> On 03/21/2016 02:11 AM, Chunyan Liu wrote: 
> > Functions like libxlNetworkPrepareDevices, libxlBuildDomainConfig, 
> > virHostdevPrepareDomainDevices will allocate and reserve some 
> > resources. In libxlDomainStart, after calling these functions, 
> > in case of failure, we need to call libxlDomainCleanup to check 
> > and release resoureces. 
> > Besides, since libxlDomainCleanup will call virDomainLockProcessPause, 
> > so we move virDomainLockProcessStart/Resume to earlier stage. 
>  
> I think this cleanup should be done before applying 4/6. It is really 
> independent of support for vf from an SR-IOV pool. 
>  
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_domain.c | 41 ++--- 
> >  1 file changed, 22 insertions(+), 19 deletions(-) 
> > 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index d11bf3a..6855ce4 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -1083,20 +1083,13 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > -if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > -goto cleanup; 
> > - 
> > -if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > -   cfg->ctx, _config) < 0) 
> > -goto cleanup; 
> > - 
> > -if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, _config) < 0) 
> > -goto cleanup; 
> > - 
> > -if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > -   vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> > -goto cleanup; 
> > - 
> > +/* libxlNetworkPrepareDevices, libxlBuildDomainConfig, 
> > + * virHostdevPrepareDomainDevices will allocate and reserve 
> > + * some resources. In case of failure, need to release 
> > + * resoureces. This can be done by calling libxlDomainCleanup. 
> > + * Since libxlDomainCleanup will call virDomainLockProcessPause, 
> > + * so we move virDomainLockProcessStart/Resume to here. 
> > + */ 
>  
> I don't think the comment is necessary. Without looking at git history, one 
> wouldn't know where it was moved from :-). 
>  
> >  if (virDomainLockProcessStart(driver->lockManager, 
> >"xen:///system", 
> >vm, 
> > @@ -,6 +1104,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  goto cleanup; 
> >  VIR_FREE(priv->lockState); 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto release_dom; 
> > + 
> > +if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > +   cfg->ctx, _config) < 0) 
> > +goto release_dom; 
> > + 
> > +if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, _config) < 0) 
> > +goto release_dom; 
> > + 
> > +if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +   vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> > +goto release_dom; 
> > + 
> >  /* Unlock virDomainObj while creating the domain */ 
> >  virObjectUnlock(vm); 
> >   
> > @@ -1194,16 +1201,12 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >   
> >   cleanup_dom: 
> >  ret = -1; 
> > -if (priv->deathW) { 
> > -libxl_evdisable_domain_death(cfg->ctx, priv->deathW); 
> > -priv->deathW = NULL; 
> > -} 
>  
> Disabling the death event should be done in another patch IMO, after adding  
> the 
> call to libxlDomainCleanup. 
>  
> >  libxlDomainDestroyInternal(driver, vm); 
> >  vm->def->id = -1; 
> >  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,  
> VIR_DOMAIN_SHUTOFF_FAILED); 
> >   
> >   release_dom: 
> > -virDomainLockProcessPause(driver->lockManager, vm, >lockState); 
> > +libxlDomainCleanup(driver, vm); 
>  
> release_dom is not the best name for the label after this change. I've split 
> this patch into 3 patches and posted the result 
>  
> https://www.redhat.com/archives/libvir-list/2016-March/msg01310.html 

Reviewed the split 3 patches, looks good.

Thanks Jim!

>  
> Can you review and comment on that series? As mentioned above, I'd like to  
> get 
> the resource leaks plugged in libxlDomainStart before adding 4/6. 
>  
> Regards, 
> Jim 
>  
>  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] libxl: only disable domain death events in libxlDomainCleanup

[Chun Yan Liu]

Reviewed-by: Chunyan Liu 

>>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-4-git-send-email-jfeh...@suse.com>, Jim Fehlig
 wrote: 
> Remove disabling domain death events from libxlDomainStart error 
> path. The domain death event is already disabled in libxlDomainCleanup. 
>  
> Signed-off-by: Jim Fehlig  
> --- 
>  src/libxl/libxl_domain.c | 4  
>  1 file changed, 4 deletions(-) 
>  
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> index 068bfb6..04962a0 100644 
> --- a/src/libxl/libxl_domain.c 
> +++ b/src/libxl/libxl_domain.c 
> @@ -1144,10 +1144,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>   
>   destroy_dom: 
>  ret = -1; 
> -if (priv->deathW) { 
> -libxl_evdisable_domain_death(cfg->ctx, priv->deathW); 
> -priv->deathW = NULL; 
> -} 
>  libxlDomainDestroyInternal(driver, vm); 
>  vm->def->id = -1; 
>  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,  
> VIR_DOMAIN_SHUTOFF_FAILED); 
 



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] libxl: fix resource leaks in libxlDomainStart error paths

[Chun Yan Liu]

Reviewed-by: Chunyan Liu 

>>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-3-git-send-email-jfeh...@suse.com>, Jim Fehlig
 wrote: 
> From: Chunyan Liu  
>  
> libxlDomainStart allocates and reserves resources that were not 
> being released in error paths. libxlDomainCleanup already handles 
> the job of releasing resources, and libxlDomainStart should call 
> it when encountering a failure. 
>  
> Change the error handling logic to call libxlDomainCleanup on 
> failure. This includes acquiring the lease sooner and allowing 
> it to be released in libxlDomainCleanup on failure, similar to 
> the way other resources are reclaimed. With the lease now 
> released in libxlDomainCleanup, the release_dom label can be 
> renamed to cleanup_dom to better reflect its changed semantics. 
>  
> Signed-off-by: Jim Fehlig  
> --- 
>  src/libxl/libxl_domain.c | 28 ++-- 
>  1 file changed, 14 insertions(+), 14 deletions(-) 
>  
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> index ead3d09..068bfb6 100644 
> --- a/src/libxl/libxl_domain.c 
> +++ b/src/libxl/libxl_domain.c 
> @@ -1036,17 +1036,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  vm, true) < 0) 
>  goto cleanup; 
>   
> -if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> -   cfg->ctx, _config) < 0) 
> -goto cleanup; 
> - 
> -if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, _config) < 0) 
> -goto cleanup; 
> - 
> -if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> -   vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> -goto cleanup; 
> - 
>  if (virDomainLockProcessStart(driver->lockManager, 
>"xen:///system", 
>vm, 
> @@ -1061,6 +1050,17 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  goto cleanup; 
>  VIR_FREE(priv->lockState); 
>   
> +if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> +   cfg->ctx, _config) < 0) 
> +goto cleanup_dom; 
> + 
> +if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, _config) < 0) 
> +goto cleanup_dom; 
> + 
> +if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> +   vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> +goto cleanup_dom; 
> + 
>  /* Unlock virDomainObj while creating the domain */ 
>  virObjectUnlock(vm); 
>   
> @@ -1091,7 +1091,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  virReportError(VIR_ERR_INTERNAL_ERROR, 
> _("libxenlight failed to restore domain '%s'"), 
> d_config.c_info.name); 
> -goto release_dom; 
> +goto cleanup_dom; 
>  } 
>   
>  /* 
> @@ -1152,8 +1152,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  vm->def->id = -1; 
>  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,  
> VIR_DOMAIN_SHUTOFF_FAILED); 
>   
> - release_dom: 
> -virDomainLockProcessPause(driver->lockManager, vm, >lockState); 
> + cleanup_dom: 
> +libxlDomainCleanup(driver, vm); 
>   
>   cleanup: 
>  libxl_domain_config_dispose(_config); 
 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] libxl: rename cleanup_dom label

[Chun Yan Liu]

Reviewed-by: Chunyan Liu 

>>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-2-git-send-email-jfeh...@suse.com>, Jim Fehlig
 wrote: 
> Rename cleanup_dom label to destroy_dom, which better describes what 
> it does. 
>  
> Signed-off-by: Jim Fehlig  
> --- 
>  src/libxl/libxl_domain.c | 12 ++-- 
>  1 file changed, 6 insertions(+), 6 deletions(-) 
>  
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> index c8d09b1..ead3d09 100644 
> --- a/src/libxl/libxl_domain.c 
> +++ b/src/libxl/libxl_domain.c 
> @@ -1102,22 +1102,22 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>   
>  /* Always enable domain death events */ 
>  if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, 
> >deathW)) 
> -goto cleanup_dom; 
> +goto destroy_dom; 
>   
>  libxlDomainCreateIfaceNames(vm->def, _config); 
>   
>  if ((dom_xml = virDomainDefFormat(vm->def, cfg->caps, 0)) == NULL) 
> -goto cleanup_dom; 
> +goto destroy_dom; 
>   
>  if (libxl_userdata_store(cfg->ctx, domid, "libvirt-xml", 
>   (uint8_t *)dom_xml, strlen(dom_xml) + 1)) { 
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 
> _("libxenlight failed to store userdata")); 
> -goto cleanup_dom; 
> +goto destroy_dom; 
>  } 
>   
>  if (libxlDomainSetVcpuAffinities(driver, vm) < 0) 
> -goto cleanup_dom; 
> +goto destroy_dom; 
>   
>  if (!start_paused) { 
>  libxl_domain_unpause(cfg->ctx, domid); 
> @@ -1127,7 +1127,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  } 
>   
>  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, cfg->caps) < 
> 0) 
> -goto cleanup_dom; 
> +goto destroy_dom; 
>   
>  if (virAtomicIntInc(>nactive) == 1 && driver->inhibitCallback) 
>  driver->inhibitCallback(true, driver->inhibitOpaque); 
> @@ -1142,7 +1142,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
>  ret = 0; 
>  goto cleanup; 
>   
> - cleanup_dom: 
> + destroy_dom: 
>  ret = -1; 
>  if (priv->deathW) { 
>  libxl_evdisable_domain_death(cfg->ctx, priv->deathW); 
 



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V4 4/6] libxl: support creating domain with VF assignment from a pool

[Chun Yan Liu]

>>> On 3/29/2016 at 08:05 AM, in message <56f9c6d0.5000...@suse.com>, Jim Fehlig
 wrote: 
> On 03/21/2016 02:11 AM, Chunyan Liu wrote: 
> > Add codes to support creating domain with network defition of assigning 
> > SRIOV VF from a pool. 
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> >  src/libxl/libxl_domain.c | 50  
>  
> >  tests/Makefile.am|  3 +++ 
> >  2 files changed, 53 insertions(+) 
> > 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index c8d09b1..d11bf3a 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -36,6 +36,7 @@ 
> >  #include "virtime.h" 
> >  #include "locking/domain_lock.h" 
> >  #include "xen_common.h" 
> > +#include "network/bridge_driver.h" 
> >   
> >  #define VIR_FROM_THIS VIR_FROM_LIBXL 
> >   
> > @@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, 
> >  if (net->ifname && 
> >  STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) 
> >  VIR_FREE(net->ifname); 
> > + 
> > +/* cleanup actual device */ 
> > +virDomainNetRemoveHostdev(vm->def, net); 
> > +networkReleaseActualDevice(vm->def, net); 
> >  } 
> >  } 
> >   
> > @@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def,  
> libxl_domain_config *d_config) 
> >  } 
> >  } 
> >   
> > +static int 
> > +libxlNetworkPrepareDevices(virDomainDefPtr def) 
> > +{ 
> > +int ret = -1; 
> > +size_t i; 
> > + 
> > +for (i = 0; i < def->nnets; i++) { 
> > +virDomainNetDefPtr net = def->nets[i]; 
> > +int actualType; 
> > + 
> > +/* If appropriate, grab a physical device from the configured 
> > + * network's pool of devices, or resolve bridge device name 
> > + * to the one defined in the network definition. 
> > + */ 
> > +if (networkAllocateActualDevice(def, net) < 0) 
> > +goto cleanup; 
> > + 
> > +actualType = virDomainNetGetActualType(net); 
> > +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && 
> > +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { 
> > +/* Each type='hostdev' network device must also have a 
> > + * corresponding entry in the hostdevs array. For netdevs 
> > + * that are hardcoded as type='hostdev', this is already 
> > + * done by the parser, but for those allocated from a 
> > + * network / determined at runtime, we need to do it 
> > + * separately. 
> > + */ 
> > +virDomainHostdevDefPtr hostdev =  
> virDomainNetGetActualHostdev(net); 
> > +virDomainHostdevSubsysPCIPtr pcisrc =  
> >source.subsys.u.pci; 
> > + 
> > +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && 
> > +hostdev->source.subsys.type ==  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; 
> > + 
> > +if (virDomainHostdevInsert(def, hostdev) < 0) 
> > +goto cleanup; 
> > +} 
> > +} 
> > +ret = 0; 
> > + cleanup: 
> > +return ret; 
>  
> Nothing to cleanup here. I think we should just return -1 on failure paths  
> and 0 
> on success. 
>  
> > +} 
> >   
> >  /* 
> >   * Start a domain through libxenlight. 
> > @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto cleanup; 
>  
> I accessed a machine to test this series and have found a few problems. 
>  
> 1. When attaching an SR-IOV vf from a pool, the attach appears successful. 
>I see that xen's pciback driver is bound to the device in the host. I 
>didn't see the device in the guest (could be a guest problem), nor in 
>dumpxml or /var/run/libvirt/libxl/.xml. Not surprisingly, 
>this causes problems with device-detach 
>  
># virsh detach-device dom sriov-pool-vif.xml 
>error: Failed to detach device from sriov-pool-vif.xml 
>error: operation failed: no device matching mac address 00:16:3e:7a:35:df  
> found 
>  
> 2. After starting a domain containing an interface from sr-iov vf pool, the 
>interface xml is changed from type='network' to type='hostdev'. E.g. 
>before creating domain 
>  
>  
>
>
>  
>  
>after creating domain 
>  
>  
>
>
>
>  function='0x1'/> 
>
>  
>  
>Maybe this is intended behavior, but it seems odd. 
>  
> 3. I started a domain containing an interface from sr-iov vf pool. Looks 
>good. I then tried 'virsh detach-device ...', which never returned 
>until keepalive timeout. The device was removed from the guest, but 

Re: [libvirt] [PATCH V3 2/2] libxl: support assignment from a pool of SRIOV VFs

[Chun Yan Liu]

>>> On 3/11/2016 at 05:56 AM, in message <56e1ed8f.4070...@suse.com>, Jim Fehlig
 wrote: 
> Chunyan Liu wrote: 
> > Add codes to support creating domain with network defition of assigning 
> > SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this 
> > kind of network defition. 
> >  
> > Signed-off-by: Chunyan Liu  
> > --- 
> > Changes: 
> >   * move a common change in libxl_conf.c into separate patch 
> >  
> >  src/libxl/libxl_domain.c | 46  
> ++ 
> >  src/libxl/libxl_driver.c | 12  
> >  tests/Makefile.am|  3 +++ 
> >  3 files changed, 61 insertions(+) 
> >  
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index c8d09b1..8191911 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -36,6 +36,7 @@ 
> >  #include "virtime.h" 
> >  #include "locking/domain_lock.h" 
> >  #include "xen_common.h" 
> > +#include "network/bridge_driver.h" 
> >   
> >  #define VIR_FROM_THIS VIR_FROM_LIBXL 
> >   
> > @@ -960,6 +961,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def,  
> libxl_domain_config *d_config) 
> >  } 
> >  } 
> >   
> > +static int 
> > +libxlNetworkPrepareDevices(virDomainDefPtr def) 
> > +{ 
> > +int ret = -1; 
> > +size_t i; 
> > + 
> > +for (i = 0; i < def->nnets; i++) { 
> > +virDomainNetDefPtr net = def->nets[i]; 
> > +int actualType; 
> > + 
> > +/* If appropriate, grab a physical device from the configured 
> > + * network's pool of devices, or resolve bridge device name 
> > + * to the one defined in the network definition. 
> > + */ 
> > +if (networkAllocateActualDevice(def, net) < 0) 
> > +goto cleanup; 
>  
> I noticed that other callers of networkAllocateActualDevice() call 
> networkReleaseActualDevice() in error paths,  
> qemuProcessNetworkPrepareDevices() 
> being a notable exception. Is it necessary to call  
> networkReleaseActualDevice() 
> in 'cleanup' if this function fails?

Not necessary in this function 'cleanup', but needed to call somewhere else,
e.g. this function is called in libxlDomainStart, so whenever libxlDomainStart
fails, need to call networkAllocateActualDevice to release resources. It will
update dev->connections for next time correct handling (if for any vif in
the pool, dev->connections != 0, it's considered as unavailable.)

Places need to call networkReleaseActualDevice(): in libxlDomainCleanup,
when libxlDomainStart fails, in hot-remove device, when hot-add device fails.

Will update and repost.

BTW: in libxlDomainStart, when it fails, why not call libxlDomainCleanup?

- Chunyan
 
>  
> Regards, 
> Jim 
>  
> > + 
> > +actualType = virDomainNetGetActualType(net); 
> > +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && 
> > +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { 
> > +/* Each type='hostdev' network device must also have a 
> > + * corresponding entry in the hostdevs array. For netdevs 
> > + * that are hardcoded as type='hostdev', this is already 
> > + * done by the parser, but for those allocated from a 
> > + * network / determined at runtime, we need to do it 
> > + * separately. 
> > + */ 
> > +virDomainHostdevDefPtr hostdev =  
> virDomainNetGetActualHostdev(net); 
> > +virDomainHostdevSubsysPCIPtr pcisrc =  
> >source.subsys.u.pci; 
> > + 
> > +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && 
> > +hostdev->source.subsys.type ==  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; 
> > + 
> > +if (virDomainHostdevInsert(def, hostdev) < 0) 
> > +goto cleanup; 
> > +} 
> > +} 
> > +ret = 0; 
> > + cleanup: 
> > +return ret; 
> > +} 
> >   
> >  /* 
> >   * Start a domain through libxenlight. 
> > @@ -1036,6 +1079,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto cleanup; 
> > + 
> >  if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > cfg->ctx, _config) < 0) 
> >  goto cleanup; 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 87ec5a5..a0b157b 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -2982,6 +2982,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >   
> >  libxl_device_pci_init(); 
> >   
> > +/* For those allocated from a network pool/ determined at runtime,  
> it's 
> > + * not handled by libxlDomainDeviceDefPostParse as hostdev, we need to 
> > + 

Re: [libvirt] [PATCH V2] libxl: support assignment from a pool of SRIOV VFs

[Chun Yan Liu]

>>> On 3/10/2016 at 08:08 AM, in message <56e0bb0f.3000...@suse.com>, Jim Fehlig
 wrote: 
> Hi Chunyan, 
>  
> Sorry for the long delay... 
>  
> On 02/23/2016 01:07 AM, Chunyan Liu wrote: 
> > Add codes to support creating domain with network defition of assigning 
> > SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this 
> > kind of network defition. 
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> > Changes: 
> > * move bug fix to another patch 
> > * use virDomainNetGetActualType instead of multiple times checking 
> > --- 
> >  src/libxl/libxl_conf.c   |  3 ++- 
> >  src/libxl/libxl_domain.c | 46  
> ++ 
> >  src/libxl/libxl_driver.c | 12  
> >  tests/Makefile.am|  3 +++ 
> >  4 files changed, 63 insertions(+), 1 deletion(-) 
> > 
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c 
> > index 48b8826..3cefbaa 100644 
> > --- a/src/libxl/libxl_conf.c 
> > +++ b/src/libxl/libxl_conf.c 
> > @@ -1453,7 +1453,8 @@ libxlMakeNicList(virDomainDefPtr def,   
> libxl_domain_config *d_config) 
> >  return -1; 
> >   
> >  for (i = 0; i < nnics; i++) { 
> > -if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) 
> > +if (virDomainNetGetActualType(l_nics[i]) 
> > +== VIR_DOMAIN_NET_TYPE_HOSTDEV) 
>  
> AFAICT this change is unrelated to the patch subject and should be done in a 
> separate patch. Also the patch no longer applies cleanly. Can you drop this 
> hunk, rebase, and send a V3? Thanks! 

Updated. V3 is posted.

Thanks,
Chunyan

>  
> Regards, 
> Jim 
>  
> >  continue; 
> >   
> >  if (libxlMakeNic(def, l_nics[i], _nics[nvnics])) 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index 50f7eed..88d1399 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -36,6 +36,7 @@ 
> >  #include "virtime.h" 
> >  #include "locking/domain_lock.h" 
> >  #include "xen_common.h" 
> > +#include "network/bridge_driver.h" 
> >   
> >  #define VIR_FROM_THIS VIR_FROM_LIBXL 
> >   
> > @@ -929,6 +930,48 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev,  
> void *for_callback) 
> >  libxl_event_free(ctx, ev); 
> >  } 
> >   
> > +static int 
> > +libxlNetworkPrepareDevices(virDomainDefPtr def) 
> > +{ 
> > +int ret = -1; 
> > +size_t i; 
> > + 
> > +for (i = 0; i < def->nnets; i++) { 
> > +virDomainNetDefPtr net = def->nets[i]; 
> > +int actualType; 
> > + 
> > +/* If appropriate, grab a physical device from the configured 
> > + * network's pool of devices, or resolve bridge device name 
> > + * to the one defined in the network definition. 
> > + */ 
> > +if (networkAllocateActualDevice(def, net) < 0) 
> > +goto cleanup; 
> > + 
> > +actualType = virDomainNetGetActualType(net); 
> > +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && 
> > +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { 
> > +/* Each type='hostdev' network device must also have a 
> > + * corresponding entry in the hostdevs array. For netdevs 
> > + * that are hardcoded as type='hostdev', this is already 
> > + * done by the parser, but for those allocated from a 
> > + * network / determined at runtime, we need to do it 
> > + * separately. 
> > + */ 
> > +virDomainHostdevDefPtr hostdev =  
> virDomainNetGetActualHostdev(net); 
> > +virDomainHostdevSubsysPCIPtr pcisrc =  
> >source.subsys.u.pci; 
> > + 
> > +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && 
> > +hostdev->source.subsys.type ==  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; 
> > + 
> > +if (virDomainHostdevInsert(def, hostdev) < 0) 
> > +goto cleanup; 
> > +} 
> > +} 
> > +ret = 0; 
> > + cleanup: 
> > +return ret; 
> > +} 
> >   
> >  /* 
> >   * Start a domain through libxenlight. 
> > @@ -1005,6 +1048,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto cleanup; 
> > + 
> >  if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > cfg->ctx, _config) < 0) 
> >  goto cleanup; 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 404016e..eef6651 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -3049,6 +3049,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >   
> >  libxl_device_pci_init(); 
> >   
> > +/* For those allocated from a network 

[libvirt] 答复： Re: [PATCH] libxl: support assignment from a pool of SRIOV VFs

[Chun Yan Liu]

>>> Jim Fehlig  2016-2-20 上午 7:30 >>>
On 01/25/2016 07:24 PM, Chunyan Liu wrote:
> Add codes to support creating domain with network defition of assigning
> SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this
> kind of network defition.
>
> Signed-off-by: Chunyan Liu 
> ---
>  src/libxl/libxl_conf.c   |  5 +++--
>  src/libxl/libxl_domain.c | 48 
> +++-
>  src/libxl/libxl_driver.c | 12 
>  tests/Makefile.am|  3 +++
>  4 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 6320421..f50c68a 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1260,7 +1260,8 @@ libxlMakeNicList(virDomainDefPtr def,  
> libxl_domain_config *d_config)
>  return -1;
>  
>  for (i = 0; i < nnics; i++) {
> -if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
> +if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
> +l_nics[i]->data.network.actual->type == 
> VIR_DOMAIN_NET_TYPE_HOSTDEV)

 > Elsewhere we use the virDomainNetGetActualType() accessor function.
 Ah, yes, I'll update.

>  continue;
>  
>  if (libxlMakeNic(def, l_nics[i], _nics[nvnics]))
> @@ -1278,7 +1279,7 @@ libxlMakeNicList(virDomainDefPtr def,  
> libxl_domain_config *d_config)
>  
>  VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
>  d_config->nics = x_nics;
> -d_config->num_nics = nnics;
> +d_config->num_nics = nvnics;

 > This looks like a bug fix. If so, it should be in a separate patch.
 Yes. Will put in a separate patch.

>  
>  return 0;
>  
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index cf5c9f6..9bf7a5a 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -35,6 +35,7 @@
>  #include "virstring.h"
>  #include "virtime.h"
>  #include "locking/domain_lock.h"
> +#include "network/bridge_driver.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>  
> @@ -314,7 +315,7 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>  virDomainHostdevSubsysPCIPtr pcisrc;
>  
>  if (dev->type == VIR_DOMAIN_DEVICE_NET)
> -hostdev = &(dev->data.net)->data.hostdev.def;
> +hostdev = >data.net->data.hostdev.def;

 > This looks like a bug fix too.
 Yes. With '()' has no harm, but it doesn't need.

>  else
>  hostdev = dev->data.hostdev;
>  pcisrc = >source.subsys.u.pci;
> @@ -916,6 +917,48 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, 
> void *for_callback)
>  libxl_event_free(ctx, ev);
>  }
>  
> +static int
> +libxlNetworkPrepareDevices(virDomainDefPtr def)
> +{
> +int ret = -1;
> +size_t i;
> +
> +for (i = 0; i < def->nnets; i++) {
> +virDomainNetDefPtr net = def->nets[i];
> +int actualType;
> +
> +/* If appropriate, grab a physical device from the configured
> + * network's pool of devices, or resolve bridge device name
> + * to the one defined in the network definition.
> + */
> +if (networkAllocateActualDevice(def, net) < 0)
> +goto cleanup;
> +
> +actualType = virDomainNetGetActualType(net);
> +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +/* Each type='hostdev' network device must also have a
> + * corresponding entry in the hostdevs array. For netdevs
> + * that are hardcoded as type='hostdev', this is already
> + * done by the parser, but for those allocated from a
> + * network / determined at runtime, we need to do it
> + * separately.
> + */
> +virDomainHostdevDefPtr hostdev = 
> virDomainNetGetActualHostdev(net);
> +virDomainHostdevSubsysPCIPtr pcisrc = 
> >source.subsys.u.pci;
> +

 > I n > see if the hostdev is already in use. Is that needed here too?
 Later when doing HostdevAttach, it will check. So I think it's not necessary.

> +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> +hostdev->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)

 > Does this need to include checking if backend is already set 
 > (pcisrc->backend ==
 > VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) ? I suppose it should always be
 > VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN.
 There are two cases, one is specified as , for this 
case, it
 will be handled during DeviceDefParse phase, it's already set as BACKEND_XEN;
 another case is from a pool of SRIOV vfs, it won't be handled during 
DeviceDefParse
 as a hostdev, so the backend type is still BACKEND_DEFAULT, for this case, we
 need to set it to be BACKEND_XEN.
 

> +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
> +
> +if (virDomainHostdevInsert(def, hostdev) < 0)
> +goto 

Re: [libvirt] [PATCH] fix vol-create when target allocation=0

[Chun Yan Liu]

 On 8/24/2015 at 05:47 PM, in message 20150824094707.GD22373@wheatley, 
 Martin
Kletzander mklet...@redhat.com wrote: 
 On Mon, Aug 24, 2015 at 01:50:41PM +0800, Chunyan Liu wrote: 
 Regression is introduced by commit e30297b0. After that, it will 
 report Cannot fill file error with following xml file: 
 volume 
   namevirtinst-vmlinuz-xen.tP1NHh/name 
   capacity4343792/capacity 
   allocation0/allocation 
   target 
 format type=raw/ 
 nocow/ 
   /target 
 /volume 
 Because of this, installing xen pv guest with virt-manager fails. 
  
 Reason is: posix_fallocate(int fd, off_t offset, off_t len) will 
 return EINVAL when len is equal to 0. So, this patch adds a check 
 before calling safezero(). 
  
  
 Oh, god that you've found out the root cause, although the same patch 
 is already on the list: 
  
 https://www.redhat.com/archives/libvir-list/2015-August/msg00808.html 

Oh yes, it's the same patch. Sorry for not reading the message in time. Really
coincidental sending same patch from different person one after another :)

Chunyan

  
 Although it would be nice if it had the root cause (or at least the 
 causing commit) in its commit message as well.  Cc'ing the author of 
 the previous patch in case he hasn't pushed yet. 
  
 Martin 
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] vm snapshot multi-disk

[Chun Yan Liu]

 On 7/14/2015 at 05:42 AM, in message
CALFpzo551ioR=w5asf7jxmtp8egqsp2au_o6awmyofz9mfg...@mail.gmail.com, Marcus
shadow...@gmail.com wrote: 
 Oh, I almost forgot to mention the versions: 
  
 libvirt 1.2.8-16.0.1.el7_1.2.x86_64 
  
 qemu 2.1.2-23.el7_1.1.x86_64

Just FYI, tested on my machine, libvirt 1.2.5, qemu 2.0.0, didn't see this 
problem.
Both disk snapshots are deleted.

  
  
 Also, I'm unclear if the domain snapshot feature is orchestralted by 
 libvirt, or something that is simply called into qemu to take care of. 
 Please forgive me if this is a qemu issue. 
  
 On Mon, Jul 13, 2015 at 2:35 PM, Marcus shadow...@gmail.com wrote: 
  
  Hi all, 
  
  I've recently been toying with VM snapshots, and have ran into an 
  issue. Given a VM with multiple disks, it seems a snapshot-create followed 
  by a snapshot-delete will only remove the qcow2 snapshot for the first disk 
  (or perhaps just the disk that contains the memory), not all of the disk 
  snapshots it created. Is this something people are aware of? 
  
  In searching around, I found a bug report where snapshot-creates would 
  fail due to the qcow2 snapshot ids being inconsistent. That looks like it 
  is patched for 2.4 qemu ( 
  http://lists.nongnu.org/archive/html/qemu-devel/2015-03/msg04963.html), 
  this bug would trigger that one by leaving IDs around that are inconsistent 
  between member disks, but is not the same. 
  
  # virsh snapshot-create 7 
  
  Domain snapshot 1436792720 created 
  
  
  # virsh snapshot-list 7 
  
   Name Creation Time State 
  
   
  
   1436792720   2015-07-13 06:05:20 -0700 running 
  
  
  # virsh domblklist 7 
  
  Target Source 
  
   
  
  vda 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/e4d6e885-1382-40bc-890b-ad9c8b51a7a 
 5 
  
  vdb 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/7033e4c6-5f59-4325-b7e0-ae191e12e86 
 c 
  
  
  # qemu-img snapshot -l 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/e4d6e885-1382-40bc-890b-ad9c8b51a7a 
 5 
  
  Snapshot list: 
  
  IDTAG VM SIZEDATE   VM CLOCK 
  
  1 1436792720 173M 2015-07-13 06:05:20   00:01:10.938 
  
  
  # qemu-img snapshot -l 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/7033e4c6-5f59-4325-b7e0-ae191e12e86 
 c 
  
  Snapshot list: 
  
  IDTAG VM SIZEDATE   VM CLOCK 
  
  1 14367927200 2015-07-13 06:05:20   00:01:10.938 
  
  
  # virsh snapshot-delete 7 1436792720 
  
  Domain snapshot 1436792720 deleted 
  
  
  # qemu-img snapshot -l 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/e4d6e885-1382-40bc-890b-ad9c8b51a7a 
 5 
  
  # qemu-img snapshot -l 
   
 /mnt/2a270ef3-f389-37a4-942f-380bed9f70aa/7033e4c6-5f59-4325-b7e0-ae191e12e86 
 c 
  
  Snapshot list: 
  
  IDTAG VM SIZEDATE   VM CLOCK 
  
  1 14367927200 2015-07-13 06:05:20   00:01:10.938 
  
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/23/2014 at 03:32 PM, in message 54991AA7.243 : 102 : 21807, Chun Yan
Liu wrote: 

  
 On 12/23/2014 at 11:42 AM, in message 5498e4ba.1000...@redhat.com, John 
 Ferlan jfer...@redhat.com wrote:  
  

 On 12/22/2014 09:55 PM, Chun Yan Liu wrote:  


  On 12/22/2014 at 08:17 PM, in message 54980bf2.1060...@redhat.com, 
  John  
  Ferlan jfer...@redhat.com wrote:   

 
  On 12/21/2014 10:15 PM, Chun Yan Liu wrote:   
 
 
  On 12/19/2014 at 08:03 PM, in message 54941429.8000...@redhat.com, 
  John   
  Ferlan jfer...@redhat.com wrote:
 
  
  On 12/19/2014 12:31 AM, Chun Yan Liu wrote:
   
   
  On 12/18/2014 at 01:00 PM, in message
  5492d008026600086...@soto.provo.novell.com, Chun Yan Liu
  cy...@suse.com wrote:
   
   
  On 12/17/2014 at 06:52 PM, in message 
  20141217105227.gq136...@orkuz.home,
  Jiri Denemark jdene...@redhat.com wrote:
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote:
  Add public API virDomainSendSysrq for sending SysRequest key.
   
  Signed-off-by: Chunyan Liu cy...@suse.com
  ---
  changes:
 * add 'flags' to the new API
 * change parameter from 'const char *key' to 'char key'
 * change version number from 1.2.11 to 1.2.12
   
include/libvirt/libvirt-domain.h |  3 +++
src/driver-hypervisor.h  |  4 
src/libvirt-domain.c | 39
  +++
src/libvirt_public.syms  |  5 +
4 files changed, 51 insertions(+)
   
  diff --git a/include/libvirt/libvirt-domain.h
  b/include/libvirt/libvirt-domain.h
  index baef32d..5f72850 100644
  --- a/include/libvirt/libvirt-domain.h
  +++ b/include/libvirt/libvirt-domain.h
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom,
   virDomainFSInfoPtr **info,
   unsigned int flags);
   
  +/* virDomainSendSysrq */
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int 
  flags);
  +
   
  I think quite a few reviewers (Daniel, Eric, and I) agreed on using 
  an
  enum instead of char so that the API is more general.
   
  Sorry, I missed this part. I'll update. One left question:
  How about 'virsh sysrq' parameters? What would we expect users to 
  pass?
   
  Any thoughts on that?
libxl_send_sysrq
  
  Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what  
 
  you had in mind for syntax previously - although it looks like:
  
   virsh sysrq domain [key]
 
  Thanks for reply. The syntax I'm used previously is:
  #virsh sysrq domain key   
  key is required. It's just a letter, like 'h', 'c', etc. About which
  options can we   
  have, on can refer to the results on guest through sysrq help. (that is, 
 
  issue   
  'virsh sysrq domain h' and look at guest kernel message. I think on each 
 
  guest,   
  there must be 'h' option, it will print help message.)   
 
  h, c, etc. doesn't tell me enough about what to expect from the   
  perspective of this naive user...  Passing 'h' via virsh to a driver   
  to return some help string that gets displayed where?  

  Guest kernel message if guest is Linux. xen/libxl just passes the key  
  blindly to guest kernel, so to pass 'h' to guest kernel, it just like one  
   
 issue:  
  #echo 'h'  /proc/sysrq-trigger  
  in a Linux guest, you will see in /var/log/messages:  

  SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)  
   memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)  
   sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)  
   nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)  
   unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)  
   show-blocked-tasks(w) dump-ftrace-buffer(z)  

   
 FWIW: My point on this was - by using 'virsh sysrq domain h' you don't  
 provide a mechanism to display this output.  
   
 Seems just from the descriptions some of those letters might return some  
 useful information... Some aren't one way commands.  Perhaps it would be  
 useful to capture output and be able to return it...  
  
 Right. But might be hard. 
 And I think of a problem when mapping enum to letter: 
 to different guests (e.g. Linux vs Windows), the letter to enum mapping  
 might 
 be different, even hypervisor may not precisely know that. At least for xen 
 hypervisor, I think it only blindly sends the key to guest, but has no idea  
 of 
 different key-letter meaning to different guests. 
  
 - Chunyan 

Hi, everyone, 

Happy New Year!

Pick up this thread again since I just could not follow your suggestions about
using 'enum' instead of 'char' as virDomainSendSysrq parameter and virsh sysrq
domain reboot|crash|... syntax. Summarize here so that you could remember it
and spot your

Re: [libvirt] [PATCH 2/2] Add tests to xmconfigtest

[Chun Yan Liu]

 On 12/23/2014 at 09:42 AM, in message 5498c888.6000...@suse.com, Jim 
 Fehlig
jfeh...@suse.com wrote: 
 Chunyan Liu wrote: 
  
 Hi Chunyan, 
  
 Thanks for the fix, and the test!  But I have a few questions regarding 
 the latter... 
  
  Add tests to testing HVM default features (pae, acpi, apic) 
  conversion from xm config to libvirt xml and from libvirt 
  xml to xm config. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
   .../xmconfigdata/test-fullvirt-default-feature.cfg | 23 +++ 
   .../test-fullvirt-default-feature.cfg.out  | 26  
   .../xmconfigdata/test-fullvirt-default-feature.xml | 48  
 ++ 
   tests/xmconfigtest.c   |  9 +++- 
   4 files changed, 105 insertions(+), 1 deletion(-) 
   create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg 
   create mode 100644 
  tests/xmconfigdata/test-fullvirt-default-feature.cfg.out 
   create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.xml 
  
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg  
 b/tests/xmconfigdata/test-fullvirt-default-feature.cfg 
  new file mode 100644 
  index 000..5ce234f 
  --- /dev/null 
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg 
 
  
 Why is this file needed?
 
Here we are testing default value conversion. That is:
if there is no apci/pae/apic specified in xm config, after conversion,
libvirt xml should include:
features
apic/
acpi/
pae/
/features

So, from xm config - xml, the cfg file should be like this.
  
  @@ -0,0 +1,23 @@ 
  +name = XenGuest2 
  +uuid = c7a5fdb2-cdaf-9455-926a-d65c16db1809
  +maxmem = 579 
  +memory = 394 
  +vcpus = 1 
  +builder = hvm 
  +kernel = /usr/lib/xen/boot/hvmloader 
  +boot = d 
  +hpet = 1 
  +localtime = 0 
  +on_poweroff = destroy 
  +on_reboot = restart 
  +on_crash = restart 
  +device_model = /usr/lib/xen/bin/qemu-dm 
  +sdl = 0 
  +vnc = 1 
  +vncunused = 1 
  +vnclisten = 127.0.0.1 
  +vncpasswd = 123poi 
  +vif = [  
 mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem 
 u ] 
  +parallel = none 
  +serial = none 
  +disk = [ phy:/dev/HostVG/XenGuest2,hda,w,  
 file:/root/boot.iso,hdc:cdrom,r ] 
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out  
 b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out 
  new file mode 100644 
  index 000..97a9827 
  --- /dev/null 
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out 
 
  
 IMO, this file should be renamed to 'test-fullvirt-default-feature.cfg'.

And from xml - xm config, if in xml there is:
features
apic/
acpi/
pae/
/features
Then after conversion, in xm config out file there will be explicitly:
acpi=1
apic=1
pae=1

So, thlis is a little different from test-fullvirt-default-feature.cfg.

  
  @@ -0,0 +1,26 @@ 
  +name = XenGuest2 
  +uuid = c7a5fdb2-cdaf-9455-926a-d65c16db1809 
  +maxmem = 579 
  +memory = 394 
  +vcpus = 1 
  +builder = hvm 
  +kernel = /usr/lib/xen/boot/hvmloader 
  +boot = d 
  +pae = 1 
  +acpi = 1 
  +apic = 1 
  +hpet = 1 
  +localtime = 0 
  +on_poweroff = destroy 
  +on_reboot = restart 
  +on_crash = restart 
  +device_model = /usr/lib/xen/bin/qemu-dm 
  +sdl = 0 
  +vnc = 1 
  +vncunused = 1 
  +vnclisten = 127.0.0.1 
  +vncpasswd = 123poi 
  +vif = [  
 mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem 
 u ] 
  +parallel = none 
  +serial = none 
  +disk = [ phy:/dev/HostVG/XenGuest2,hda,w,  
 file:/root/boot.iso,hdc:cdrom,r ] 
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.xml  
 b/tests/xmconfigdata/test-fullvirt-default-feature.xml 
  new file mode 100644 
  index 000..57a6531 
  --- /dev/null 
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.xml 
  @@ -0,0 +1,48 @@ 
  +domain type='xen' 
  +  nameXenGuest2/name 
  +  uuidc7a5fdb2-cdaf-9455-926a-d65c16db1809/uuid 
  +  memory unit='KiB'592896/memory 
  +  currentMemory unit='KiB'403456/currentMemory 
  +  vcpu placement='static'1/vcpu 
  +  os 
  +type arch='i686' machine='xenfv'hvm/type 
  +loader type='rom'/usr/lib/xen/boot/hvmloader/loader 
  +boot dev='cdrom'/ 
  +  /os 
  +  features 
  +acpi/ 
  +apic/ 
  +pae/ 
  +  /features 
  +  clock offset='utc' adjustment='reset' 
  +timer name='hpet' present='yes'/ 
  +  /clock 
  +  on_poweroffdestroy/on_poweroff 
  +  on_rebootrestart/on_reboot 
  +  on_crashrestart/on_crash 
  +  devices 
  +emulator/usr/lib/xen/bin/qemu-dm/emulator 
  +disk type='block' device='disk' 
  +  driver name='phy'/ 
  +  source dev='/dev/HostVG/XenGuest2'/ 
  +  target dev='hda' bus='ide'/ 
  +/disk 
  +disk type='file' device='cdrom' 
  +  driver name='file'/ 
  +  source file='/root/boot.iso'/ 
  +  target dev='hdc' bus='ide'/ 
  +  readonly/ 
  +/disk 
  +interface type='bridge' 
  +  mac address='00:16:3e:66:92:9c'/ 
  +  source 

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/22/2014 at 08:17 PM, in message 54980bf2.1060...@redhat.com, John
Ferlan jfer...@redhat.com wrote: 

  
 On 12/21/2014 10:15 PM, Chun Yan Liu wrote: 
   
   
  On 12/19/2014 at 08:03 PM, in message 54941429.8000...@redhat.com, 
  John 
  Ferlan jfer...@redhat.com wrote:  
   

  On 12/19/2014 12:31 AM, Chun Yan Liu wrote:  
  
  
  On 12/18/2014 at 01:00 PM, in message  
  5492d008026600086...@soto.provo.novell.com, Chun Yan Liu  
  cy...@suse.com wrote:  
  
  
  On 12/17/2014 at 06:52 PM, in message 
  20141217105227.gq136...@orkuz.home,  
  Jiri Denemark jdene...@redhat.com wrote:  
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote:  
  Add public API virDomainSendSysrq for sending SysRequest key.  
  
  Signed-off-by: Chunyan Liu cy...@suse.com  
  ---  
  changes:  
 * add 'flags' to the new API  
 * change parameter from 'const char *key' to 'char key'  
 * change version number from 1.2.11 to 1.2.12  
  
include/libvirt/libvirt-domain.h |  3 +++  
src/driver-hypervisor.h  |  4   
src/libvirt-domain.c | 39  
  +++  
src/libvirt_public.syms  |  5 +  
4 files changed, 51 insertions(+)  
  
  diff --git a/include/libvirt/libvirt-domain.h  
  b/include/libvirt/libvirt-domain.h  
  index baef32d..5f72850 100644  
  --- a/include/libvirt/libvirt-domain.h  
  +++ b/include/libvirt/libvirt-domain.h  
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom,  
   virDomainFSInfoPtr **info,  
   unsigned int flags);  
  
  +/* virDomainSendSysrq */  
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int 
  flags);  
  +  
  
  I think quite a few reviewers (Daniel, Eric, and I) agreed on using an  
  enum instead of char so that the API is more general.  
  
  Sorry, I missed this part. I'll update. One left question:  
  How about 'virsh sysrq' parameters? What would we expect users to pass?  
  
  Any thoughts on that?  
libxl_send_sysrq  

  Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what   
  you had in mind for syntax previously - although it looks like:  

   virsh sysrq domain [key]  
   
  Thanks for reply. The syntax I'm used previously is:  
  #virsh sysrq domain key 
  key is required. It's just a letter, like 'h', 'c', etc. About which  
 options can we 
  have, on can refer to the results on guest through sysrq help. (that is,  
 issue 
  'virsh sysrq domain h' and look at guest kernel message. I think on each  
 guest, 
  there must be 'h' option, it will print help message.) 
  
 h, c, etc. doesn't tell me enough about what to expect from the 
 perspective of this naive user...  Passing 'h' via virsh to a driver 
 to return some help string that gets displayed where?

Guest kernel message if guest is Linux. xen/libxl just passes the key
blindly to guest kernel, so to pass 'h' to guest kernel, it just like one issue:
#echo 'h'  /proc/sysrq-trigger
in a Linux guest, you will see in /var/log/messages:

SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)
 memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)
 sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)
 nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)
 unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)
 show-blocked-tasks(w) dump-ftrace-buffer(z)

  Was there a 
 mechanism I missed to return and display that output? Do you have sample 
 output to show on a system with these changes applied? 

I don't know how if any other hypervisor behaves differently, for xen/libxl,
they just send sysrq key to guest blindly if I understand correctly. So, which 
letter
is corresponding to which option is all the same with guest sysrq key 
definition,
in other words, it depends on guest sysrq key definition.

  
   

  Where if not provided key would be NULL, which doesn't look good for how   
  the code reads now.  
   
  As said above, key is required, it couldn't be NULL, otherwise, it will  
 report error. 
   
  
 While the check in virsh because VSH_OFLAG_REQ is set for key is good, 
 what if someone calls the API directly? You have no check there for 
 key being non null - it just gets passed along. 
  
  The description for key in virDomainSendSysrq is   
  still not sufficient to help me either:  

  + * @key:SysRq key, like h, c, ...  

  What does 'h', 'c', ... mean?  What are the options? What do they map to   
  functionality wise?  I assume it's hypervisor dependent, but that's all   
  stuff you need to describe somewhere. I don't want to guess or go   
  searching for the answer through numerous search engine hits.  
   
  I can add more description on how one could get those options, but the way 
  I think is through 'sysrq help' and check guest message. 
   

  Looking at the enum Jirka proposed:  

  typedef enum

Re: [libvirt] [PATCH 2/2] Add tests to xmconfigtest

[Chun Yan Liu]

 On 12/23/2014 at 11:13 AM, in message 5498ddcc.2020...@suse.com, Jim 
 Fehlig
jfeh...@suse.com wrote: 
 Chun Yan Liu wrote: 
 
  On 12/23/2014 at 09:42 AM, in message 5498c888.6000...@suse.com, Jim 
  Fehlig 
   
  jfeh...@suse.com wrote:  
 
  Chunyan Liu wrote:  

  Hi Chunyan,  

  Thanks for the fix, and the test!  But I have a few questions regarding  
  the latter...  

   
  Add tests to testing HVM default features (pae, acpi, apic)  
  conversion from xm config to libvirt xml and from libvirt  
  xml to xm config.  
  
  Signed-off-by: Chunyan Liu cy...@suse.com  
  ---  
   .../xmconfigdata/test-fullvirt-default-feature.cfg | 23 +++  
   .../test-fullvirt-default-feature.cfg.out  | 26   
   .../xmconfigdata/test-fullvirt-default-feature.xml | 48   
 
  ++  
   
   tests/xmconfigtest.c   |  9 +++-  
   4 files changed, 105 insertions(+), 1 deletion(-)  
   create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg  
   create mode 100644  
 tests/xmconfigdata/test-fullvirt-default-feature.cfg.out  
   create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.xml  
  
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg   
 
  b/tests/xmconfigdata/test-fullvirt-default-feature.cfg  
   
  new file mode 100644  
  index 000..5ce234f  
  --- /dev/null  
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg  
  
 

  Why is this file needed? 
   

  Here we are testing default value conversion. That is: 
  if there is no apci/pae/apic specified in xm config, after conversion, 
  libvirt xml should include: 
  features 
  apic/ 
  acpi/ 
  pae/ 
  /features 
 
  
 Ah, ok.  Agreed that this test is missing. 
  
  So, from xm config - xml, the cfg file should be like this. 
 

   
  @@ -0,0 +1,23 @@  
  +name = XenGuest2  
  +uuid = c7a5fdb2-cdaf-9455-926a-d65c16db1809 
  +maxmem = 579  
  +memory = 394  
  +vcpus = 1  
  +builder = hvm  
  +kernel = /usr/lib/xen/boot/hvmloader  
  +boot = d  
  +hpet = 1  
  +localtime = 0  
  +on_poweroff = destroy  
  +on_reboot = restart  
  +on_crash = restart  
  +device_model = /usr/lib/xen/bin/qemu-dm  
  +sdl = 0  
  +vnc = 1  
  +vncunused = 1  
  +vnclisten = 127.0.0.1  
  +vncpasswd = 123poi  
  +vif = [   
 
   
 mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem  
  u ]  
   
  +parallel = none  
  +serial = none  
  +disk = [ phy:/dev/HostVG/XenGuest2,hda,w,   
 
  file:/root/boot.iso,hdc:cdrom,r ]  
   
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out   
 
  b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out  
   
  new file mode 100644  
  index 000..97a9827  
  --- /dev/null  
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out  
  
 

  IMO, this file should be renamed to 'test-fullvirt-default-feature.cfg'. 
   
  
  And from xml - xm config, if in xml there is: 
  features 
  apic/ 
  acpi/ 
  pae/ 
  /features 
  Then after conversion, in xm config out file there will be explicitly: 
  acpi=1 
  apic=1 
  pae=1 
  
  So, thlis is a little different from test-fullvirt-default-feature.cfg. 
 
  
 This is actually tested in all of the other test-fullvirt-* tests.  I 
 don't think we need an explicit test for it. 
  
 

   
  @@ -0,0 +1,26 @@  
  +name = XenGuest2  
  +uuid = c7a5fdb2-cdaf-9455-926a-d65c16db1809  
  +maxmem = 579  
  +memory = 394  
  +vcpus = 1  
  +builder = hvm  
  +kernel = /usr/lib/xen/boot/hvmloader  
  +boot = d  
  +pae = 1  
  +acpi = 1  
  +apic = 1  
  +hpet = 1  
  +localtime = 0  
  +on_poweroff = destroy  
  +on_reboot = restart  
  +on_crash = restart  
  +device_model = /usr/lib/xen/bin/qemu-dm  
  +sdl = 0  
  +vnc = 1  
  +vncunused = 1  
  +vnclisten = 127.0.0.1  
  +vncpasswd = 123poi  
  +vif = [   
 
   
 mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem  
  u ]  
   
  +parallel = none  
  +serial = none  
  +disk = [ phy:/dev/HostVG/XenGuest2,hda,w,   
 
  file:/root/boot.iso,hdc:cdrom,r ]  
   
  diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.xml   
 
  b/tests/xmconfigdata/test-fullvirt-default-feature.xml  
   
  new file mode 100644  
  index 000..57a6531  
  --- /dev/null  
  +++ b/tests/xmconfigdata/test-fullvirt-default-feature.xml  
  @@ -0,0 +1,48 @@  
  +domain type='xen'  
  +  nameXenGuest2/name  
  +  uuidc7a5fdb2-cdaf-9455-926a-d65c16db1809/uuid  
  +  memory unit='KiB'592896/memory  
  +  currentMemory unit='KiB'403456/currentMemory  
  +  vcpu placement='static'1/vcpu  
  +  os  
  +type arch='i686' machine='xenfv'hvm/type  
  +loader type='rom'/usr/lib/xen/boot/hvmloader/loader  
  +boot dev='cdrom'/  
  +  /os

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/23/2014 at 11:42 AM, in message 5498e4ba.1000...@redhat.com, John
Ferlan jfer...@redhat.com wrote: 

  
 On 12/22/2014 09:55 PM, Chun Yan Liu wrote: 
   
   
  On 12/22/2014 at 08:17 PM, in message 54980bf2.1060...@redhat.com, 
  John 
  Ferlan jfer...@redhat.com wrote:  
   

  On 12/21/2014 10:15 PM, Chun Yan Liu wrote:  


  On 12/19/2014 at 08:03 PM, in message 54941429.8000...@redhat.com, 
  John  
  Ferlan jfer...@redhat.com wrote:   

 
  On 12/19/2014 12:31 AM, Chun Yan Liu wrote:   
  
  
  On 12/18/2014 at 01:00 PM, in message   
  5492d008026600086...@soto.provo.novell.com, Chun Yan Liu   
  cy...@suse.com wrote:   
  
  
  On 12/17/2014 at 06:52 PM, in message 
  20141217105227.gq136...@orkuz.home,   
  Jiri Denemark jdene...@redhat.com wrote:   
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote:   
  Add public API virDomainSendSysrq for sending SysRequest key.   
  
  Signed-off-by: Chunyan Liu cy...@suse.com   
  ---   
  changes:   
 * add 'flags' to the new API   
 * change parameter from 'const char *key' to 'char key'   
 * change version number from 1.2.11 to 1.2.12   
  
include/libvirt/libvirt-domain.h |  3 +++   
src/driver-hypervisor.h  |  4    
src/libvirt-domain.c | 39   
  +++   
src/libvirt_public.syms  |  5 +   
4 files changed, 51 insertions(+)   
  
  diff --git a/include/libvirt/libvirt-domain.h   
  b/include/libvirt/libvirt-domain.h   
  index baef32d..5f72850 100644   
  --- a/include/libvirt/libvirt-domain.h   
  +++ b/include/libvirt/libvirt-domain.h   
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom,   
   virDomainFSInfoPtr **info,   
   unsigned int flags);   
  
  +/* virDomainSendSysrq */   
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int 
  flags);   
  +   
  
  I think quite a few reviewers (Daniel, Eric, and I) agreed on using 
  an   
  enum instead of char so that the API is more general.   
  
  Sorry, I missed this part. I'll update. One left question:   
  How about 'virsh sysrq' parameters? What would we expect users to 
  pass?   
  
  Any thoughts on that?   
libxl_send_sysrq   
 
  Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what   
   
  you had in mind for syntax previously - although it looks like:   
 
   virsh sysrq domain [key]   

  Thanks for reply. The syntax I'm used previously is:   
  #virsh sysrq domain key  
  key is required. It's just a letter, like 'h', 'c', etc. About which   
  options can we  
  have, on can refer to the results on guest through sysrq help. (that is,  
   
  issue  
  'virsh sysrq domain h' and look at guest kernel message. I think on each  
   
  guest,  
  there must be 'h' option, it will print help message.)  

  h, c, etc. doesn't tell me enough about what to expect from the  
  perspective of this naive user...  Passing 'h' via virsh to a driver  
  to return some help string that gets displayed where? 
   
  Guest kernel message if guest is Linux. xen/libxl just passes the key 
  blindly to guest kernel, so to pass 'h' to guest kernel, it just like one  
 issue: 
  #echo 'h'  /proc/sysrq-trigger 
  in a Linux guest, you will see in /var/log/messages: 
   
  SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e) 
   memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j) 
   sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m) 
   nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q) 
   unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V) 
   show-blocked-tasks(w) dump-ftrace-buffer(z) 
   
  
 FWIW: My point on this was - by using 'virsh sysrq domain h' you don't 
 provide a mechanism to display this output. 
  
 Seems just from the descriptions some of those letters might return some 
 useful information... Some aren't one way commands.  Perhaps it would be 
 useful to capture output and be able to return it... 

Right. But might be hard.
And I think of a problem when mapping enum to letter:
to different guests (e.g. Linux vs Windows), the letter to enum mapping might
be different, even hypervisor may not precisely know that. At least for xen
hypervisor, I think it only blindly sends the key to guest, but has no idea of
different key-letter meaning to different guests.

- Chunyan

  
 John 
   Was there a  
  mechanism I missed to return and display that output? Do you have sample  
  output to show on a system with these changes applied?  
   
  I don't know how if any other hypervisor behaves differently, for  
 xen/libxl, 
  they just send sysrq key to guest blindly if I understand correctly. So,  
 which letter 
  is corresponding to which option is all the same with guest sysrq key  
 definition, 
  in other words, it depends on guest sysrq key definition

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/19/2014 at 08:03 PM, in message 54941429.8000...@redhat.com, John
Ferlan jfer...@redhat.com wrote: 

  
 On 12/19/2014 12:31 AM, Chun Yan Liu wrote: 
  
  
  On 12/18/2014 at 01:00 PM, in message 
  5492d008026600086...@soto.provo.novell.com, Chun Yan Liu 
  cy...@suse.com wrote: 
  
  
  On 12/17/2014 at 06:52 PM, in message 
  20141217105227.gq136...@orkuz.home, 
  Jiri Denemark jdene...@redhat.com wrote: 
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote: 
  Add public API virDomainSendSysrq for sending SysRequest key. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
  changes: 
 * add 'flags' to the new API 
 * change parameter from 'const char *key' to 'char key' 
 * change version number from 1.2.11 to 1.2.12 
  
include/libvirt/libvirt-domain.h |  3 +++ 
src/driver-hypervisor.h  |  4  
src/libvirt-domain.c | 39 
  +++ 
src/libvirt_public.syms  |  5 + 
4 files changed, 51 insertions(+) 
  
  diff --git a/include/libvirt/libvirt-domain.h 
  b/include/libvirt/libvirt-domain.h 
  index baef32d..5f72850 100644 
  --- a/include/libvirt/libvirt-domain.h 
  +++ b/include/libvirt/libvirt-domain.h 
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom, 
   virDomainFSInfoPtr **info, 
   unsigned int flags); 
  
  +/* virDomainSendSysrq */ 
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int flags); 
  + 
  
  I think quite a few reviewers (Daniel, Eric, and I) agreed on using an 
  enum instead of char so that the API is more general. 
  
  Sorry, I missed this part. I'll update. One left question: 
  How about 'virsh sysrq' parameters? What would we expect users to pass? 
  
  Any thoughts on that? 
libxl_send_sysrq 
  
 Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what  
 you had in mind for syntax previously - although it looks like: 
  
  virsh sysrq domain [key] 

Thanks for reply. The syntax I'm used previously is: 
#virsh sysrq domain key
key is required. It's just a letter, like 'h', 'c', etc. About which options 
can we
have, on can refer to the results on guest through sysrq help. (that is, issue
'virsh sysrq domain h' and look at guest kernel message. I think on each guest,
there must be 'h' option, it will print help message.)

  
 Where if not provided key would be NULL, which doesn't look good for how  
 the code reads now. 

As said above, key is required, it couldn't be NULL, otherwise, it will report 
error.

 The description for key in virDomainSendSysrq is  
 still not sufficient to help me either: 
  
 + * @key:SysRq key, like h, c, ... 
  
 What does 'h', 'c', ... mean?  What are the options? What do they map to  
 functionality wise?  I assume it's hypervisor dependent, but that's all  
 stuff you need to describe somewhere. I don't want to guess or go  
 searching for the answer through numerous search engine hits. 

I can add more description on how one could get those options, but the way
I think is through 'sysrq help' and check guest message.

  
 Looking at the enum Jirka proposed: 
  
 typedef enum { 
  VIR_DOMAIN_SYSRQ_REBOOT, 
  VIR_DOMAIN_SYSRQ_CRASH, 
  VIR_DOMAIN_SYSRQ_OOM_KILL, 
  VIR_DOMAIN_SYSRQ_SYNC, 
  ... 
 } virDomainSysrqCommand; 
  
 It seems REBOOT would/could be the default. So if key wasn't provided  
 the incoming key would be 0 (zero)... If you didn't want a default,  
 then you'd have to force a style to be chosen. You're defining the API  
 so you show us how you want to handle that. Eventually, each hypervisor  
 would map that enum into a character. That is, you'll end up with a way  
 to map the enum to a letter for the types of sysrq's each hypervisor  
 could support. If a hypervisor doesn't support a specific type of sysrq,  
 then decide how to handle. 
  
 Anyway given the above enum list, I would think the virsh would be: 
  
  virsh sysrq domain reboot 
  virsh sysrq domain crash 
  virsh sysrq domain kill 
  virsh sysrq domain sync 
  ... 

OK. That's what I'm concerned and why I hesitated to change API parameter
from 'char key' to 'enum'. Personally I don't think this is a better user
interface and has risk to miss some functionality, since we don't know
which options those hypervisors can support.

I still prefer: 
#virsh sysrq domain key_letter
One can first issue 'virsh sysrq domain h', and check guest kernel message
for all sysrq options. Then send option as he need.
And as a result, I still think I don't see benefit of changing the API parameter
from 'char key' to 'enum'.

How do you think?

Chunyan

 And key goes from optional to required unless you want to allow 'virsh  
 sysrq domain' to mean reboot by default (e.g., if not provided the  
 default is to reboot). 
  
 The string for key would be passed to the virDomainSendSysrq which would  
 then convert or map

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/21/2014 at 10:34 PM, in message 5496da81.40...@redhat.com, Peter 
 Krempa
pkre...@redhat.com wrote: 
 On 12/19/14 13:03, John Ferlan wrote: 
   
   
  On 12/19/2014 12:31 AM, Chun Yan Liu wrote: 
  
  
  On 12/18/2014 at 01:00 PM, in message 
  5492d008026600086...@soto.provo.novell.com, Chun Yan Liu 
  cy...@suse.com wrote: 
  
  
  On 12/17/2014 at 06:52 PM, in message 
  20141217105227.gq136...@orkuz.home, 
  Jiri Denemark jdene...@redhat.com wrote: 
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote: 
  Add public API virDomainSendSysrq for sending SysRequest key. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
  changes: 
 * add 'flags' to the new API 
 * change parameter from 'const char *key' to 'char key' 
 * change version number from 1.2.11 to 1.2.12 
  
include/libvirt/libvirt-domain.h |  3 +++ 
src/driver-hypervisor.h  |  4  
src/libvirt-domain.c | 39 
  +++ 
src/libvirt_public.syms  |  5 + 
4 files changed, 51 insertions(+) 
  
  diff --git a/include/libvirt/libvirt-domain.h 
  b/include/libvirt/libvirt-domain.h 
  index baef32d..5f72850 100644 
  --- a/include/libvirt/libvirt-domain.h 
  +++ b/include/libvirt/libvirt-domain.h 
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom, 
   virDomainFSInfoPtr **info, 
   unsigned int flags); 
  
  +/* virDomainSendSysrq */ 
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int 
  flags); 
  + 
  
  I think quite a few reviewers (Daniel, Eric, and I) agreed on using an 
  enum instead of char so that the API is more general. 
  
  Sorry, I missed this part. I'll update. One left question: 
  How about 'virsh sysrq' parameters? What would we expect users to pass? 
  
  Any thoughts on that? 
libxl_send_sysrq 
   
  Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what 
  you had in mind for syntax previously - although it looks like: 
   
  virsh sysrq domain [key] 
   
  Where if not provided key would be NULL, which doesn't look good for how 
  the code reads now. The description for key in virDomainSendSysrq is 
  still not sufficient to help me either: 
   
  + * @key:SysRq key, like h, c, ... 
   
  What does 'h', 'c', ... mean?  What are the options? What do they map to 
  functionality wise?  I assume it's hypervisor dependent, but that's all 
  stuff you need to describe somewhere. I don't want to guess or go 
  searching for the answer through numerous search engine hits. 
   
  Looking at the enum Jirka proposed: 
   
  typedef enum { 
  VIR_DOMAIN_SYSRQ_REBOOT, 
  VIR_DOMAIN_SYSRQ_CRASH, 
  VIR_DOMAIN_SYSRQ_OOM_KILL, 
  VIR_DOMAIN_SYSRQ_SYNC, 
  ... 
  } virDomainSysrqCommand; 
   
  It seems REBOOT would/could be the default. So if key wasn't provided 
  the incoming key would be 0 (zero)... If you didn't want a default, 
  then you'd have to force a style to be chosen. You're defining the API 
  so you show us how you want to handle that. Eventually, each hypervisor 
  would map that enum into a character. That is, you'll end up with a way 
  to map the enum to a letter for the types of sysrq's each hypervisor 
  could support. If a hypervisor doesn't support a specific type of sysrq, 
  then decide how to handle. 
   
  Anyway given the above enum list, I would think the virsh would be: 
   
  virsh sysrq domain reboot 
  virsh sysrq domain crash 
  virsh sysrq domain kill 
  virsh sysrq domain sync 
  ... 
   
  And key goes from optional to required unless you want to allow 'virsh 
  sysrq domain' to mean reboot by default (e.g., if not provided the 
  default is to reboot). 
   
  
 This still can be implemented using the existing API for sending general 
 keystrokes to the guest. I still don't see a reason to add a new API as 
 a special case of an existing one. 

First version is implemented by using .domainSendKey but objected. See:
https://www.redhat.com/archives/libvir-list/2014-December/msg00480.html

Thanks.

  
 Peter 
  
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/18/2014 at 01:00 PM, in message
5492d008026600086...@soto.provo.novell.com, Chun Yan Liu
cy...@suse.com wrote: 

  
 On 12/17/2014 at 06:52 PM, in message 
 20141217105227.gq136...@orkuz.home, 
 Jiri Denemark jdene...@redhat.com wrote:  
  On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote:  
   Add public API virDomainSendSysrq for sending SysRequest key.  
 
   Signed-off-by: Chunyan Liu cy...@suse.com  
   ---  
   changes:  
 * add 'flags' to the new API  
 * change parameter from 'const char *key' to 'char key'  
 * change version number from 1.2.11 to 1.2.12  
 
include/libvirt/libvirt-domain.h |  3 +++  
src/driver-hypervisor.h  |  4   
src/libvirt-domain.c | 39   
  +++  
src/libvirt_public.syms  |  5 +  
4 files changed, 51 insertions(+)  
 
   diff --git a/include/libvirt/libvirt-domain.h   
  b/include/libvirt/libvirt-domain.h  
   index baef32d..5f72850 100644  
   --- a/include/libvirt/libvirt-domain.h  
   +++ b/include/libvirt/libvirt-domain.h  
   @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom,  
   virDomainFSInfoPtr **info,  
   unsigned int flags);  
  
   +/* virDomainSendSysrq */  
   +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int flags);  
   +  

  I think quite a few reviewers (Daniel, Eric, and I) agreed on using an  
  enum instead of char so that the API is more general.  
  
 Sorry, I missed this part. I'll update. One left question: 
 How about 'virsh sysrq' parameters? What would we expect users to pass?

Any thoughts on that?
 
  

  Jirka  


  
  
  
 -- 
 libvir-list mailing list 
 libvir-list@redhat.com 
 https://www.redhat.com/mailman/listinfo/libvir-list 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 3/5] virsh: add 'sysrq' command

[Chun Yan Liu]

 On 12/17/2014 at 08:14 PM, in message 549173ac.2000...@redhat.com, Ján
Tomkojto...@redhat.com wrote: 
 On 12/17/2014 09:48 AM, Chunyan Liu wrote: 
  All domainSendSysrq related API should be manageable from 
  the virsh command line. So, expose 'virsh sysrq' command. 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
   tools/virsh-domain.c | 54  
  
   1 file changed, 54 insertions(+) 
   
  
  + 
  +static bool 
  +cmdSysrq(vshControl *ctl, const vshCmd *cmd) 
  +{ 
  +virDomainPtr dom; 
  +bool ret = false; 
  +const char *key = NULL; 
  + 
  +if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) 
  +return false; 
  + 
  +if (vshCommandOptStringReq(ctl, cmd, key, key)  0) 
  +return false; 
  
 dom needs to be freed. 
  
  + 
  +if (!(virDomainSendSysrq(dom, key[0], 0)  0)) 
  +ret = true; 
  + 
  
 Just a nitpick: I find our usual template more readable: 
 if (vir...()  0) 
 goto cleanup; 
  
 ret = true 
  cleanup: 

Thanks, I'll update them.

  
 Jan 
  
  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/17/2014 at 06:52 PM, in message 20141217105227.gq136...@orkuz.home,
Jiri Denemark jdene...@redhat.com wrote: 
 On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote: 
  Add public API virDomainSendSysrq for sending SysRequest key. 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
  changes: 
* add 'flags' to the new API 
* change parameter from 'const char *key' to 'char key' 
* change version number from 1.2.11 to 1.2.12 
   
   include/libvirt/libvirt-domain.h |  3 +++ 
   src/driver-hypervisor.h  |  4  
   src/libvirt-domain.c | 39  
 +++ 
   src/libvirt_public.syms  |  5 + 
   4 files changed, 51 insertions(+) 
   
  diff --git a/include/libvirt/libvirt-domain.h  
 b/include/libvirt/libvirt-domain.h 
  index baef32d..5f72850 100644 
  --- a/include/libvirt/libvirt-domain.h 
  +++ b/include/libvirt/libvirt-domain.h 
  @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom, 
  virDomainFSInfoPtr **info, 
  unsigned int flags); 

  +/* virDomainSendSysrq */ 
  +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int flags); 
  + 
  
 I think quite a few reviewers (Daniel, Eric, and I) agreed on using an 
 enum instead of char so that the API is more general. 

Sorry, I missed this part. I'll update. One left question:
* How about 'virsh sysrq' parameters? What would we expect users to pass?

  
 Jirka 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/15/2014 at 11:27 AM, in message
548ec58d026600084...@soto.provo.novell.com, Chun Yan Liu
cy...@suse.com wrote: 

  
 On 12/12/2014 at 05:35 PM, in message 
 20141212093545.gd136...@orkuz.home, 
 Jiri Denemark jdene...@redhat.com wrote:  
  On Fri, Dec 12, 2014 at 10:18:36 +0100, Peter Krempa wrote:  
   On 12/12/14 10:04, Chunyan Liu wrote:  
Add public API virDomainSendSysrq for sending SysRequest key.  
  
Signed-off-by: Chunyan Liu cy...@suse.com  
---  
 include/libvirt/libvirt-domain.h |  3 +++  
 src/driver-hypervisor.h  |  4   
 src/libvirt-domain.c | 38   
  ++  
 src/libvirt_public.syms  |  1 +  
 4 files changed, 46 insertions(+)  
  
 
   [...]  
 
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c  
index cb76d8c..4658fd7 100644  
--- a/src/libvirt-domain.c  
+++ b/src/libvirt-domain.c  
@@ -11192,3 +11192,41 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)  
 VIR_FREE(info-devAlias[i]);  
 VIR_FREE(info-devAlias);  
 }  
+  
+  
+/**  
+ * virDomainSendSysrq:  
+ * @domain:pointer to domain object, or NULL for Domain0  
+ * @key:SysRq key, like h, c, ...  
+ *  
+ * Send SysRq key to the guest.  
+ *  
+ * Returns 0 in case of success, -1 in case of failure.  
+ */  
+int  
+virDomainSendSysrq(virDomainPtr domain, const char *key)  
 
   The new API should definitely have a 'flags' argument although it may be  
   unused for now.  

  Moreover, passing a single character by reference sounds pretty strange,  
  why not just char key? 
  
 I tried to define as 'char key', but meet some trouble in remote protocol, 
 +struct remote_domain_send_sysrq_args { 
 +remote_nonnull_domain dom; 
 +char key; 
 +}; 
 will report 'char key' as unsupported type. 

Any ideas here?

With 'char key' in remote_protocol.x, it'll report:
unhandled type for argument value: char key; at ./rpc/gendispatch.pl line 
1234.
I didn't find a way to avoid that, that's why I turned to use 'char *key' (a 
string, and
when calling libxl API, pass key[0]).


 
  Moreover, wouldn't it be better to provide an  
  enum of possible values with meaningful names (rather than keys), such  
  as  

  typedef enum {  
  VIR_DOMAIN_SYSRQ_REBOOT,  
  VIR_DOMAIN_SYSRQ_CRASH,  
  VIR_DOMAIN_SYSRQ_OOM_KILL,  
  VIR_DOMAIN_SYSRQ_SYNC,  
  ...  
  } virDomainSysrqCommand;  

  This way, virDomainSendSysrq(dom, VIR_DOMAIN_SYSRQ_OOM_KILL, 0) would be  
  pretty self-explaining. The prototype would have to change to  
  
 Mainly because xen/libxl API both accept letter as parameter directly, so  
 define 
 enum will need to map enum to letter before calling xen/libxl API. 
  
 Chunyan 
  

  int virDomainSendSysrq(virDomainPtr domain,  
 int command, /* one of virDomainSysrqCommand */  
 unsigned int flags);  

  Jirka  


  
  
  
 -- 
 libvir-list mailing list 
 libvir-list@redhat.com 
 https://www.redhat.com/mailman/listinfo/libvir-list 
  
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 2/5] implement remote protocol for domainSendSysrq

[Chun Yan Liu]

 On 12/12/2014 at 05:20 PM, in message 548ab36e.30...@redhat.com, Peter 
 Krempa
pkre...@redhat.com wrote: 
 On 12/12/14 10:04, Chunyan Liu wrote: 
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
   src/remote/remote_driver.c   |  2 +- 
   src/remote/remote_protocol.x | 13 - 
   2 files changed, 13 insertions(+), 2 deletions(-) 
   
  diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c 
  index 999f16d..97ea64b 100644 
  --- a/src/remote/remote_driver.c 
  +++ b/src/remote/remote_driver.c 
  @@ -7941,7 +7941,6 @@ remoteDomainGetFSInfo(virDomainPtr dom, 
   return rv; 
   } 

  - 
  
 Spurious whitespace change. 
  
   /* get_nonnull_domain and get_nonnull_network turn an on-wire 
* (name, uuid) pair into virDomainPtr or virNetworkPtr object. 
* These can return NULL if underlying memory allocations fail, 
  @@ -8285,6 +8284,7 @@ static virHypervisorDriver hypervisor_driver = { 
   .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8  
 */ 
   .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */ 
   .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */ 
  +.domainSendSysrq = remoteDomainSendSysrq, /* 1.2.11 */ 
  
 We are now in freeze for 1.2.11, and thus won't take new features. This 
 needs to change to 1.2.12. 

Thanks, will update.

  
   }; 

   static virNetworkDriver network_driver = { 
  diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x 
  index cbd3ec7..d988182 100644 
  --- a/src/remote/remote_protocol.x 
  +++ b/src/remote/remote_protocol.x 
  @@ -1084,6 +1084,11 @@ struct remote_domain_send_key_args { 
   unsigned int flags; 
   }; 

  +struct remote_domain_send_sysrq_args { 
  +remote_nonnull_domain dom; 
  +remote_nonnull_string key; 
  
 Flags argument needs to be added 

Thanks, will update.

  
  +}; 
  + 
   struct remote_domain_send_process_signal_args { 
   remote_nonnull_domain dom; 
   hyper pid_value; 
  @@ -5550,5 +,11 @@ enum remote_procedure { 
* @generate: none 
* @acl: domain:fs_freeze 
*/ 
  -REMOTE_PROC_DOMAIN_GET_FSINFO = 349 
  +REMOTE_PROC_DOMAIN_GET_FSINFO = 349, 
  + 
  +/** 
  + * @generate: both 
  + * @acl: domain:send_input 
  + */ 
  +REMOTE_PROC_DOMAIN_SEND_SYSRQ = 350 
   }; 
   
  
 Missing change to remote_protocol-structs. Did you run syntax-check? 

Yes. And I tested the command through 'virsh -c xen+ssh:// to a remote
host', then 'sysrq', it works. It still needs to change remote_protocol-strcuts?

  
 Peter 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 1/5] Add public API virDomainSendSysrq

[Chun Yan Liu]

 On 12/12/2014 at 05:35 PM, in message 20141212093545.gd136...@orkuz.home,
Jiri Denemark jdene...@redhat.com wrote: 
 On Fri, Dec 12, 2014 at 10:18:36 +0100, Peter Krempa wrote: 
  On 12/12/14 10:04, Chunyan Liu wrote: 
   Add public API virDomainSendSysrq for sending SysRequest key. 

   Signed-off-by: Chunyan Liu cy...@suse.com 
   --- 
include/libvirt/libvirt-domain.h |  3 +++ 
src/driver-hypervisor.h  |  4  
src/libvirt-domain.c | 38  
 ++ 
src/libvirt_public.syms  |  1 + 
4 files changed, 46 insertions(+) 

   
  [...] 
   
   diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c 
   index cb76d8c..4658fd7 100644 
   --- a/src/libvirt-domain.c 
   +++ b/src/libvirt-domain.c 
   @@ -11192,3 +11192,41 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) 
VIR_FREE(info-devAlias[i]); 
VIR_FREE(info-devAlias); 
} 
   + 
   + 
   +/** 
   + * virDomainSendSysrq: 
   + * @domain:pointer to domain object, or NULL for Domain0 
   + * @key:SysRq key, like h, c, ... 
   + * 
   + * Send SysRq key to the guest. 
   + * 
   + * Returns 0 in case of success, -1 in case of failure. 
   + */ 
   +int 
   +virDomainSendSysrq(virDomainPtr domain, const char *key) 
   
  The new API should definitely have a 'flags' argument although it may be 
  unused for now. 
  
 Moreover, passing a single character by reference sounds pretty strange, 
 why not just char key?

I tried to define as 'char key', but meet some trouble in remote protocol,
+struct remote_domain_send_sysrq_args {
+remote_nonnull_domain dom;
+char key;
+};
will report 'char key' as unsupported type.

 Moreover, wouldn't it be better to provide an 
 enum of possible values with meaningful names (rather than keys), such 
 as 
  
 typedef enum { 
 VIR_DOMAIN_SYSRQ_REBOOT, 
 VIR_DOMAIN_SYSRQ_CRASH, 
 VIR_DOMAIN_SYSRQ_OOM_KILL, 
 VIR_DOMAIN_SYSRQ_SYNC, 
 ... 
 } virDomainSysrqCommand; 
  
 This way, virDomainSendSysrq(dom, VIR_DOMAIN_SYSRQ_OOM_KILL, 0) would be 
 pretty self-explaining. The prototype would have to change to 

Mainly because xen/libxl API both accept letter as parameter directly, so define
enum will need to map enum to letter before calling xen/libxl API.

Chunyan

  
 int virDomainSendSysrq(virDomainPtr domain, 
int command, /* one of virDomainSysrqCommand */ 
unsigned int flags); 
  
 Jirka 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 2/5] implement remote protocol for domainSendSysrq

[Chun Yan Liu]

 On 12/12/2014 at 09:38 PM, in message 20141212133825.gj32...@redhat.com,
Daniel P. Berrange berra...@redhat.com wrote: 
 On Fri, Dec 12, 2014 at 01:24:57PM +0100, Peter Krempa wrote: 
  On 12/12/14 12:49, John Ferlan wrote: 


   On 12/12/2014 04:04 AM, Chunyan Liu wrote: 
   Signed-off-by: Chunyan Liu cy...@suse.com 
   --- 
src/remote/remote_driver.c   |  2 +- 
src/remote/remote_protocol.x | 13 - 
2 files changed, 13 insertions(+), 2 deletions(-) 
   
   
struct remote_domain_send_process_signal_args { 
remote_nonnull_domain dom; 
hyper pid_value; 
   @@ -5550,5 +,11 @@ enum remote_procedure { 
 * @generate: none 
 * @acl: domain:fs_freeze 
 */ 
   -REMOTE_PROC_DOMAIN_GET_FSINFO = 349 
   +REMOTE_PROC_DOMAIN_GET_FSINFO = 349, 
   + 
   +/** 
   + * @generate: both 
   + * @acl: domain:send_input 
   + */ 

   Just send_input?  The result of the send is essentially 'init_control' 
   right? Like a shutdown. Perhaps even like destroy (eg, 'stop'). Or 
   'shutdown'... I'm not sure of all the options here, but this seems much 
   more invasive than just sending input because the result of the sent key 
   is a bit more final. 
   
  Since you are able to do the same thing with the virDomainSendKey API 
  which has the same ACL class: 
   
  virsh send-key dom KEY_LEFTALT KEY_SYSRQ KEY_O 
   
  I don't think it should require any other permission since it's just a 
  keystroke basically. 
  
 Agreed, 'send_input' basically gives away the keys to the kingdom, 
 so there's nothing to gain by having a separate permission for 
 this new API 
  
 As a general rule we should always seek to reuse existing permissions 
 because we don't want to end up having one permission for each separate 
 API 

Got it. Thanks.

  
 Regards, 
 Daniel 
 --  
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/  
 :| 
 |: http://libvirt.org  -o- http://virt-manager.org  
 :| 
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/  
 :| 
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc  
 :| 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] libxl: add .domainSendKey

[Chun Yan Liu]

 On 12/10/2014 at 05:21 PM, in message 20141210092125.gb6...@redhat.com,
Daniel P. Berrange berra...@redhat.com wrote: 
 On Tue, Dec 09, 2014 at 11:19:22PM -0700, Chun Yan Liu wrote: 
   
   
   On 12/9/2014 at 05:44 PM, in message 
   2014120909.gb29...@redhat.com, 
  Daniel P. Berrange berra...@redhat.com wrote:  
   On Tue, Dec 09, 2014 at 11:27:46AM +0800, Chunyan Liu wrote:  
libxl supports sysrq. Add .domainSendKey function to support  
sending sysrq key.  
 
   I think this is really bending the semantics of the virDomainSendKey  
   API too much.  This API is documented to inject *any* scancodes into  
   the guest operating system. Implementing it in such a way that you  
   can only send sysrq key sequences to the guest kernel is not what  
   applications will expect when they try to use this API. So NACK to  
   this patch  
 
   I'm open to the idea of adding an explicit API for triggering the  
   sysrq sequence though. eg virDomainSendSysRequest or something  
   like that, if you really want access to sysrq via the API. 
   
  Libxl now has no ability to send any key sequence to guest kernel 
  but supports sending sysrq key. We just want a way to send sysrq key, 
  so adding new  virDomainSendSysRequest API is OK to me. 
   
  Meanwhile, that means: 
  Adding .domainSendSysRequest to virHypervisorDriver? 
  And in virsh, add a new 'virsh sysrq' command or update code of 
  cmdSendKey to handle sysrq key sequence? (like if .domainSendKey is 
  not supported by driver, check if it is sysrq key sequence, if yes, try 
  virDomainSysRequest if driver supports that.) 
  
 Yes pretty much, but I wouldn't do anything with domainSendKey. Apps can 
 choose to try to use domainSendKey if domainSysRequest isn't supported. 

OK. I'll update.

  
 Regards, 
 Daniel 
 --  
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/  
 :| 
 |: http://libvirt.org  -o- http://virt-manager.org  
 :| 
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/  
 :| 
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc  
 :| 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [virt-tools-list] [PATCH] virtinst: refresh pools status before fetch_pools

[Chun Yan Liu]

 On 12/5/2014 at 09:54 PM, in message 5481b907.4040...@redhat.com, Cole
Robinson crobi...@redhat.com wrote: 
 On 12/05/2014 03:40 AM, Chunyan Liu wrote: 
  Currently, when connecting to hypervisor, if there are pools active 
  but in fact target path already deleted (or for other reasons the 
  pool is not working), libvirtd not refresh status yet, fetch_pools 
  will fail, that will cause connecting to hypervisor process 
  reporting error and exit. The whole connection work failed. 
  
  With the patch, always refresh pool status before fetch pools. Let 
  the libvirtd pool status reflect the reality, avoid the non-synced 
  status affects the hypervisor connection. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
virtinst/pollhelpers.py | 13 + 
1 file changed, 13 insertions(+) 
  
  diff --git a/virtinst/pollhelpers.py b/virtinst/pollhelpers.py 
  index a9b1527..e8702f0 100644 
  --- a/virtinst/pollhelpers.py 
  +++ b/virtinst/pollhelpers.py 
  @@ -133,6 +133,19 @@ def fetch_pools(backend, origmap, build_func): 
  
if backend.check_support( 
backend.SUPPORT_CONN_LISTALLSTORAGEPOOLS) and not  
 _force_old_poll: 
  + 
  +# Refresh pools before poll_helper. For those 
  +# 'active' but target path not exist (or other reasons 
  +# causing the pool not working), but libvirtd not 
  +# refresh the status, this will make it refreshed 
  +# and mark that pool as 'inactive'. 
  +objs = backend.listAllStoragePools() 
  +for obj in objs: 
  +try: 
  +obj.refresh(0) 
  +except Exception, e: 
  +pass 
  + 
return _new_poll_helper(origmap, name, 
backend.listAllStoragePools, build_func) 
else: 
  
  
 This is a very heavy hammer, refresh is a potentially long running operation  
  
 so this could cause decent slowdown in some scenarios. 
  
 IMO this is essentially a libvirt bug, for pools with target directories  
 (dir,  
 fs, netfs), libvirt should be periodically checking the directory ctime and  
 doing the pool refresh for us. And if the target has disappeared, it shuts  
 down the pool (like shutting down a VM if it crashes). 

Hi, libvirt list,

I'm not sure if Cole's suggestion could be done in libvirt, so just forward
the mail to libvirt mailing list. Any opinions?

Chunyan
  
 We have so many hacks sprinkled around in virtinst/virt-manager dealing with  
 the fallout of this lacking libvirt feature. Really wish I had implemented  
 it  
 years ago. But I'd rather focus on that then adding yet more hacks. 
  
 If there's a specific issue you're hitting that's manifesting itself  
 elsewhere  
 in the app, let us know and maybe there's a way to mitigate it in the  
 interim 
  
 - Cole 
  
 ___ 
 virt-tools-list mailing list 
 virt-tools-l...@redhat.com 
 https://www.redhat.com/mailman/listinfo/virt-tools-list 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] libxl: add .domainSendKey

[Chun Yan Liu]

 On 12/9/2014 at 05:44 PM, in message 2014120909.gb29...@redhat.com,
Daniel P. Berrange berra...@redhat.com wrote: 
 On Tue, Dec 09, 2014 at 11:27:46AM +0800, Chunyan Liu wrote: 
  libxl supports sysrq. Add .domainSendKey function to support 
  sending sysrq key. 
  
 I think this is really bending the semantics of the virDomainSendKey 
 API too much.  This API is documented to inject *any* scancodes into 
 the guest operating system. Implementing it in such a way that you 
 can only send sysrq key sequences to the guest kernel is not what 
 applications will expect when they try to use this API. So NACK to 
 this patch 
  
 I'm open to the idea of adding an explicit API for triggering the 
 sysrq sequence though. eg virDomainSendSysRequest or something 
 like that, if you really want access to sysrq via the API.

Libxl now has no ability to send any key sequence to guest kernel
but supports sending sysrq key. We just want a way to send sysrq key,
so adding new  virDomainSendSysRequest API is OK to me.

Meanwhile, that means:
Adding .domainSendSysRequest to virHypervisorDriver?
And in virsh, add a new 'virsh sysrq' command or update code of
cmdSendKey to handle sysrq key sequence? (like if .domainSendKey is
not supported by driver, check if it is sysrq key sequence, if yes, try
virDomainSysRequest if driver supports that.)

Thanks,
Chunyan

  
  
 Regards, 
 Daniel 
 --  
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/  
 :| 
 |: http://libvirt.org  -o- http://virt-manager.org  
 :| 
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/  
 :| 
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc  
 :| 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

 On 9/2/2014 at 04:54 PM, in message 20140902085434.ga21...@redhat.com,
Daniel P. Berrange berra...@redhat.com wrote: 
 On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote: 
  To use virtio-serial device, unix socket created for chardev with 
  default umask(022) has insufficient permissions. 
   
  e.g. start kvm guest with: 
  -device virtio-serial \ 
  -chardev socket,path=/tmp/foo,server,nowait,id=foo \ 
  -device virtserialport,chardev=foo,name=org.fedoraproject.port.0 
   
  Check permissions for the socket file that has been created in the host 
  to enable communication through virtual serial ports in the guest: 
  #ls -l /tmp/somefile.sock 
  srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock 
   
  Other users in the qemu group (like real user, test engines, etc) cannot 
  write to this socket. 
   
  Problem reported here: 
  https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11 
  https://bugzilla.novell.com/show_bug.cgi?id=888166 
   
  This patch tries to add a 'umask' option to 'chardev', so that user 
  can have chance to indicate a umask overwritting the default one (default 
  is 022), then create unix sockets with expected permissions. 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
  This is patch for qemu. 
   
   qemu-char.c |  3 +++ 
   qemu-options.hx |  9 +++-- 
   util/qemu-sockets.c | 12 +++- 
   3 files changed, 21 insertions(+), 3 deletions(-) 
  
  diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c 
  index 5d38395..facf2c6 100644 
  --- a/util/qemu-sockets.c 
  +++ b/util/qemu-sockets.c 
  @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
   { 
   struct sockaddr_un un; 
   const char *path = qemu_opt_get(opts, path); 
  -int sock, fd; 
  +int newmask = qemu_opt_get_number(opts, umask, 0); 
  +int sock, fd, oldmask; 

   sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); 
   if (sock  0) { 
  @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
   } 

   unlink(un.sun_path); 
  +if (newmask) { 
  +oldmask = umask(newmask); 
  +} 
   if (bind(sock, (struct sockaddr*) un, sizeof(un))  0) { 
  +if (newmask) { 
  +umask(oldmask); 
  +} 
   error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED); 
   goto err; 
   } 
  +if (newmask) { 
  +umask(oldmask); 
  +} 
  
 Setting  umask() is not thread-safe as it affects the entire process. While 
 this is OK for chardevs which are cold-plugged at startup, once QEMU is 
 running it is not OK to alter umask during hotplug of devices. 
  
 Wouldn't it be simpler for libvirt to simply set the umask to 002 when it 
 first launches QEMU, avoiding the need for trying todo this per device. 

I think that's OK. Only one thing: I'm not sure if permissions of any other
file created in qemu will be changed due to this change, and if that is
unexpected or not.
 
  
 Regards, 
 Daniel 
 --  
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/  
 :| 
 |: http://libvirt.org  -o- http://virt-manager.org  
 :| 
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/  
 :| 
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc  
 :| 
  
  
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

 On 9/2/2014 at 05:16 PM, in message 20140902091655.gb21...@redhat.com,
Daniel P. Berrange berra...@redhat.com wrote: 
 On Tue, Sep 02, 2014 at 03:08:51AM -0600, Chun Yan Liu wrote: 
   
   
   On 9/2/2014 at 04:54 PM, in message 
   20140902085434.ga21...@redhat.com, 
  Daniel P. Berrange berra...@redhat.com wrote:  
   On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote:  
To use virtio-serial device, unix socket created for chardev with  
default umask(022) has insufficient permissions.  
  
e.g. start kvm guest with:  
-device virtio-serial \  
-chardev socket,path=/tmp/foo,server,nowait,id=foo \  
-device virtserialport,chardev=foo,name=org.fedoraproject.port.0  
  
Check permissions for the socket file that has been created in the host 
 
to enable communication through virtual serial ports in the guest:  
#ls -l /tmp/somefile.sock  
srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock  
  
Other users in the qemu group (like real user, test engines, etc) 
cannot  
write to this socket.  
  
Problem reported here:  
https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11  
https://bugzilla.novell.com/show_bug.cgi?id=888166  
  
This patch tries to add a 'umask' option to 'chardev', so that user  
can have chance to indicate a umask overwritting the default one 
(default  
  
is 022), then create unix sockets with expected permissions.  
  
Signed-off-by: Chunyan Liu cy...@suse.com  
---  
This is patch for qemu.  
  
 qemu-char.c |  3 +++  
 qemu-options.hx |  9 +++--  
 util/qemu-sockets.c | 12 +++-  
 3 files changed, 21 insertions(+), 3 deletions(-)  
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c  
index 5d38395..facf2c6 100644  
--- a/util/qemu-sockets.c  
+++ b/util/qemu-sockets.c  
@@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)  
 {  
 struct sockaddr_un un;  
 const char *path = qemu_opt_get(opts, path);  
-int sock, fd;  
+int newmask = qemu_opt_get_number(opts, umask, 0);  
+int sock, fd, oldmask;  
   
 sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);  
 if (sock  0) {  
@@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error 
**errp)  
 }  
   
 unlink(un.sun_path);  
+if (newmask) {  
+oldmask = umask(newmask);  
+}  
 if (bind(sock, (struct sockaddr*) un, sizeof(un))  0) {  
+if (newmask) {  
+umask(oldmask);  
+}  
 error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);  
 goto err;  
 }  
+if (newmask) {  
+umask(oldmask);  
+}  
 
   Setting  umask() is not thread-safe as it affects the entire process.  
 While  
   this is OK for chardevs which are cold-plugged at startup, once QEMU is  
   running it is not OK to alter umask during hotplug of devices.  
 
   Wouldn't it be simpler for libvirt to simply set the umask to 002 when it 

   first launches QEMU, avoiding the need for trying todo this per device.  
   
  I think that's OK. Only one thing: I'm not sure if permissions of any other 
  file created in qemu will be changed due to this change, and if that is 
  unexpected or not. 
  
 Whether or not it causes problems today is only half the story. I'm more 
 concerned about the long term problems. The use of threads is increasing 
 in QEMU over time, so manipulating umask() is a time-bomb waiting to strike 
 at some point in the future when people have forgotten that this proposed 
 feature exists. IMHO umask() should simply never be used when multiple 
 threads are running, to avoid this long term risk entirely. 

I agree. Now suppose when it explicitly sets the mode S_IWGRP while creating
new file, group user writing permission is really expected, I think setting 
umask
to 002 to the whole qemu process should be OK.

  
 Regards, 
 Daniel 
 --  
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/  
 :| 
 |: http://libvirt.org  -o- http://virt-manager.org  
 :| 
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/  
 :| 
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc  
 :| 
  
  
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cmdMigrate: move vshConnect before vshWatchJob

[Chun Yan Liu]

 On 8/8/2014 at 04:44 PM, in message
1407487476-28077-1-git-send-email-cy...@suse.com, Chunyan Liu
cy...@suse.com wrote: 
 A possible fix to issue: 
 http://www.redhat.com/archives/libvir-list/2014-August/thread.html#00227 
  
 While doing migration on KVM host, found problem sometimes: 
 VM is already running on the target host and disappears from source 
 host, but 'virsh migrate' command line hangs, cannot exit normally. 
 If pressing ENTER key, it will exit. 
  
 The code hangs at tools/virsh-domain.c: cmdMigrate 
 -vshWatchJob-poll(): 
 poll() is trying to select pipe_fd, which is used to receive message 
 from doMigrate thread. In debugging, found that doMigrate finishes 
 and at the end it does call safewrite() to write the retval ('0' or 
 '1') to pipe_fd, and the write is completed. But cmdMigrate poll() 
 cannot get the event. If pressing ENTER key, poll() can get the 
 event and select pipe_fd, then command line can exit. 
  
 In current code, authentication thread which is called by vshConnect 
 will use stdin, and at the same time, in cmdMigrate main process, 
 poll() is listening to stdin, that probably affect poll() to get 
 pipe_fd event. Better to move authentication before vshWatchJob. With 
 this change, above problem does not exist.

Any comments about this patch?

  
 Signed-off-by: Chunyan Liu cy...@suse.com 
 --- 
  tools/virsh-domain.c | 26 -- 
  tools/virsh.h|  1 + 
  2 files changed, 21 insertions(+), 6 deletions(-) 
  
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c 
 index f7193cb..2562326 100644 
 --- a/tools/virsh-domain.c 
 +++ b/tools/virsh-domain.c 
 @@ -8965,6 +8965,7 @@ doMigrate(void *opaque) 
  virTypedParameterPtr params = NULL; 
  int nparams = 0; 
  int maxparams = 0; 
 +virConnectPtr dconn = data-dconn; 
   
  sigemptyset(sigmask); 
  sigaddset(sigmask, SIGINT); 
 @@ -9079,18 +9080,12 @@ doMigrate(void *opaque) 
  ret = '0'; 
  } else { 
  /* For traditional live migration, connect to the destination host  
 directly. */ 
 -virConnectPtr dconn = NULL; 
  virDomainPtr ddom = NULL; 
   
 -dconn = vshConnect(ctl, desturi, false); 
 -if (!dconn) 
 -goto out; 
 - 
  if ((ddom = virDomainMigrate3(dom, dconn, params, nparams, flags)))  
 { 
  virDomainFree(ddom); 
  ret = '0'; 
  } 
 -virConnectClose(dconn); 
  } 
   
   out: 
 @@ -9152,6 +9147,23 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd) 
  data.cmd = cmd; 
  data.writefd = p[1]; 
   
 +if (vshCommandOptBool(cmd, p2p) || vshCommandOptBool(cmd, direct))  
 { 
 +data.dconn = NULL; 
 +} else { 
 +/* For traditional live migration, connect to the destination host.  
 */ 
 +virConnectPtr dconn = NULL; 
 +const char *desturi = NULL; 
 + 
 +if (vshCommandOptStringReq(ctl, cmd, desturi, desturi)  0) 
 +goto cleanup; 
 + 
 +dconn = vshConnect(ctl, desturi, false); 
 +if (!dconn) 
 +goto cleanup; 
 + 
 +data.dconn = dconn; 
 +} 
 + 
  if (virThreadCreate(workerThread, 
  true, 
  doMigrate, 
 @@ -9163,6 +9175,8 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd) 
  virThreadJoin(workerThread); 
   
   cleanup: 
 +if (data.dconn) 
 +virConnectClose(data.dconn); 
  virDomainFree(dom); 
  VIR_FORCE_CLOSE(p[0]); 
  VIR_FORCE_CLOSE(p[1]); 
 diff --git a/tools/virsh.h b/tools/virsh.h 
 index 7656407..b4df24b 100644 
 --- a/tools/virsh.h 
 +++ b/tools/virsh.h 
 @@ -371,6 +371,7 @@ struct _vshCtrlData { 
  vshControl *ctl; 
  const vshCmd *cmd; 
  int writefd; 
 +virConnectPtr dconn; 
  }; 
   
  /* error handling */ 
 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] virsh migrate: sometimes command line cannot exit until manually press ENTER key

[Chun Yan Liu]

 On 8/8/2014 at 12:18 AM, in message
CAAfHSJvP1P20kBFD9ZgYy=0JVvG5oiS0eVCfGZc+k39=eha...@mail.gmail.com,
Shivaprasad bhat shivaprasadb...@gmail.com wrote: 
 On Thu, Aug 7, 2014 at 11:02 AM, Chun Yan Liu cy...@suse.com wrote: 
  Hi, List, 
  
  Do you meet the same problem? Host is kvm, doing: (e.g.) 
  #virsh migrate sles11 qemu+ssh://147.2.207.55/system --live --unsafe 
  password: 
  Sometimes, VM is already running on the target host and disappears 
  from source host, but the command line still hangs there, if pressing 
  ENTER, it will exit. 
  
  I debugged the code, but found the result is weird. The code hangs at 
  tools/virsh-domain.c - cmdMigrate -vshWatchJob-poll(): 
  poll() is trying to select pipe_fd, which is used to receive message from 
  doMigrate thread. In debugging, found that doMigrate finishes and at 
  the end it  does call safewrite() to write the retval ('0' or '1') to  
 pipe_fd, 
  and the write is completed. But cmdMigrate poll() cannot get the event. 
  Then after pressing ENTER key, poll() can get the event and select 
  pipe_fd, then command line can exit. 
  
  Any ideas about the possible reason? 
  
 It is because, the authentication thread is independently waiting for the 
 password on virsh. I see that the virWatchJob interferes with authentication 
 thread and the password is never taken successfully. The virWatchJob 
 probably needs to wait till the authentication thread completes taking the 
 password after which it should start watching the job.

Agree. Although in my testing password can be taken successfully, and I
don't know how to explain why pressing ENTER then poll() can get
the pipe_fd event, I do think both authentication thread and main process
uses stdin might have some bad affect.

I'd like to adjust the code a little to move the authentication before poll(). 

Thanks!
 Moreover, I see 
 that --p2p 
 migration doesnt work at all when auto login is not configured. 
  
  
  Thanks, 
  Chunyan 
  
  
  -- 
  libvir-list mailing list 
  libvir-list@redhat.com 
  https://www.redhat.com/mailman/listinfo/libvir-list 
  
 Thanks, 
 Shiva 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] virsh migrate: sometimes command line cannot exit until manually press ENTER key

[Chun Yan Liu]

Hi, List,

Do you meet the same problem? Host is kvm, doing: (e.g.)
#virsh migrate sles11 qemu+ssh://147.2.207.55/system --live --unsafe
password:
Sometimes, VM is already running on the target host and disappears
from source host, but the command line still hangs there, if pressing
ENTER, it will exit.

I debugged the code, but found the result is weird. The code hangs at
tools/virsh-domain.c - cmdMigrate -vshWatchJob-poll():
poll() is trying to select pipe_fd, which is used to receive message from
doMigrate thread. In debugging, found that doMigrate finishes and at
the end it  does call safewrite() to write the retval ('0' or '1') to pipe_fd,
and the write is completed. But cmdMigrate poll() cannot get the event.
Then after pressing ENTER key, poll() can get the event and select
pipe_fd, then command line can exit.

Any ideas about the possible reason?

Thanks,
Chunyan


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

 On 7/31/2014 at 05:52 PM, in message 53DA11DB.FE6 : 102 : 21807, Chun Yan 
 Liu
wrote: 

  
 On 7/31/2014 at 11:35 AM, in message 53d9b993.4040...@redhat.com, Eric 
 Blake 
 ebl...@redhat.com wrote:  
  On 07/30/2014 09:29 PM, Chun Yan Liu wrote:  
   
  A better idea would be to rely on the volume lease manager - obtaining a  
   
  lease should be impossible for an image already in use (and should even   
  cover the case of copying 'base - active' onto 'base', which your   
  equality test wouldn't catch). I'm not sure why the lease manager is not  
   
  already flagging this issue - are we still using the nop lease manager   
  by default, and would the fcntl or sanlock lease manager do a better job? 
   

  Besides the default lock is 'nop', currently lock manager is only used in: 
   
  VM start/stop and attach/detach disk, blockcopy not using it.  
   
 But that's not true - the code IS trying to use it.  qemuDomainBlockCopy  
 calls qemuDomainPrepareDiskChainElement(), which calls  
 virDomainLockImageAttach(), and that should be the use of the lock  
 manager.  Can you debug why it is not working when using something other  
 than the 'nop' manager?  
  
 Update: 
 1. identical device with exactly the same spelled name. e.g. source is  
 /dev/sda4, dest is /dev/sda4: 
 * 'lockd' manager is working well with --reuse-external. 
 * my previous testing result which makes the VM cannot be started  
successfully after doing blockcopy to same device and shutdown
VM and start VM again, because I didn't add --reuse-external
option. Then in code, it  considers the dest as a new created file,
 when error happens, it unlinks the dest, which s actually 
also my source disk. 
 2. identical device with not exactly the same spelling. e.g. source is  
 /dev/sda4, dest is /dev///sda4, or a softlink: 
 'lockd' manager is not working. Both hashtable check and fcntl can't  
  give right result. 
 * HashTable checks the resource in the locked list depending on the  
name. If name is not exactly the same, it will treat as 'not found in  
locked list'. 
 * In fcntl stage, since fcntl is to one process, in the same process, do  
two times fcntl F_SETLK, both will succeed. Since everytime it's
virtlockd tries fcntl, so in this case, 1st time fcntl /dev/sda4, it
succeeds; 2nd time fcntl /dev///sda4, also succeeds. 
Not as we expected 'cannot get the lock'. 

About the 2nd point, 'lockd' manager cannot handle identical device but
different name (extra / in name or softlink), I didn't think of a quick fix.
Maybe still better to check src and dst name in earlier stage. Could call
realpath() to cover more cases. Any other ideas?

   
  To use it in blockcopy, maybe can refer to attach/detach disk: before 
  doing  
  blockcopy, try AcquireResource; after blockcopy finish, try   
 releaseResource.  
   
 That should be what is already happening.  
   
   
 --   
 Eric Blake   eblake redhat com+1-919-301-3266  
 Libvirt virtualization library http://libvirt.org  
   
   
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

 On 7/31/2014 at 11:35 AM, in message 53d9b993.4040...@redhat.com, Eric 
 Blake
ebl...@redhat.com wrote: 
 On 07/30/2014 09:29 PM, Chun Yan Liu wrote: 
  
  A better idea would be to rely on the volume lease manager - obtaining a  
  lease should be impossible for an image already in use (and should even  
  cover the case of copying 'base - active' onto 'base', which your  
  equality test wouldn't catch). I'm not sure why the lease manager is not  
  already flagging this issue - are we still using the nop lease manager  
  by default, and would the fcntl or sanlock lease manager do a better job? 
   
  Besides the default lock is 'nop', currently lock manager is only used in: 
  VM start/stop and attach/detach disk, blockcopy not using it. 
  
 But that's not true - the code IS trying to use it.  qemuDomainBlockCopy 
 calls qemuDomainPrepareDiskChainElement(), which calls 
 virDomainLockImageAttach(), and that should be the use of the lock 
 manager.  Can you debug why it is not working when using something other 
 than the 'nop' manager? 

Update:
1. identical device with exactly the same spelled name. e.g. source is 
/dev/sda4,
dest is /dev/sda4:
* 'lockd' manager is working well with --reuse-external.
* my previous testing result which makes the VM cannot be started 
successfully
   after doing blockcopy to same device and shutdown VM and start VM again,
   because I didn't add --reuse-external option. Then in code, it considers 
the dest
   as a new created file, when error happens, it unlinks the dest, which is 
actually
   also my source disk.
2. identical device with not exactly the same spelling. e.g. source is 
/dev/sda4,
dest is /dev///sda4, or a softlink:
'lockd' manager is not working. Both hashtable check and fcntl can't give 
right result.
* HashTable checks the resource in the locked list depending on the name.
   If name is not exactly the same, it will treat as 'not found in locked 
list'.
* In fcntl stage, since fcntl is to one process, in the same process, do 
two times fcntl
   F_SETLK, both will succeed. Since everytime it's virtlockd tries fcntl, 
so in this case,
   1st time fcntl /dev/sda4, it succeeds; 2nd time fcntl /dev///sda4, also 
succeeds.
   Not as we expected 'cannot get the lock'.

  
  To use it in blockcopy, maybe can refer to attach/detach disk: before doing 
  blockcopy, try AcquireResource; after blockcopy finish, try  
 releaseResource. 
  
 That should be what is already happening. 
  
  
 --  
 Eric Blake   eblake redhat com+1-919-301-3266 
 Libvirt virtualization library http://libvirt.org 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

 On 7/29/2014 at 11:47 PM, in message 53d7c1f6.4030...@redhat.com, Eric 
 Blake
ebl...@redhat.com wrote: 
 On 07/29/2014 07:37 AM, Eric Blake wrote: 
  On 07/29/2014 03:59 AM, Chunyan Liu wrote: 
  Check whether dst is the same device as source, if yes, report 
  error and exit. 
  
  Currently if dst is the same device as source, blockcopy is still 
  going and qemu 'drive-mirror' is executed. Considering that: 
  a). blockcopy to the same device is meaningless. b.) result is 
  unexpected. (tested with block device whose source path is /dev/sdaX, 
  after blockcopy, shutdown VM and then create VM from xml again, the 
  VM cannot be started.) This case should not be allowed. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
   src/qemu/qemu_driver.c | 7 +++ 
   1 file changed, 7 insertions(+) 
  
  diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c 
  index 704ba39..87a3790 100644 
  --- a/src/qemu/qemu_driver.c 
  +++ b/src/qemu/qemu_driver.c 
  @@ -15309,6 +15309,13 @@ qemuDomainBlockCopy(virDomainObjPtr vm, 
   } 

   /* Prepare the destination file.  */ 
  +if (STREQ(disk-src-path, dest)) { 
   
  STREQ is too weak (consider symlinks, or even a/b vs. a//b).  It 
  will catch some cases, but not all. 
   
  I don't know that we can reliably detect all possible ways the user can 
  shoot themselves in the foot, so I'm not sure this patch is a good idea. 
  
 A better idea would be to rely on the volume lease manager - obtaining a 
 lease should be impossible for an image already in use (and should even 
 cover the case of copying 'base - active' onto 'base', which your 
 equality test wouldn't catch). I'm not sure why the lease manager is not 
 already flagging this issue - are we still using the nop lease manager 
 by default, and would the fcntl or sanlock lease manager do a better job? 

According to code, it's still using 'nop' by default.
Code:
if (!(qemu_driver-lockManager =
  virLockManagerPluginNew(cfg-lockManagerName ?
  cfg-lockManagerName : nop,
  qemu,
  cfg-configBaseDir,
  0)))
goto error;

Saw Daniel's virlockd patch series, the last patch:
Change the default QEMU lock manager to 'lockd' is ACKed. But this patch is
not committed. All other patches in that series have been committed.
http://www.redhat.com/archives/libvir-list/2012-December/msg00734.html

  
 --  
 Eric Blake   eblake redhat com+1-919-301-3266 
 Libvirt virtualization library http://libvirt.org 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

 On 7/29/2014 at 11:47 PM, in message 53d7c1f6.4030...@redhat.com, Eric 
 Blake
ebl...@redhat.com wrote: 
 On 07/29/2014 07:37 AM, Eric Blake wrote: 
  On 07/29/2014 03:59 AM, Chunyan Liu wrote: 
  Check whether dst is the same device as source, if yes, report 
  error and exit. 
  
  Currently if dst is the same device as source, blockcopy is still 
  going and qemu 'drive-mirror' is executed. Considering that: 
  a). blockcopy to the same device is meaningless. b.) result is 
  unexpected. (tested with block device whose source path is /dev/sdaX, 
  after blockcopy, shutdown VM and then create VM from xml again, the 
  VM cannot be started.) This case should not be allowed. 
  
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
   src/qemu/qemu_driver.c | 7 +++ 
   1 file changed, 7 insertions(+) 
  
  diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c 
  index 704ba39..87a3790 100644 
  --- a/src/qemu/qemu_driver.c 
  +++ b/src/qemu/qemu_driver.c 
  @@ -15309,6 +15309,13 @@ qemuDomainBlockCopy(virDomainObjPtr vm, 
   } 

   /* Prepare the destination file.  */ 
  +if (STREQ(disk-src-path, dest)) { 
   
  STREQ is too weak (consider symlinks, or even a/b vs. a//b).  It 
  will catch some cases, but not all. 
   
  I don't know that we can reliably detect all possible ways the user can 
  shoot themselves in the foot, so I'm not sure this patch is a good idea. 
  
 A better idea would be to rely on the volume lease manager - obtaining a 
 lease should be impossible for an image already in use (and should even 
 cover the case of copying 'base - active' onto 'base', which your 
 equality test wouldn't catch). I'm not sure why the lease manager is not 
 already flagging this issue - are we still using the nop lease manager 
 by default, and would the fcntl or sanlock lease manager do a better job?

Besides the default lock is 'nop', currently lock manager is only used in:
VM start/stop and attach/detach disk, blockcopy not using it.
To use it in blockcopy, maybe can refer to attach/detach disk: before doing
blockcopy, try AcquireResource; after blockcopy finish, try releaseResource.
Anyway, using lock manager is a better idea in such cases.

  
 --  
 Eric Blake   eblake redhat com+1-919-301-3266 
 Libvirt virtualization library http://libvirt.org 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] blockcopy: check dst = identical device

[Chun Yan Liu]

 On 7/31/2014 at 11:35 AM, in message 53d9b993.4040...@redhat.com, Eric 
 Blake
ebl...@redhat.com wrote: 
 On 07/30/2014 09:29 PM, Chun Yan Liu wrote: 
  
  A better idea would be to rely on the volume lease manager - obtaining a  
  lease should be impossible for an image already in use (and should even  
  cover the case of copying 'base - active' onto 'base', which your  
  equality test wouldn't catch). I'm not sure why the lease manager is not  
  already flagging this issue - are we still using the nop lease manager  
  by default, and would the fcntl or sanlock lease manager do a better job? 
   
  Besides the default lock is 'nop', currently lock manager is only used in: 
  VM start/stop and attach/detach disk, blockcopy not using it. 
  
 But that's not true - the code IS trying to use it.  qemuDomainBlockCopy 
 calls qemuDomainPrepareDiskChainElement(), which calls 
 virDomainLockImageAttach(), and that should be the use of the lock 
 manager.  Can you debug why it is not working when using something other 
 than the 'nop' manager? 

Yeah. I'll have a look.

  
  To use it in blockcopy, maybe can refer to attach/detach disk: before doing 
  blockcopy, try AcquireResource; after blockcopy finish, try  
 releaseResource. 
  
 That should be what is already happening. 
  
  
 --  
 Eric Blake   eblake redhat com+1-919-301-3266 
 Libvirt virtualization library http://libvirt.org 
  
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2] storagevol: add nocow to vol xml

[Chun Yan Liu]

 On 7/9/2014 at 07:25 PM, in message 53bd26a7.9090...@redhat.com, Ján
Tomkojto...@redhat.com wrote: 
 On 07/08/2014 08:47 AM, Chunyan Liu wrote: 
  Add 'nocow' to storage volume xml so that user can have an option 
  to set NOCOW flag to the newly created volume. It's useful on btrfs 
  file system to enhance performance. 
   
  Btrfs has low performance when hosting VM images, even more when the guest 
  in those VM are also using btrfs as file system. One way to mitigate this 
  bad performance is to turn off COW attributes on VM files. Generally, there 
  are two ways to turn off NOCOW on btrfs: a) by mounting fs with nodatacow, 
  
 s/turn off NOCOW/turn off COW/ 
  
  then all newly created files will be NOCOW. b) per file. Add the NOCOW file 
  attribute. It could only be done to empty or new files. 
   
  This patch tries the second way, according to 'nocow' option, it could set 
  NOCOW flag per file: 
  for raw file images, handle 'nocow' in libvirt code; for non-raw file  
 images, 
  pass 'nocow=on' option to qemu-img, and let qemu-img to handle that 
  (requires 
  qemu-img version = 2.1). 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  --- 
  Changes: 
- now qemu-img can handle 'nocow=on' option, just pass 'nocow=on' to 
  qemu-img for non-raw file images. No need to handle all file type 
  in libvirt code. 
   
  Pervious version is here: 
http://www.redhat.com/archives/libvir-list/2013-December/msg01257.html 
   
  --- 
   docs/formatstorage.html.in|  7 +++ 
   docs/schemas/storagevol.rng   |  5 + 
  
 Adding a test case to storagevolxml2argvtest would be nice. Also, by adding 
 the volume XML to storagevolxml2xmlin for this test, it will get validated 
 against the rng schema in 'storagevolschematest'.

Thanks. Will add it.

- Chunyan

  
   src/conf/storage_conf.c   |  3 +++ 
   src/storage/storage_backend.c | 22 ++ 
   src/util/virstoragefile.h |  1 + 
   5 files changed, 38 insertions(+) 
   
  diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in 
  index 1cd82b4..e8862bf 100644 
  --- a/docs/formatstorage.html.in 
  +++ b/docs/formatstorage.html.in 
  @@ -385,6 +385,7 @@ 
   lt;labelgt;virt_image_tlt;/labelgt; 
 lt;/permissionsgt; 
 lt;compatgt;1.1lt;/compatgt; 
  +  lt;nocow/gt; 
 lt;featuresgt; 
   lt;lazy_refcounts/gt; 
 lt;/featuresgt; 
  @@ -424,6 +425,12 @@ 
   1.1 is used. If omitted, qemu-img default is used. 
   span class=sinceSince 1.1.0/span 
 /dd 
  +  dtcodenocow/code/dt 
  +  ddTurn off COW of the newly created volume. So far, this is only  
 valid 
  +to a file image in btrfs file system. It will improve performance  
 when 
  
 s/to a file/for a file/ 
  
  +the file image is used in VM. To create non-raw file images, it 
  +requires QEMU version since 2.1. span class=sinceSince  
 1.2.6/span 
  
 1.2.7 
  
 Otherwise looks good to me. 
  
 Jan 
  
  

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] libxl: support syntax interface type=hostdev

[Chun Yan Liu]

 On 5/10/2014 at 06:18 AM, in message 536d541d.5040...@suse.com, Jim Fehlig
jfeh...@suse.com wrote: 
 Chunyan Liu wrote: 
  Signed-off-by: Chunyan Liu cy...@suse.com 
 
  
 A while back when testing Chunyan's common hostdev library series, I 
 mentioned that interface type='hostdev' was not working with the libxl 
 driver.  Chunyan later privately sent a v1 of this patch for testing 
 in my setup.  In addition to testing, I provided some private comments.  
 I see those have been incorporated in this patch and functionally it 
 looks good, but I do have one additional question about the commit 
 message... 
  
  --- 
   src/libxl/libxl_conf.c | 16 +++- 
   1 file changed, 11 insertions(+), 5 deletions(-) 
  
  diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c 
  index 298c8a1..b7fed7f 100644 
  --- a/src/libxl/libxl_conf.c 
  +++ b/src/libxl/libxl_conf.c 
  @@ -921,25 +921,31 @@ static int 
   libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config) 
   { 
   virDomainNetDefPtr *l_nics = def-nets; 
  -int nnics = def-nnets; 
  +size_t nnics = def-nnets; 
   libxl_device_nic *x_nics; 
  -size_t i; 
  +size_t i, nvnics = 0; 

   if (VIR_ALLOC_N(x_nics, nnics)  0) 
   return -1; 

   for (i = 0; i  nnics; i++) { 
  -if (libxlMakeNic(def, l_nics[i], x_nics[i])) 
  +if (l_nics[i]-type == VIR_DOMAIN_NET_TYPE_HOSTDEV) 
  +continue; 
 
  
 After looking at this again, it seems we are really *fixing* interface 
 type='hostdev'.  The driver already supports creating the hostdev 
 device, but without this patch a libxl_device_nic is created too.  Is 
 that a fair statement?  If so, the commit message should be changed to 
 reflect this.  Thanks! 

A NET_TYPE_HOSTDEV device is really a hostdev device, the driver will create
a hostdev device for it, so no need to create a libxl_device_nic again. Before 
this patch, it tried to call libxlMakeNic to create a libxl_device_nic for it 
but
failed since NET_TYPE_HOSTDEV is not supported there.

I'll add this to commit message. Is that OK?

- Chunyan

  
 Regards, 
 Jim 
  
  + 
  +if (libxlMakeNic(def, l_nics[i], x_nics[nvnics])) 
   goto error; 
   /* 
* The devid (at least right now) will not get initialized by 
* libxl in the setup case but is required for starting the 
* device-model. 
*/ 
  -if (x_nics[i].devid  0) 
  -x_nics[i].devid = i; 
  +if (x_nics[nvnics].devid  0) 
  +x_nics[nvnics].devid = nvnics; 
  + 
  +nvnics++; 
   } 

  +VIR_SHRINK_N(x_nics, nnics, nnics - nvnics); 
   d_config-nics = x_nics; 
   d_config-num_nics = nnics; 

 
  



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v14 00/49] write separate module for hostdev passthrough

[Chun Yan Liu]

The whole patch series could be get from here:
https://github.com/chunyanliu/libvirt/commits/hostdev_passthrough

-Chunyan

 On 3/7/2014 at 06:52 PM, in message
1394189596-22994-1-git-send-email-cy...@suse.com, Chunyan Liu
cy...@suse.com wrote: 
 These patches implements a separate module for hostdev passthrough so that it 
 could be shared by different drivers and can maintain a global state of a  
 host 
 device. 
  
 Patch 1: fix memory leak in virscsi.c 
 Patch 2: improve virHostdevManager to use virOject wrapper and keep  
 reference 
  
 Patches 3~7: switch existing qemu and lxc driver to use virHostdevManager's 
 lists, so that to maintain a global state of every host device. 
  
 Patches 8~45 are to extract general code from qemu_hostdev.c piece by piece, 
 make them reusable common APIs. 
  
 Patches 46: unit test for the virhostdev common library 
 Patches 47: add a hostdev backend type for xen 
 Patches 48: add pci passthrough to libxl driver based on the common library 
 Patches 49: change lxc to use common library APIs 
  
 --- 
 changes to v13: 
   * change virHostdevManager, use virObject and keep reference, solve 'free' 
 problem 
   * add .hostdevMgr to qemu/lxc/libxl driver, get virHostdevManager in 
 StateInitialize for once and keep reference, instead of getting it every 
 time virHostdevManager is used. 
   * fix v13 3/49 qemu-reuse-hostdev-interfaces-to-avoid-duplicate.patch: 
 update qemuPrepareHostUSBDevices parameters to specify hostdevs and 
 nhostdevs, so that could be reused in qemu_hotplug. 
   * rebase left patches 
  
 Chunyan Liu (49): 
   virscsi: fix memory leak 
   virhostdev: use virObject to virHostdevManager to keep reference 
   update qemuPrepareHostUSBDevices parameters to keep consistency 
   qemu: reuse hostdev interfaces to avoid duplicate 
   qemu: remove functions used internally only from qemu_hostdev.h 
   qemu: use general virhostdev lists instead of its own 
   lxc: use general virhostdev lists instead of its own 
   qemu_hostdev: move cfg-relaxedACS as a flag 
   qemu_hostdev: move ColdBoot as a flag 
   qemu_hostdev: move netconfig file location to virhostdev stateDir 
   extract general code from qemuPrepareHostdevPCIDevices 
   rename qemu*NetConfigRestore/Replace to 
 virHostdevNetConfigRestore/Replace 
   rename qemuGet*PciHostDeviceList to virHostdevGet*PciHostDeviceList 
   pass driver name as a parameter to virHostdevPrepareHostdevPCIDevices 
   extract general code from qemuDomainReAttachHostdevDevices 
   pass driver name as a parameter to virHostdevReAttachPCIDevices 
   rename qemuReAttachPciDevice to virHostdevReAttachPciDevice 
   move virHostdevPrepare(ReAttach)PCIDevices to virhostdev.c 
   extract general code from qemuUpdateActivePciHostdevs 
   extract general code from qemuUpdateActiveUsbHostdevs 
   extract general code from qemuUpdateActiveScsiHostdevs 
   pass driver_name as parameter of virHostdevUpdate*Hostdevs functions 
   move virHostdevUpdate* functions to virhostdev.c 
   extract general code from qemuPrepareHostUSBDevices 
   rename qemu*USBDevices to virHostdev*USBDevices 
   pass driver name to virHostdevPrepareUSBDevices 
   move virHostdevPrepareHostUSBDevices to virhostdev.c 
   extract general code from qemuPrepareHostSCSIDevices 
   pass driver name as parameter to virHostdevPrepareSCSIDevices 
   move virHostdevPrepareHostSCSIDevices to virhostdev.c 
   extract general code from qemuDomainReAttachHostUsbDevices 
   pass driver name as paramter to virHostdevReAttachUsbHostdevs 
   move virHostdevReAttachUsbHostdevs to virhostdev.c 
   extract general code from qemuDomainReAttachHostScsiDevices 
   pass driver name as parameter to virHostdevReAttachScciHostdevs 
   move virHostdevReAttachHostScsiDevices to virhostdev.c 
   extract general code of NodeDeviceDetach 
   extract general code of NodeDeviceReAttach 
   extract general code of NodeDeviceReset 
   move virHostdevNodeDevice* to virhostdev.c 
   improve parameter name to let it more meaningful 
   rename some function names to keep consistency 
   improve virHostdevUpdate* parameters to make it more widely used 
   add 3 wrapper functions for prepare/reattach/update domain hostdevs 
   add parameter checks to common interfaces 
   add unit test for new virhostdev common library 
   change lxc_hostdev.c to use virhostdev common library APIs 
   add hostdev pci backend type for xen 
   add pci passthrough to libxl driver 
  
  .gitignore|1 + 
  docs/schemas/domaincommon.rng |1 + 
  src/conf/domain_conf.c|3 +- 
  src/conf/domain_conf.h|1 + 
  src/libvirt_private.syms  |   15 + 
  src/libxl/libxl_conf.c|   63 ++ 
  src/libxl/libxl_conf.h|7 +- 
  src/libxl/libxl_domain.c  |9 + 
  src/libxl/libxl_driver.c  |  454 +++- 
  src/lxc/lxc_conf.h|5 +- 
  src/lxc/lxc_driver.c  |   12 +- 
  src/lxc/lxc_hostdev.c |  307 

Re: [libvirt] [PATCH] Add migration APIs for libxl driver

[Chun Yan Liu]

 I didn't get a chance to test this yet, but have some initial review
 comments.

 Signed-off-by: Chunyan Liu 
 ---
  src/libxl/libxl_driver.c |  617
 ++
  src/libxl/libxl_driver.h |   17 ++-
  2 files changed, 632 insertions(+), 2 deletions(-)

 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index d5fa64a..4037635 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -30,6 +30,12 @@
  #include 
  #include 
  #include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 

  #include internal.h
  #include logging.h
 @@ -60,6 +66,13 @@
  #define XEN_SCHED_CREDIT_NPARAM   2

  static libxlDriverPrivatePtr libxl_driver = NULL;
 +static virThread migrate_receive_thread;


 This prevents receiving concurrent migrations.
Yes. It is a problem. Defined as static is to be used in Begin3 function 
virThreadCreate and in Finish3() function virThreadJoin, but actually the 
thread will exit when receiving data completed or meets error, so virThreadJoin 
doesn't have much effect here. If a call of virThreadJoin is not needed, then 
can specify migrate_receive_thread as a local variable.


 +
 +typedef struct migrate_receive_args {
 +virConnectPtr conn;
 +virDomainObjPtr vm;
 +int sockfd;
 +} migrate_receive_args;


 If there is a future need to extend this structure, will it cause
 incompatibility issues between a source with the extensions and a
 destination without?  Or vise versa?
It will if send logic and receive logic doesn't match. Maybe need to add some 
extra check, but seems no better way to completely avoid that?


  /* Function declarations */
  static int
 @@ -1095,6 +1108,17 @@ libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED)
  return 0;
  }

 +static int
 +libxlSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED, int feature)
 +{
 +switch (feature) {
 +case VIR_DRV_FEATURE_MIGRATION_V3:
 +return 1;
 +default:
 +return 0;
 +}
 +}
 +
  static const char *
  libxlGetType(virConnectPtr conn ATTRIBUTE_UNUSED)
  {
 @@ -3842,12 +3866,600 @@ libxlIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED)
  return 1;
  }

 +static int libxlReadFixedMessage(int fd, const void *msg, int msgsz)
 +{
 +char buf[msgsz];
 +
 +if (saferead(fd, buf, msgsz) != msgsz || memcmp(buf, msg, msgsz)) {
 +return -1;
 +}
 +
 +return 0;
 +}
 +
 +static int doParseURI(const char *uri, char **p_hostname, int *p_port)
 +{
 +char *p, *hostname;
 +char port[16] = 0;
 +
 +if (strstr (uri, //)) {   /* Full URI. */
 +virURIPtr uriptr = virURIParse(uri);
 +if (!uriptr) {
 +libxlError(VIR_ERR_INVALID_ARG,
 +   _(Invalid URI));
 +return -1;
 +}
 +if (uriptr-scheme  STRCASENEQ(uriptr-scheme, xlmigr)) {
 +libxlError(VIR_ERR_INVALID_ARG,
 +_(Only xlmigr://
 + migrations are supported by Xen));
 +xmlFreeURI(uriptr);
 +return -1;
 +}


 This is just tcp migration.  Any reason for requiring the xlmigr scheme?
It's not a necessary. Just refer to xen_driver syntax, migration uri could be 
[xlmigr://]IP[:Port]. We can also define libxl own syntax, migration uri like 
IP[:Port].

 +if (!uriptr-server) {
 +libxlError(VIR_ERR_INVALID_ARG,
 +  _(A hostname must be
 + specified in the URI));
 +xmlFreeURI(uriptr);
 +return -1;
 +}
 +hostname = strdup(uriptr-server);


 I think it would be better to rework this, and other uses of strdup()
 and snprintf() in this function, to use virAsprintf().

 +if (!hostname) {
 +virReportOOMError();
 +xmlFreeURI(uriptr);
 +return -1;
 +}
 +if (uriptr-port)
 +snprintf(port, sizeof(port), %d, uriptr-port);
 +xmlFreeURI(uriptr);
 +}
 +else if ((p = strrchr(uri, ':')) != NULL) { /* hostname:port */
 +int port_nr, n;
 +
 +if (virStrToLong_i(p+1, NULL, 10, port_nr)  0) {
 +libxlError(VIR_ERR_INVALID_ARG,
 + _(Invalid port number));
 +return -1;
 +}
 +snprintf(port, sizeof(port), %d, port_nr);
 +
 +/* Get the hostname. */
 +n = p - uri; /* n = Length of hostname in bytes. */
 +hostname = strdup(uri);
 +if (!hostname) {
 +virReportOOMError();
 +return -1;
 +}
 +hostname[n] = '\0';
 +}
 +else {/* hostname (or IP address) */
 +hostname = strdup(uri);
 +if (!hostname) {
 +virReportOOMError();
 +return -1;
 +}
 +}
 +*p_hostname = hostname;
 +*p_port = atoi(port);
 +return 0;
 +}
 +
 +static bool libxlMigrationFromIsAllowed(libxlDriverPrivatePtr driver,
 virDomainObjPtr vm)
 +{
 +