From: Chen Hanxiao <chenhanxiao@gmail.com>
Some services, such as Nova, check whether device was not found
by errror messages "not found". [1]
This patch unify logs of qemuDomainDetachDeviceLive, which will be helpful.
[1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L406
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
---
src/qemu/qemu_hotplug.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d97aa6051..925574b92 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5093,7 +5093,7 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("device not present in domain configuration"));
+ _("device not found in domain configuration"));
return -1;
}
@@ -5150,7 +5150,7 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
watchdog->action == dev->action &&
virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("watchdog device not present in domain configuration"));
+ _("watchdog device not found in domain configuration"));
return -1;
}
@@ -5233,8 +5233,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
- if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
+ if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("netdev %s not found"), dev->data.net->mac.addr);
goto cleanup;
+ }
detach = vm->def->nets[detachidx];
@@ -5420,8 +5423,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
char *devstr = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("device not present in domain configuration"));
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("device %s not found in domain configuration"),
+ chr->target.name);
goto cleanup;
}
@@ -5468,7 +5472,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("device not present in domain configuration"));
+ _("device not found in domain configuration"));
return -1;
}
@@ -5511,7 +5515,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("device not present in domain configuration"));
+ _("device not found in domain configuration"));
return -1;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/14/2017 06:16 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@gmail.com>
>
> Some services, such as Nova, check whether device was not found
> by errror messages "not found". [1]
error
>
> This patch unify logs of qemuDomainDetachDeviceLive, which will be helpful.
>
> [1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L406
>
> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> ---
> src/qemu/qemu_hotplug.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
Something about a tool that parses the error message(s) looking for a
specific message string in English and needing to alter libvirt sources
to match that tools' needs strikes me as incorrect and a "slippery
slope" to follow.
I'm not in favor of this because we'll be constantly chasing these types
of bugs to match some other tools' (what I think is) incorrect means to
handle errors.
John
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d97aa6051..925574b92 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5093,7 +5093,7 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>
> if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + _("device not found in domain configuration"));
> return -1;
> }
>
> @@ -5150,7 +5150,7 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> watchdog->action == dev->action &&
> virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("watchdog device not present in domain configuration"));
> + _("watchdog device not found in domain configuration"));
> return -1;
> }
>
> @@ -5233,8 +5233,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
> virDomainNetDefPtr detach = NULL;
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> - if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
> + if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("netdev %s not found"), dev->data.net->mac.addr);
> goto cleanup;
> + }
>
> detach = vm->def->nets[detachidx];
>
> @@ -5420,8 +5423,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
> char *devstr = NULL;
>
> if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("device %s not found in domain configuration"),
> + chr->target.name);
> goto cleanup;
> }
>
> @@ -5468,7 +5472,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
>
> if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + _("device not found in domain configuration"));
> return -1;
> }
>
> @@ -5511,7 +5515,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
>
> if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + _("device not found in domain configuration"));
> return -1;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
At 2017-12-16 09:26:32, "John Ferlan" <jferlan@redhat.com> wrote: > > >On 12/14/2017 06:16 AM, Chen Hanxiao wrote: >> From: Chen Hanxiao <chenhanxiao@gmail.com> >> >> Some services, such as Nova, check whether device was not found >> by errror messages "not found". [1] > >error > >> >> This patch unify logs of qemuDomainDetachDeviceLive, which will be helpful. >> >> [1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L406 >> >> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> >> --- >> src/qemu/qemu_hotplug.c | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> > >Something about a tool that parses the error message(s) looking for a >specific message string in English and needing to alter libvirt sources >to match that tools' needs strikes me as incorrect and a "slippery >slope" to follow. > >I'm not in favor of this because we'll be constantly chasing these types >of bugs to match some other tools' (what I think is) incorrect means to >handle errors. > Agree. But we don't have enough error code to cover all of scenario. For qemuDomainDetachDeviceDiskLive, VIR_ERR_OPERATION_FAILED can be "disk not found", also can be "cannot hot unplug multifunction PCI device" in the following call of qemuDomainDetachVirtioDiskDevice. So the tools powered by libvirt had to find a workaround by analyzing error messages... Regards, - Chen -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/15/2017 09:50 PM, Chen Hanxiao wrote: > > > At 2017-12-16 09:26:32, "John Ferlan" <jferlan@redhat.com> wrote: >> >> >> On 12/14/2017 06:16 AM, Chen Hanxiao wrote: >>> From: Chen Hanxiao <chenhanxiao@gmail.com> >>> >>> Some services, such as Nova, check whether device was not found >>> by errror messages "not found". [1] >> >> error >> >>> >>> This patch unify logs of qemuDomainDetachDeviceLive, which will be helpful. >>> >>> [1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L406 >>> >>> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> >>> --- >>> src/qemu/qemu_hotplug.c | 18 +++++++++++------- >>> 1 file changed, 11 insertions(+), 7 deletions(-) >>> >> >> Something about a tool that parses the error message(s) looking for a >> specific message string in English and needing to alter libvirt sources >> to match that tools' needs strikes me as incorrect and a "slippery >> slope" to follow. >> >> I'm not in favor of this because we'll be constantly chasing these types >> of bugs to match some other tools' (what I think is) incorrect means to >> handle errors. >> > > Agree. > But we don't have enough error code to cover all of scenario. > Then there'd be a "different" fix required - adding an error code... Perhaps VIR_ERR_NO_DEVICE which would have error text "device %s not found"? Of course that would require up the stack source code changes as well. Still better than scanning the error message which only works for a specific language set. IOW: how would this work for non "English" languages? > For qemuDomainDetachDeviceDiskLive, VIR_ERR_OPERATION_FAILED can be > "disk not found", also can be "cannot hot unplug multifunction PCI device" > in the following call of qemuDomainDetachVirtioDiskDevice. So it's "OK" to have a different operational failure? I assume that'll cause a failure elsewhere in the code. I didn't go digging on the source code - just a quick look at the _try_detach_device method from the above nova link. > > So the tools powered by libvirt had to find a workaround by > analyzing error messages... Again, wouldn't those tools be broken without the right language set being used? John > > Regards, > - Chen > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
At 2017-12-18 21:57:11, "John Ferlan" <jferlan@redhat.com> wrote: > > >On 12/15/2017 09:50 PM, Chen Hanxiao wrote: >> >> >> At 2017-12-16 09:26:32, "John Ferlan" <jferlan@redhat.com> wrote: >>> >>> >>> On 12/14/2017 06:16 AM, Chen Hanxiao wrote: >>>> From: Chen Hanxiao <chenhanxiao@gmail.com> >>>> >>>> Some services, such as Nova, check whether device was not found >>>> by errror messages "not found". [1] >>> >>> error >>> >>>> >>>> This patch unify logs of qemuDomainDetachDeviceLive, which will be helpful. >>>> >>>> [1] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L406 >>>> >>>> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> >>>> --- >>>> src/qemu/qemu_hotplug.c | 18 +++++++++++------- >>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>> >>> >>> Something about a tool that parses the error message(s) looking for a >>> specific message string in English and needing to alter libvirt sources >>> to match that tools' needs strikes me as incorrect and a "slippery >>> slope" to follow. >>> >>> I'm not in favor of this because we'll be constantly chasing these types >>> of bugs to match some other tools' (what I think is) incorrect means to >>> handle errors. >>> >> >> Agree. >> But we don't have enough error code to cover all of scenario. >> > >Then there'd be a "different" fix required - adding an error code... >Perhaps VIR_ERR_NO_DEVICE which would have error text "device %s not found"? Thanks for the review. I'll post a patch to throw VIR_ERR_NO_DEVICE for hot_plug cases. > >Of course that would require up the stack source code changes as well. >Still better than scanning the error message which only works for a >specific language set. IOW: how would this work for non "English" languages? > >> For qemuDomainDetachDeviceDiskLive, VIR_ERR_OPERATION_FAILED can be >> "disk not found", also can be "cannot hot unplug multifunction PCI device" >> in the following call of qemuDomainDetachVirtioDiskDevice. > >So it's "OK" to have a different operational failure? I assume that'll >cause a failure elsewhere in the code. I didn't go digging on the source >code - just a quick look at the _try_detach_device method from the above >nova link. > >> >> So the tools powered by libvirt had to find a workaround by >> analyzing error messages... > >Again, wouldn't those tools be broken without the right language set >being used? > Thanks for the detail clarification. Checking words in error log is a bad way to catch errors and should be fixed. Regards, - Chen -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.