[libvirt] [PATCH] qemu: Do not overwrite existing messages in hotplug

[Eric Farman]

On 04/26/2017 01:55 PM, John Ferlan wrote:
>
>
> On 04/24/2017 02:02 PM, Eric Farman wrote:
>> I tried to attach a SCSI LUN to two different guests, and forgot
>> to specify "shareable" in the hostdev XML.  Attaching the device
>> to the second guest failed, but the message was not helpful in
>> telling me what I was doing wrong:
>>
>>   $ cat scsi_scratch_disk.xml
>>     <hostdev mode='subsystem' type='scsi'>
>>       <source>
>>         <adapter name='scsi_host3'/>
>>         <address bus='0' target='15' unit='1074151456'/>
>>       </source>
>>     </hostdev>
>>
>>   $ virsh attach-device dasd_sles_d99c scsi_scratch_disk.xml
>>   Device attached successfully
>>
>>   $ virsh attach-device dasd_fedora_0e1e scsi_scratch_disk.xml
>>   error: Failed to attach device from scsi_scratch_disk.xml
>>   error: internal error: Unable to prepare scsi hostdev: scsi_host3:0:15:1074151456
>>
>> I eventually discovered my error, but thought it was weird that
>> Libvirt doesn't provide something more helpful in this case.
>> Looking over the code we had just gone through, I commented out
>> the "internal error" message, and got something more useful:
>>
>>   $ virsh attach-device dasd_fedora_0e1e scsi_scratch_disk.xml
>>   error: Failed to attach device from scsi_scratch_disk.xml
>>   error: Requested operation is not valid: SCSI device 3:0:15:1074151456 is already in use by other domain(s) as 'non-shareable'
>>
>> Seems that the virReportError issued for an internal error
>> overwrites one that might have already existed.  Rather than
>> remove them altogether (there may be error paths that don't
>> spit out a more helpful message), I thought maybe it'd be
>> best to check if one exists, and exit if one does.  If not,
>> the existing internal error messages are probably helpful.
>
> Yep - this is more or less what happens you get the last error reported.
>
> Although I
>>
>> Make this adjustment for both virtio-scsi and vhost-scsi
>> devices.
>>
>> Signed-off-by: Eric Farman <farman at linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> ---
>>  src/qemu/qemu_hotplug.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 120bcdc..d421a77 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -2470,6 +2470,10 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>>      if (qemuHostdevPrepareSCSIDevices(driver, vm->def->name,
>>                                        &hostdev, 1)) {
>
> This should have been a "< 0" check, sigh.... Looks like commit id
> '8f76ad99' never added that... This should be adjusted as well.
>
>>          virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
>> +
>> +        if (virGetLastError())
>> +            return -1;
>> +
>
> So the question becomes can qemuHostdevPrepareSCSIDevices return -1
> without setting an error message...
>
> For all the paths I checked I couldn't find one that did, so I feel
> comfortable in just saying nix the messages below.
>
>>          if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
>>              virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -2595,9 +2599,12 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
>>
>>      if (qemuHostdevPrepareSCSIVHostDevices(driver, vm->def->name, &hostdev, 1) < 0) {
>>          virDomainHostdevSubsysSCSIVHostPtr hostsrc = &hostdev->source.subsys.u.scsi_host;
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Unable to prepare scsi_host hostdev: %s"),
>> -                       hostsrc->wwpn);
>> +
>> +        if (!virGetLastError()) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Unable to prepare scsi_host hostdev: %s"),
>> +                           hostsrc->wwpn);
>> +        }
>
> Same here... Although it's related to the previous one - let's create a
> separate patch for it.
>
> I think you end up with 3 patches #1 to check -1, #2 to get rid of the
> first set of messages, and #3 to get rid of this message set.

The above all sounds good.  Thanks, I'll whip up a v2.

  - Eric

>
> John
>>          return -1;
>>      }
>>
>>
>