[libvirt] [PATCH] startupPolicy: Change event argument
As this is on yet unreleased API this change is possible. This patch changes devAlias parameter in event callback to disk target as mgmt application is more likely to know target (it's required on domain definition) but aliases are generated by the daemon. So we can say this spares mgmt application at least one subsequent call of dumpxml on incoming event. --- daemon/remote.c | 6 +++--- examples/domain-events/events-c/event-test.c | 6 +++--- examples/domain-events/events-python/event-test.py | 6 +++--- include/libvirt/libvirt.h.in | 3 ++- python/libvirt-override-virConnect.py | 4 ++-- python/libvirt-override.c | 4 ++-- src/conf/domain_event.c | 18 +++++++++--------- src/conf/domain_event.h | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/remote/remote_driver.c | 2 +- src/remote/remote_protocol.x | 2 +- src/remote_protocol-structs | 2 +- 12 files changed, 30 insertions(+), 29 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 9d70163..c1441d1 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -455,7 +455,7 @@ static int remoteRelayDomainEventDiskChange(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason, void *opaque) { @@ -467,7 +467,7 @@ static int remoteRelayDomainEventDiskChange(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; VIR_DEBUG("Relaying domain %s %d disk change %s %s %s %d", - dom->name, dom->id, oldSrcPath, newSrcPath, devAlias, reason); + dom->name, dom->id, oldSrcPath, newSrcPath, target, reason); /* build return data */ memset(&data, 0, sizeof data); @@ -483,7 +483,7 @@ static int remoteRelayDomainEventDiskChange(virConnectPtr conn ATTRIBUTE_UNUSED, data.oldSrcPath = oldSrcPath_p; data.newSrcPath = newSrcPath_p; - if (!(data.devAlias = strdup(devAlias))) + if (!(data.target = strdup(target))) goto mem_error; data.reason = reason; diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 7c99222..adc6d7c 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -293,13 +293,13 @@ static int myDomainEventDiskChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason, void *opaque ATTRIBUTE_UNUSED) { - printf("%s EVENT: Domain %s(%d) disk change oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s\n", + printf("%s EVENT: Domain %s(%d) disk change oldSrcPath: %s newSrcPath: %s target: %s reason: %s\n", __func__, virDomainGetName(dom), virDomainGetID(dom), - oldSrcPath, newSrcPath, devAlias, diskChangeReasonStrings[reason]); + oldSrcPath, newSrcPath, target, diskChangeReasonStrings[reason]); return 0; } diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py index 9628f6e..5dd6eaa 100644 --- a/examples/domain-events/events-python/event-test.py +++ b/examples/domain-events/events-python/event-test.py @@ -469,9 +469,9 @@ def myDomainEventIOErrorCallback(conn, dom, srcpath, devalias, action, opaque): def myDomainEventGraphicsCallback(conn, dom, phase, localAddr, remoteAddr, authScheme, subject, opaque): print "myDomainEventGraphicsCallback: Domain %s(%s) %d %s" % (dom.name(), dom.ID(), phase, authScheme) -def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, newSrcPath, devAlias, reason, opaque): - print "myDomainEventDiskChangeCallback: Domain %s(%s) disk change oldSrcPath: %s newSrcPath: %s devAlias: %s reason: %s" % ( - dom.name(), dom.ID(), oldSrcPath, newSrcPath, devAlias, reason) +def myDomainEventDiskChangeCallback(conn, dom, oldSrcPath, newSrcPath, target, reason, opaque): + print "myDomainEventDiskChangeCallback: Domain %s(%s) disk change oldSrcPath: %s newSrcPath: %s target %s reason: %s" % ( + dom.name(), dom.ID(), oldSrcPath, newSrcPath, target, reason) def usage(out=sys.stderr): print >>out, "usage: "+os.path.basename(sys.argv[0])+" [-hdl] [uri]" print >>out, " uri will default to qemu:///system" diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7102bce..7bcbae6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3010,6 +3010,7 @@ typedef enum { * @dom: domain on which the event occurred * @oldSrcPath: old source path * @newSrcPath: new source path + * @target: disk target where event occurred * @reason: reason why this callback was called; any of * virConnectDomainEventDiskChangeReason * @opaque: application specified data @@ -3026,7 +3027,7 @@ typedef void (*virConnectDomainEventDiskChangeCallback)(virConnectPtr conn, virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason, void *opaque); diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index b908b32..75058d9 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -125,13 +125,13 @@ except AttributeError: pass - def _dispatchDomainEventDiskChangeCallback(self, dom, oldSrcPath, newSrcPath, devAlias, reason, cbData): + def _dispatchDomainEventDiskChangeCallback(self, dom, oldSrcPath, newSrcPath, target, reason, cbData): """Dispatches event to python user domain diskChange event callbacks """ cb = cbData["cb"] opaque = cbData["opaque"] - cb(self, virDomain(self, _obj=dom), oldSrcPath, newSrcPath, devAlias, reason, opaque) + cb(self, virDomain(self, _obj=dom), oldSrcPath, newSrcPath, target, reason, opaque) return 0; def domainEventDeregisterAny(self, callbackID): diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 1759bae..310bc00 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -4379,7 +4379,7 @@ libvirt_virConnectDomainEventDiskChangeCallback(virConnectPtr conn ATTRIBUTE_UNU virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason, void *opaque) { @@ -4407,7 +4407,7 @@ libvirt_virConnectDomainEventDiskChangeCallback(virConnectPtr conn ATTRIBUTE_UNU (char*)"OsssiO", pyobj_dom, oldSrcPath, newSrcPath, - devAlias, reason, pyobj_cbData); + target, reason, pyobj_cbData); Py_DECREF(pyobj_cbData); Py_DECREF(pyobj_dom); diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index a04b9d0..2ab68dc 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -91,7 +91,7 @@ struct _virDomainEvent { struct { char *oldSrcPath; char *newSrcPath; - char *devAlias; + char *target; int reason; } diskChange; } data; @@ -519,7 +519,7 @@ void virDomainEventFree(virDomainEventPtr event) case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: VIR_FREE(event->data.diskChange.oldSrcPath); VIR_FREE(event->data.diskChange.newSrcPath); - VIR_FREE(event->data.diskChange.devAlias); + VIR_FREE(event->data.diskChange.target); break; } @@ -978,14 +978,14 @@ virDomainEventDiskChangeNew(int id, const char *name, unsigned char *uuid, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, int reason) + const char *target, int reason) { virDomainEventPtr ev = virDomainEventNewInternal(VIR_DOMAIN_EVENT_ID_DISK_CHANGE, id, name, uuid); if (ev) { - if (!(ev->data.diskChange.devAlias = strdup(devAlias))) + if (!(ev->data.diskChange.target = strdup(target))) goto error; if (oldSrcPath && @@ -1010,23 +1010,23 @@ error: virDomainEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason) { return virDomainEventDiskChangeNew(obj->def->id, obj->def->name, obj->def->uuid, oldSrcPath, - newSrcPath, devAlias, reason); + newSrcPath, target, reason); } virDomainEventPtr virDomainEventDiskChangeNewFromDom(virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason) { return virDomainEventDiskChangeNew(dom->id, dom->name, dom->uuid, oldSrcPath, newSrcPath, - devAlias, reason); + target, reason); } /** @@ -1175,7 +1175,7 @@ void virDomainEventDispatchDefaultFunc(virConnectPtr conn, ((virConnectDomainEventDiskChangeCallback)cb)(conn, dom, event->data.diskChange.oldSrcPath, event->data.diskChange.newSrcPath, - event->data.diskChange.devAlias, + event->data.diskChange.target, event->data.diskChange.reason, cbopaque); break; diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 3ba418e..6c56fbd 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -182,12 +182,12 @@ virDomainEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, virDomainEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason); virDomainEventPtr virDomainEventDiskChangeNewFromDom(virDomainPtr dom, const char *oldSrcPath, const char *newSrcPath, - const char *devAlias, + const char *target, int reason); int virDomainEventQueuePush(virDomainEventQueuePtr evtQueue, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 198ebcc..82e2d70 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1668,7 +1668,7 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, "due to not accessible source '%s'", disk->dst, vm->def->name, uuid, disk->src); - event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->dst, VIR_DOMAIN_DISK_CHANGE_MISSING_ON_START); if (event) qemuDomainEventQueue(driver, event); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e98ebd7..8726112 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3354,7 +3354,7 @@ remoteDomainBuildEventDiskChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventDiskChangeNewFromDom(dom, msg->oldSrcPath ? *msg->oldSrcPath : NULL, msg->newSrcPath ? *msg->newSrcPath : NULL, - msg->devAlias, + msg->target, msg->reason); virDomainFree(dom); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d135653..8f8c55b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2014,7 +2014,7 @@ struct remote_domain_event_disk_change_msg { remote_nonnull_domain dom; remote_string oldSrcPath; remote_string newSrcPath; - remote_nonnull_string devAlias; + remote_nonnull_string target; int reason; }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 569fcb3..fe347fa 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1513,7 +1513,7 @@ struct remote_domain_event_disk_change_msg { remote_nonnull_domain dom; remote_string oldSrcPath; remote_string newSrcPath; - remote_nonnull_string devAlias; + remote_nonnull_string target; int reason; }; struct remote_domain_managed_save_args { -- 1.7.3.4
On Wed, Oct 26, 2011 at 12:26:51PM +0200, Michal Privoznik wrote:
As this is on yet unreleased API this change is possible. This patch changes devAlias parameter in event callback to disk target as mgmt application is more likely to know target (it's required on domain definition) but aliases are generated by the daemon. So we can say this spares mgmt application at least one subsequent call of dumpxml on incoming event.
We already use 'devAlias' in the block I/O error event, so IMHO it is good that we are consistent here too. So I'd rather not change what we have for media change events. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 26.10.2011 12:36, Daniel P. Berrange wrote:
On Wed, Oct 26, 2011 at 12:26:51PM +0200, Michal Privoznik wrote:
As this is on yet unreleased API this change is possible. This patch changes devAlias parameter in event callback to disk target as mgmt application is more likely to know target (it's required on domain definition) but aliases are generated by the daemon. So we can say this spares mgmt application at least one subsequent call of dumpxml on incoming event.
We already use 'devAlias' in the block I/O error event, so IMHO it is good that we are consistent here too. So I'd rather not change what we have for media change events.
Daniel
But on the otherhand, I assume 'devAlias' is limited just to QEMU, isn't it? But target isn't. So I rather have good API than consistent but bad. Moreover, I can't avoid feeling that we have 'devAlias' only because it is what we actually get on qemu event (so we don't have to do any translation, just a simple passthru). However, ff we change this to target and once QEMU sens events on cdrom eject we need to do a translation, though. I'd enhance others to speak up as once this gets written into a stone ... Michal
On Wed, Oct 26, 2011 at 01:22:02PM +0200, Michal Privoznik wrote:
On 26.10.2011 12:36, Daniel P. Berrange wrote:
On Wed, Oct 26, 2011 at 12:26:51PM +0200, Michal Privoznik wrote:
As this is on yet unreleased API this change is possible. This patch changes devAlias parameter in event callback to disk target as mgmt application is more likely to know target (it's required on domain definition) but aliases are generated by the daemon. So we can say this spares mgmt application at least one subsequent call of dumpxml on incoming event.
We already use 'devAlias' in the block I/O error event, so IMHO it is good that we are consistent here too. So I'd rather not change what we have for media change events.
Daniel
But on the otherhand, I assume 'devAlias' is limited just to QEMU, isn't it? But target isn't. So I rather have good API than consistent but bad. Moreover, I can't avoid feeling that we have 'devAlias' only because it is what we actually get on qemu event (so we don't have to do any translation, just a simple passthru). However, ff we change this to target and once QEMU sens events on cdrom eject we need to do a translation, though.
The device aliases are not intended to be limited to just QEMU. The intention when adding device aliases was to have a standard way to uniquely refer to a particular device, no matter what type of device it is. Previously you had to use disk targets, net MAC addrs, serial port numbers, etc, etc, and some device types didn't even have any good unique attribute. With device alias, we have a way to provide a standard identifer for any type of device. While other drivers may not use this ability yet, there's nothing preventing them from doing so. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/26/2011 05:30 AM, Daniel P. Berrange wrote:
The device aliases are not intended to be limited to just QEMU. The intention when adding device aliases was to have a standard way to uniquely refer to a particular device, no matter what type of device it is. Previously you had to use disk targets, net MAC addrs, serial port numbers, etc, etc, and some device types didn't even have any good unique attribute. With device alias, we have a way to provide a standard identifer for any type of device. While other drivers may not use this ability yet, there's nothing preventing them from doing so.
I guess I can live with devAlias on output, if there were an easy way to map from target to devAlias, and also to map from devAlias to target. Also, it would be nice if functions that take device names and/or device paths on input could also be taught to take devAlias on input as another synonym. In particular, I'm thinking of my recent work in commits 88a993b and 89b6284, where I taught disk snapshots, block peek, and getBlockInfo to all operate on target name or disk path as synonyms. Making those functions also operate on device aliases, as well as making 'virsh domblklist' capable of outputting the devAlias that corresponds to each disk, will make it easier to go between any of the three namespaces (where two are under the control of the xml). But your argument that blk io error events already use devAlias is pretty convincing. I guess even if we don't have full transparency between the namespaces yet, that consistency argues that both disk-related events should be using the same data. So I think the best plan forward is for this patch to keep devAlias after all (it needs documentation, but it reduces the number of other places to patch), and then later add a patch (series) that makes it easier to convert between aliases and target names. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik