[libvirt] [PATCH v3 1/2] network: Introduce network hooks

[Daniel P. Berrange]

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 :|