1. Patchew
  2. Libvirt
  3. View series

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

Eric Farman posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1493056972-28026-1-git-send-email-farman@linux.vnet.ibm.com
src/qemu/qemu_hotplug.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
[libvirt] [PATCH] qemu: Do not overwrite existing messages in hotplug
Posted by Eric Farman 6 years, 11 months ago 
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.

Make this adjustment for both virtio-scsi and vhost-scsi
devices.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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)) {
         virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
+
+        if (virGetLastError())
+            return -1;
+
         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);
+        }
         return -1;
     }
 
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Do not overwrite existing messages in hotplug
Posted by John Ferlan 6 years, 11 months ago 

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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.

John
>          return -1;
>      }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Do not overwrite existing messages in hotplug
Posted by Eric Farman 6 years, 11 months ago 

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@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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;
>>      }
>>
>>
>

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.