On Mon, Feb 10, 2014 at 07:52:34PM +0100, Michal Privoznik wrote:
> @@ -3583,6 +3639,48 @@ validate:
> }
> }
>
> + /* finally we can call the 'plugged' hook script if any */
> + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
> + /* the XML construction is a bit complicated here,
> + * as we want to pass both domain XML and network XML */
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *xml, *net_xml, *dom_xml;
> + int hookret;
> +
> + net_xml = virNetworkDefFormat(netdef, 0);
> + dom_xml = virDomainDefFormat(dom, 0);
> +
> + if (!net_xml || !dom_xml) {
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> + goto error;
> + }
> +
> + virBufferAdd(&buf, net_xml, -1);
> + virBufferAdd(&buf, dom_xml, -1);
This isn't very easy for applications to consume. With all the
other things you can just pass stdin straight to your XML
parser and process it. When you concatenate 2 XML docs this
way it becomes much harder, since you have to split the two
docs which means parsing them without an XML parser. Usually
you'd want to place the 2 docs within a parent XML doc so the
overall result is still a single wellformed XML doc.
> +
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + goto error;
> + }
> +
> + xml = virBufferContentAndReset(&buf);
> +
> + hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
> + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
> + VIR_FREE(xml);
> +
> + /*
> + * If the script raised an error abort the allocation
> + */
> + if (hookret < 0)
> + goto error;
> + }
> +
> if (dev) {
> /* we are now assured of success, so mark the allocation */
> dev->connections++;
> @@ -3618,6 +3716,7 @@ error:
> }
>
> /* networkNotifyActualDevice:
> + * @dom: domain definition that @iface belongs to
> * @iface: the domain's NetDef with an "actual" device already filled in.
> *
> * Called to notify the network driver when libvirtd is restarted and
> @@ -3628,7 +3727,8 @@ error:
> * Returns 0 on success, -1 on failure.
> */
> int
> -networkNotifyActualDevice(virDomainNetDefPtr iface)
> +networkNotifyActualDevice(virDomainDefPtr dom,
> + virDomainNetDefPtr iface)
> {
> virNetworkDriverStatePtr driver = driverState;
> enum virDomainNetType actualType = virDomainNetGetActualType(iface);
> @@ -3781,6 +3881,48 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
> }
>
> success:
> + /* finally we can call the 'plugged' hook script if any */
> + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
> + /* the XML construction is a bit complicated here,
> + * as we want to pass both domain XML and network XML */
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *xml, *net_xml, *dom_xml;
> + int hookret;
> +
> + net_xml = virNetworkDefFormat(netdef, 0);
> + dom_xml = virDomainDefFormat(dom, 0);
> +
> + if (!net_xml || !dom_xml) {
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> + goto error;
> + }
> +
> + virBufferAdd(&buf, net_xml, -1);
> + virBufferAdd(&buf, dom_xml, -1);
> +
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + goto error;
> + }
> +
> + xml = virBufferContentAndReset(&buf);
> +
> + hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
> + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
> + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
> + VIR_FREE(xml);
> +
> + /*
> + * If the script raised an error abort the allocation
> + */
> + if (hookret < 0)
> + goto error;
> + }
> +
> netdef->connections++;
> VIR_DEBUG("Using network %s, %d connections",
> netdef->name, netdef->connections);
> @@ -3796,6 +3938,7 @@ error:
>
>
> /* networkReleaseActualDevice:
> + * @dom: domain definition that @iface belongs to
> * @iface: a domain's NetDef (interface definition)
> *
> * Given a domain <interface> element that previously had its <actual>
> @@ -3806,7 +3949,8 @@ error:
> * Returns 0 on success, -1 on failure.
> */
> int
> -networkReleaseActualDevice(virDomainNetDefPtr iface)
> +networkReleaseActualDevice(virDomainDefPtr dom,
> + virDomainNetDefPtr iface)
> {
> virNetworkDriverStatePtr driver = driverState;
> enum virDomainNetType actualType = virDomainNetGetActualType(iface);
> @@ -3925,6 +4069,42 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
> success:
> if (iface->data.network.actual)
> netdef->connections--;
> +
> + /* finally we can call the 'unplugged' hook script if any */
> + if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
> + /* the XML construction is a bit complicated here,
> + * as we want to pass both domain XML and network XML */
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *xml, *net_xml, *dom_xml;
> +
> + net_xml = virNetworkDefFormat(netdef, 0);
> + dom_xml = virDomainDefFormat(dom, 0);
> +
> + if (!net_xml || !dom_xml) {
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> + goto error;
> + }
> +
> + virBufferAdd(&buf, net_xml, -1);
> + virBufferAdd(&buf, dom_xml, -1);
> +
> + VIR_FREE(net_xml);
> + VIR_FREE(dom_xml);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + goto error;
> + }
> +
> + xml = virBufferContentAndReset(&buf);
> +
> + virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
> + VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
> + VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
> + VIR_FREE(xml);
> + }
> +
> VIR_DEBUG("Releasing network %s, %d connections",
> netdef->name, netdef->connections);
There's alot of repeated code patterns throughout this file.
Seems this would be shorter if we had a helper method for doing
all the hook presence check / xml formatting hook invocation.
Each usage would then just be a single line function call.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list