Re: [libvirt] [PATCH] qemu: fix cannot attach a virtio channel

2015-06-15 Thread lhuang 


On 06/15/2015 04:25 PM, Ján Tomko wrote:

I'd rather be more specific in the commit summary, e.g.:
fix address allocation on chardev hotplug


good summary, thanks for you help


On Wed, Jun 10, 2015 at 10:49:37PM +0800, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1230039

When attach a channel which target type is virtio, we will
get an error:

error: Failed to attach device from channel-file.xml
error: internal error: virtio serial device has invalid address type

This issue was introduced in commit 9807c4.
We didn't check the chr device type is serial then check
if the device target type is pci-serial, but not all the
Chr device is a serial type so we should check the device
type before check the target type to avoid assign wrong
address to other device type chr device.

Also most of chr device do not need {pci, virtio-serial} address
in qemu, we just get the address for the device which needed.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 72 +++--
  1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3562de6..4d60513 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1531,6 +1531,48 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
  return ret;
  }
  
+static int

+qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv,
+virDomainChrDefPtr chr)
+{
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, true)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI 
+(chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+ chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) 

Do we need to check the address type here? pci-serial should always have
a PCI address.


Yes, pci-serial should always have a PCI address, and add this check
is avoid always assign a pci address even we already specified the address
which type is not pci, in that case i think output an error will be better.
(although it is a corner case, and i notice when we start a guest with wrong
address type pci-serial will get the error,  but cannot get the error 
when attach

it, so i guess QE will complain about this ;) )

and after add this check the error will be:

# virsh attach-device rhel7.0  pci-serial.xml
error: Failed to attach device from pci-serial.xml
error: unsupported configuration: pci-serial requires address of pci type


+virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, false)  0)
+return -1;
+
+return 0;
+}
+
+static void
+qemuDomainAttachChrDeviceReleaseAddr(qemuDomainObjPrivatePtr priv,
+ virDomainObjPtr vm,
+ virDomainChrDefPtr chr)
+{
+if ((chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+ chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) ||
+(chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+ chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))
+virDomainVirtioSerialAddrRelease(priv-vioserialaddrs, chr-info);
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI)
+qemuDomainReleaseDeviceAddress(vm, chr-info, NULL);
+}
+
  int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainChrDefPtr chr)
@@ -1541,7 +1583,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  char *devstr = NULL;
  char *charAlias = NULL;
  bool need_release = false;
-bool allowZero = false;
  
  if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {

  virReportError(VIR_ERR_OPERATION_INVALID, %s,
@@ -1552,22 +1593,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  if (qemuAssignDeviceChrAlias(vmdef, chr, -1)  0)
  goto cleanup;
  
-if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 

-chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO)
-allowZero = true;
-
-if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
-if (virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
-goto cleanup;
-} else if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
-

Re: [libvirt] [PATCH] qemu: fix cannot attach a virtio channel

2015-06-15 Thread Ján Tomko 
I'd rather be more specific in the commit summary, e.g.:
fix address allocation on chardev hotplug

On Wed, Jun 10, 2015 at 10:49:37PM +0800, Luyao Huang wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1230039
 
 When attach a channel which target type is virtio, we will
 get an error:
 
 error: Failed to attach device from channel-file.xml
 error: internal error: virtio serial device has invalid address type
 
 This issue was introduced in commit 9807c4.
 We didn't check the chr device type is serial then check
 if the device target type is pci-serial, but not all the
 Chr device is a serial type so we should check the device
 type before check the target type to avoid assign wrong
 address to other device type chr device.
 
 Also most of chr device do not need {pci, virtio-serial} address
 in qemu, we just get the address for the device which needed.
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 72 
 +++--
  1 file changed, 46 insertions(+), 26 deletions(-)
 
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index 3562de6..4d60513 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -1531,6 +1531,48 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
  return ret;
  }
  
 +static int
 +qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv,
 +virDomainChrDefPtr chr)
 +{
 +if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO 
 +virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
 +chr-info, true)  0)
 +return -1;
 +
 +if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
 +chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI 

 +(chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
 + chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) 

Do we need to check the address type here? pci-serial should always have
a PCI address.

 +virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
 +return -1;
 +
 +if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
 +chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
 +virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
 +chr-info, false)  0)
 +return -1;
 +
 +return 0;
 +}
 +
 +static void
 +qemuDomainAttachChrDeviceReleaseAddr(qemuDomainObjPrivatePtr priv,
 + virDomainObjPtr vm,
 + virDomainChrDefPtr chr)
 +{
 +if ((chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 + chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) ||
 +(chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
 + chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))
 +virDomainVirtioSerialAddrRelease(priv-vioserialaddrs, chr-info);
 +
 +if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
 +chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI)
 +qemuDomainReleaseDeviceAddress(vm, chr-info, NULL);
 +}
 +
  int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainChrDefPtr chr)
 @@ -1541,7 +1583,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  char *devstr = NULL;
  char *charAlias = NULL;
  bool need_release = false;
 -bool allowZero = false;
  
  if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
  virReportError(VIR_ERR_OPERATION_INVALID, %s,
 @@ -1552,22 +1593,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  if (qemuAssignDeviceChrAlias(vmdef, chr, -1)  0)
  goto cleanup;
  
 -if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO)
 -allowZero = true;
 -
 -if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
 -if (virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
 -goto cleanup;
 -} else if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
 -/* XXX */
 -} else {
 -if (virDomainVirtioSerialAddrAutoAssign(NULL,
 -priv-vioserialaddrs,
 -chr-info,
 -allowZero)  0)
 -goto cleanup;
 -}
 +if (qemuDomainAttachChrDeviceAssignAddr(priv, chr)  0)
 +goto cleanup;
  need_release = true;
  
  if (qemuBuildChrDeviceStr(devstr, vm-def, chr, priv-qemuCaps)  0)
 @@ -1601,15 +1628,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
   cleanup:
  if (ret  0  virDomainObjIsActive(vm))

[libvirt] [PATCH] qemu: fix cannot attach a virtio channel

2015-06-10 Thread Luyao Huang 
https://bugzilla.redhat.com/show_bug.cgi?id=1230039

When attach a channel which target type is virtio, we will
get an error:

error: Failed to attach device from channel-file.xml
error: internal error: virtio serial device has invalid address type

This issue was introduced in commit 9807c4.
We didn't check the chr device type is serial then check
if the device target type is pci-serial, but not all the
Chr device is a serial type so we should check the device
type before check the target type to avoid assign wrong
address to other device type chr device.

Also most of chr device do not need {pci, virtio-serial} address
in qemu, we just get the address for the device which needed.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
 src/qemu/qemu_hotplug.c | 72 +++--
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3562de6..4d60513 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1531,6 +1531,48 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
 return ret;
 }
 
+static int
+qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv,
+virDomainChrDefPtr chr)
+{
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, true)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI 
+(chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+ chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) 
+virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, false)  0)
+return -1;
+
+return 0;
+}
+
+static void
+qemuDomainAttachChrDeviceReleaseAddr(qemuDomainObjPrivatePtr priv,
+ virDomainObjPtr vm,
+ virDomainChrDefPtr chr)
+{
+if ((chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+ chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) ||
+(chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+ chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))
+virDomainVirtioSerialAddrRelease(priv-vioserialaddrs, chr-info);
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI)
+qemuDomainReleaseDeviceAddress(vm, chr-info, NULL);
+}
+
 int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virDomainChrDefPtr chr)
@@ -1541,7 +1583,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 char *devstr = NULL;
 char *charAlias = NULL;
 bool need_release = false;
-bool allowZero = false;
 
 if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
 virReportError(VIR_ERR_OPERATION_INVALID, %s,
@@ -1552,22 +1593,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 if (qemuAssignDeviceChrAlias(vmdef, chr, -1)  0)
 goto cleanup;
 
-if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
-chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO)
-allowZero = true;
-
-if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
-if (virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
-goto cleanup;
-} else if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
-/* XXX */
-} else {
-if (virDomainVirtioSerialAddrAutoAssign(NULL,
-priv-vioserialaddrs,
-chr-info,
-allowZero)  0)
-goto cleanup;
-}
+if (qemuDomainAttachChrDeviceAssignAddr(priv, chr)  0)
+goto cleanup;
 need_release = true;
 
 if (qemuBuildChrDeviceStr(devstr, vm-def, chr, priv-qemuCaps)  0)
@@ -1601,15 +1628,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  cleanup:
 if (ret  0  virDomainObjIsActive(vm))
 qemuDomainChrInsertPreAllocCleanup(vm-def, chr);
-if (ret  0  need_release) {
-if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
-qemuDomainReleaseDeviceAddress(vm, chr-info, NULL);
-} else if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
-/* XXX */
-} else {
-