[libvirt] [PATCH 2/3] qemu: Separate calls based on controller bus type

[Eric Farman]

On 11/20/2013 04:16 AM, Ján Tomko wrote:
> On 11/12/2013 05:59 PM, Eric Farman wrote:
>> For systems without a PCI bus, attaching a SCSI controller fails:
>>
>>    [root at xxxxxxxx ~]# cat controller.xml
>>    <controller type='scsi' model='virtio-scsi' index='0' />
>>    [root at xxxxxxxx ~]# virsh attach-device guest01 controller.xml
>>    error: Failed to attach device from controller.xml
>>    error: XML error: No PCI buses available
>>
>> A similar problem occurs with the detach of a controller:
>>
>>    [root at xxxxxxxx ~]# virsh detach-device guest01 controller.xml
>>    error: Failed to detach device from controller.xml
>>    error: operation failed: controller scsi:0 not found
>>
>> The qemuDomainXXtachPciControllerDevice routines made assumptions
>> that any caller had a PCI bus.  These routines now selectively calls
>> PCI functions where necessary, and assigns the device information
>> type to one appropriate for the bus in use.
>>
>> Signed-off-by: Eric Farman <farman at linux.vnet.ibm.com>
>> ---
>>   src/qemu/qemu_hotplug.c |   52 ++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 29c6aea..f18e233 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -365,9 +365,23 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
>>           return -1;
>>       }
>>   
>> +    if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>> +        if (STRPREFIX(vm->def->os.machine, "s390-ccw") &&
>> +            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
>> +            controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
>> +        else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
>> +            controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
>> +    }
>> +
> These would be better inside the if below (if QEMU_CAPS_DEVICE is not set, we
> don't probe for the VIRTIO_CCW and VIRTIO_S390 caps anyway).
>
>>       if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>> -        if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0)
>> -            goto cleanup;
>> +        if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
>> +            controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> +            if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0)
>> +                goto cleanup;
>> +        } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
>> +            if (qemuDomainCCWAddressAssign(&controller->info, priv->ccwaddrs, true) < 0)
>> +                goto cleanup;
>> +        }
>
>>           releaseaddr = true;
>>           if (qemuAssignDeviceControllerAlias(controller) < 0)
>>               goto cleanup;
>> @@ -3031,18 +3046,27 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>>   
>>       detach = vm->def->controllers[idx];
>>   
>> -    if (!virDomainDeviceAddressIsValid(&detach->info,
>> -                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
>> -        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> -                       _("device cannot be detached without a PCI address"));
>> -        goto cleanup;
>> -    }
>> +    if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> +        if (!virDomainDeviceAddressIsValid(&detach->info,
>> +                                           VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                           _("device cannot be detached without a PCI address"));
>> +            goto cleanup;
>> +        }
>>   
>> -    if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
>> -        virReportError(VIR_ERR_OPERATION_FAILED,
>> -                       _("cannot hot unplug multifunction PCI device: %s"),
>> -                       dev->data.disk->dst);
>> -        goto cleanup;
>> +        if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED,
>> +                           _("cannot hot unplug multifunction PCI device: %s"),
>> +                           dev->data.disk->dst);
>> +            goto cleanup;
>> +        }
>> +    } else if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
>> +        if (!virDomainDeviceAddressIsValid(&detach->info,
>> +                                           VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                           _("device cannot be detached without a CCW address"));
>> +            goto cleanup;
>> +        }
>>       }
>>   
>>       if (qemuDomainControllerIsBusy(vm, detach)) {
>>
> This doesn't catch the case of ADDRESS_TYPE_NONE.
Ah, great point.  I'll make these changes, and send a v3 of the series 
to the list later today.
> How about:
>
> if (detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>      detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
>      detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) {
>      virReportError(VIR_ERR_OPERATION_FAILED,
>                     _("device with '%s' address cannot be detached"),
>                     virDomainDeviceAddressTypeToString(detach->info.type));
>      goto cleanup;
> }
>
> if (!virDomainDeviceAddressIsValid(&detach->info, detach->info.type)) {
>      virReportError(VIR_ERR_OPERATION_FAILED,
>                     _("device with invalid '%s' address cannot be detached"),
>                     virDomainDeviceAddressTypeToString(detach->info.type));
>      goto cleanup;
> }
>
> if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>      qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
>      virReportError(VIR_ERR_OPERATION_FAILED,
>                     _("cannot hot unplug multifunction PCI device: %s"),
>                     dev->data.disk->dst);
>      goto cleanup;
> }
>
> Jan
>
>
Eric