[libvirt] [PATCH 2/3] virsh: prevent destroying a in-used network for net-destroy

[Laine Stump]

On 02/02/2015 09:08 AM, Lin Ma wrote:
> * add --system flag for net-dumpxml to show information about the
>   attached interfaces of the virtual network.

I don't like this extra flag - I think it is unnecessary. If we're going
to more info to the status, then we can just always add it to the status.

As to how it can be provided in the status XML (getting back to patch
1/3 which was using the netcf API in a way that I didn't like), better
data can be gathered into a list in the network object as guests are
created - this would be done in networkAllocateActualDevice() (and
networkNotifyActualDevice()) at the points where connections is
incremented for the network, and removed from the list in
networkReleaseActualDevice() where connections is decremented. You'll
notice that those functions get both the virDomainDefPtr and the
virDomainNetDefPtr, so they have enough information to store the name of
the domain as well as the namne of the interface.

I recall a long time ago considering adding this information to the
network XML, but I was dissuaded for some reason; unfortunately I don't
remember why...

>
> * call virNetworkGetXMLDesc in cmdNetworkDestroy to get the live state
>   info to check whether the virtual network is in use.
>
> * add --force flag for net-destroy to forcibly destroy the virtual
>   network even if it's in use.

As Michal points out (and I pointed out when you sent your previous
patchset), this does change behavior that some scripts may be dependent
on. I too like the increased safety, but am concerned about breaking
functionality. In cases like this, I always like to hear the learned
advice of Dan Berrange - Daniel, what do you say about changing the
default behavior of virsh net-destroy? Safe enough?

>
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  tools/virsh-network.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++----
>  tools/virsh.pod       |  8 +++++--
>  2 files changed, 64 insertions(+), 6 deletions(-)
>
> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
> index 5f8743c..17a5710 100644
> --- a/tools/virsh-network.c
> +++ b/tools/virsh-network.c
> @@ -253,6 +253,10 @@ static const vshCmdOptDef opts_network_destroy[] = {
>       .flags = VSH_OFLAG_REQ,
>       .help = N_("network name or uuid")
>      },
> +    {.name = "force",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("Forcefully stop a given network")
> +    },
>      {.name = NULL}
>  };
>  
> @@ -260,20 +264,55 @@ static bool
>  cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd)
>  {
>      virNetworkPtr network;
> -    bool ret = true;
> +    bool ret = false;
>      const char *name;
> +    char *br_xml = NULL;
> +    xmlDocPtr xml_doc = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
>  
>      if (!(network = vshCommandOptNetwork(ctl, cmd, &name)))
> -        return false;
> +        goto cleanup;
> +
> +#ifdef WITH_NETCF
> +    if (!vshCommandOptBool(cmd, "force")) {
> +        if (virNetworkIsActive(network) <= 0) {
> +            vshError(ctl, "%s", _("network is not active"));
> +            goto cleanup;
> +        }
> +        if (!(br_xml = virNetworkGetXMLDesc(network,
> +                                            VIR_NETWORK_XML_IFACE_ATTACHED)))
> +            goto cleanup;
> +
> +        if (!(xml_doc = virXMLParseStringCtxt(br_xml,
> +                                              _("(bridge interface "
> +                                                "definition)"), &ctxt))) {
> +            vshError(ctl, "%s", _("Failed to parse network configuration"));
> +            goto cleanup;
> +        }
> +
> +        if (virXPathNode("./bridge/interface[1]", ctxt) != NULL) {
> +            vshError(ctl, "%s", _("The network is in use by guests"));
> +            vshPrint(ctl, "%s", _("Use --force flag if you do want to do so"));
> +            goto cleanup;
> +        }
> +    }
> +#endif
>  
>      if (virNetworkDestroy(network) == 0) {
>          vshPrint(ctl, _("Network %s destroyed\n"), name);
>      } else {
>          vshError(ctl, _("Failed to destroy network %s"), name);
> -        ret = false;
> +        goto cleanup;
>      }
>  
> -    virNetworkFree(network);
> +    ret = true;
> + cleanup:
> +    if(network)
> +        virNetworkFree(network);
> +    VIR_FREE(br_xml);
> +    xmlXPathFreeContext(ctxt);
> +    xmlFreeDoc(xml_doc);
> +
>      return ret;
>  }
>  
> @@ -300,6 +339,10 @@ static const vshCmdOptDef opts_network_dumpxml[] = {
>       .type = VSH_OT_BOOL,
>       .help = N_("network information of an inactive domain")
>      },
> +    {.name = "system",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("current live state information including attached interfaces")
> +    },
>      {.name = NULL}
>  };
>  
> @@ -311,6 +354,7 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd)
>      char *dump;
>      unsigned int flags = 0;
>      int inactive;
> +    bool system = false;
>  
>      if (!(network = vshCommandOptNetwork(ctl, cmd, NULL)))
>          return false;
> @@ -319,6 +363,16 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd)
>      if (inactive)
>          flags |= VIR_NETWORK_XML_INACTIVE;
>  
> +    system = vshCommandOptBool(cmd, "system");
> +    if (system) {
> +        flags = VIR_NETWORK_XML_IFACE_ATTACHED;
> +        if (virNetworkIsActive(network) <= 0) {
> +            vshError(ctl, "%s", _("--system only works on active network"));
> +            virNetworkFree(network);
> +            return false;
> +        }
> +    }
> +
>      dump = virNetworkGetXMLDesc(network, flags);
>  
>      if (dump != NULL) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 804458e..3373ada 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2672,16 +2672,20 @@ to get a description of the XML network format used by libvirt.
>  Define a persistent virtual network from an XML I<file>, the network is just
>  defined but not instantiated (started).
>  
> -=item B<net-destroy> I<network>
> +=item B<net-destroy> I<network>  [I<--force>]
>  
>  Destroy (stop) a given transient or persistent virtual network
>  specified by its name or UUID. This takes effect immediately.
> +If I<--force> is specified, then forcefully destroy the virtual
> +network even if it's in use.
>  
> -=item B<net-dumpxml> I<network> [I<--inactive>]
> +=item B<net-dumpxml> I<network> [I<--inactive>] [I<--system>]
>  
>  Output the virtual network information as an XML dump to stdout.
>  If I<--inactive> is specified, then physical functions are not
>  expanded into their associated virtual functions.
> +If I<--system> is specified, then directly output the current
> +live state corresponding to this network from system.
>  
>  =item B<net-edit> I<network>
>