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

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