On 12/20/2017 04:29 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>
> We used VIR_ERR_OPERATION_FAILED when target detaching device
> is not found.
> That error code VIR_ERR_OPERATION_FAILED is widely used,
> so the tools powered by libvirt, such as nova,
> can't catch the exact errors from libvirt.
> This patch uses VIR_ERR_NO_DEVICE instead.
>
> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_hotplug.c | 51 +++++++++++++++++++++++++++++++-----------------
> 2 files changed, 35 insertions(+), 18 deletions(-)
>
So VIR_ERR_NO_DEVICE in include/libvirt/virterror.h is:
VIR_ERR_NO_DEVICE = 23, /* missing domain devices information */
and the error message generated from src/util/virerror.c
case VIR_ERR_NO_DEVICE:
if (info == NULL)
errmsg = _("missing devices information");
else
errmsg = _("missing devices information for %s");
break;
That message was perhaps intended for missing <devices> information in
the guest XML. Took me a while to search on it, but commit '4d56d3c6'
seems to be the last time it was used after an original commit
'8bc437e4' which was generated when /domain/devices/disk returned 0
disks defined (old stuff indeed).
In any case, you should not "reuse" an older error message just so you
can get the VIR_ERR_NO_DEVICE to key off of. Since consumer code would
be changing anyway to key off both the existing mechanism
(OPERATION_FAILED and "not found") and some newer error code, we should
create a new error code and message.
Not a really perfect situation here, but if you add a new message
VIR_ERR_DEVICE_MISSING which is "device not found" or "device not found:
%s", then you're looking for that new code and can take the error
message from libvirt too.
Adding a new message has implications in libvirt-perl and libvirt-go...
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d5c3b9abb..31e83f152 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -203,6 +203,7 @@ virDomainChrConsoleTargetTypeToString;
> virDomainChrDefForeach;
> virDomainChrDefFree;
> virDomainChrDefNew;
> +virDomainChrDeviceTypeToString;
> virDomainChrEquals;
> virDomainChrFind;
> virDomainChrGetDomainPtrs;
> @@ -427,6 +428,7 @@ virDomainMemoryDefFree;
> virDomainMemoryFindByDef;
> virDomainMemoryFindInactiveByDef;
> virDomainMemoryInsert;
> +virDomainMemoryModelTypeToString;
> virDomainMemoryRemove;
> virDomainMemorySourceTypeFromString;
> virDomainMemorySourceTypeToString;
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7de04c85a..0fa3c54c0 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3454,7 +3454,7 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
> int ret = -1;
>
> if (!olddev) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + virReportError(VIR_ERR_NO_DEVICE, "%s",
> _("cannot find existing graphics device to modify"));
I think the message could change to:
"cannot find existing graphics device to modify of type '%s'", type
but it perhaps should it's own patch - as in one patch to change the
message and one patch to change the error code.
> goto cleanup;
> }
> @@ -4743,7 +4743,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
> if ((idx = virDomainControllerFind(vm->def,
> dev->data.controller->type,
> dev->data.controller->idx)) < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("controller %s:%d not found"),
> virDomainControllerTypeToString(dev->data.controller->type),
> dev->data.controller->idx);
> @@ -4972,18 +4972,18 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> if (idx < 0) {
> switch (subsys->type) {
> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("host pci device %.4x:%.2x:%.2x.%.1x not found"),
> pcisrc->addr.domain, pcisrc->addr.bus,
> pcisrc->addr.slot, pcisrc->addr.function);
> break;
> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> if (usbsrc->bus && usbsrc->device) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("host usb device %03d.%03d not found"),
> usbsrc->bus, usbsrc->device);
> } else {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("host usb device vendor=0x%.4x product=0x%.4x not found"),
> usbsrc->vendor, usbsrc->product);
> }
> @@ -4992,13 +4992,13 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> if (scsisrc->protocol ==
> VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
> virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("host scsi iSCSI path %s not found"),
> iscsisrc->src->path);
> } else {
> virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
> &scsisrc->u.host;
> - virReportError(VIR_ERR_OPERATION_FAILED,
> + virReportError(VIR_ERR_NO_DEVICE,
> _("host scsi device %s:%u:%u.%llu not found"),
> scsihostsrc->adapter, scsihostsrc->bus,
> scsihostsrc->target, scsihostsrc->unit);
> @@ -5036,8 +5036,10 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));> + virReportError(VIR_ERR_NO_DEVICE,
> + _("Shmem device of model '%s' not found "
> + "in domain configuration"),
> + virDomainShmemModelTypeToString(dev->model));
s/Shmem/shmem
Although you'll need to split error code and error message change into
multiple patches... Also, there's some danger in changing the error
message especially since the reason you're adding "not found" to an
error code is because there's a code with a message that doesn't suffice
your needs. So what if there's some code somewhere that's looking for
"device not present in the domain configuration" that will now fail
because the message is changed.
Damned if you do, damned if you don't.
> return -1;
> }
>
> @@ -5093,8 +5095,10 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> watchdog->model == dev->model &&
> watchdog->action == dev->action &&
> virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("watchdog device not present in domain configuration"));
> + virReportError(VIR_ERR_NO_DEVICE,
> + _("watchdog device of model '%s' is not "
> + "found in domain configuration"),
> + virDomainWatchdogModelTypeToString(watchdog->model));
Likewise 2 patches - one for error code change and one for message change.
> return -1;
> }
>
> @@ -5134,8 +5138,13 @@ 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) {
> + char mac[VIR_MAC_STRING_BUFLEN];
> + virReportError(VIR_ERR_NO_DEVICE,
> + _("netdev '%s' not found in domain configuration"),
> + virMacAddrFormat(&dev->data.net->mac, mac));
Same...
> goto cleanup;
> + }
>
> detach = vm->def->nets[detachidx];
>
> @@ -5321,8 +5330,10 @@ 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_NO_DEVICE,
> + _("Chr device of type '%s' not found "
> + "in domain configuration"),
> + virDomainChrDeviceTypeToString(chr->deviceType));
Again here... Also it's "chr" not "Chr" in existing messages.
> goto cleanup;
> }
>
> @@ -5368,8 +5379,10 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
> int ret = -1;
>
> if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + virReportError(VIR_ERR_NO_DEVICE,
> + _("RNG device of model '%s' not found "
> + "in domain configuration"),
> + virDomainRNGBackendTypeToString(rng->model));
Again 2 patches...
> return -1;
> }
>
> @@ -5411,8 +5424,10 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
> qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
>
> if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("device not present in domain configuration"));
> + virReportError(VIR_ERR_NO_DEVICE,
> + _("memory device of model '%s' not found "
> + "in domain configuration"),
> + virDomainMemoryModelTypeToString(memdef->model));
here too.
John
> return -1;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
