[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/5 v3] Added new option to virsh net-dumpxml called --inactive



Eric, 

Many Thanks for your comments and for pushing the patches through.

I understand the review comments you and Daniel posted on my patch series and was going to work on them today.

I saw your modified patches and wanted to thank you for your contribution towards the patch series and for pushing them upstream.

Regards,
Shradha Shah

On 01/11/2012 07:31 PM, Eric Blake wrote:
> On 12/14/2011 03:50 AM, Shradha Shah wrote:
>> The above option helps to differentiate between implicit and explicit interface pools.
>> ---
>>  include/libvirt/libvirt.h.in |    4 ++++
>>  src/conf/network_conf.c      |   10 +++++++---
>>  src/conf/network_conf.h      |    2 +-
>>  src/network/bridge_driver.c  |    4 ++--
>>  src/test/test_driver.c       |    2 +-
>>  src/vbox/vbox_tmpl.c         |    2 +-
>>  tests/networkxml2xmltest.c   |    2 +-
>>  tools/virsh.c                |   13 +++++++++++--
>>  8 files changed, 28 insertions(+), 11 deletions(-)
> 
> In addition to Daniel's comments, you are missing documentation for the
> new flag; src/libvirt.c needs to mention the valid 'flags' value, and
> tools/virsh.pod needs to document the new code.
> 
> I'm squashing this to your last patch, then rebasing things slightly
> (I'm moving portions of network_conf.c that format pf on output to patch
> 3, to parallel parsing it on input, so that this patch is only left with
> adding flags support to network_conf.c), then pushing the series.
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 21b2d0a..009479c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1080,14 +1080,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> 
>          /* all of these modes can use a pool of physical interfaces */
>          nForwardIfs = virXPathNodeSet("./interface", ctxt,
> &forwardIfNodes);
> -        if (nForwardIfs <= 0) {
> -            nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes);
> +        nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes);
> 
> -            if (nForwardPfs < 0) {
> -                virNetworkReportError(VIR_ERR_XML_ERROR,
> -                                      _("No interface pool or SRIOV
> physical device given"));
> -                goto error;
> -            }
> +        if (nForwardIfs < 0 || nForwardPfs < 0) {
> +            virNetworkReportError(VIR_ERR_XML_ERROR,
> +                                  _("No interface pool or SRIOV
> physical device given"));
> +            goto error;
>          }
> 
>          if (nForwardPfs == 1) {
> @@ -1118,7 +1116,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>              virNetworkReportError(VIR_ERR_XML_ERROR,
>                                    _("Use of more than one physical
> interface is not allowed"));
>              goto error;
> -        } else if ((nForwardIfs > 0) || forwardDev) {
> +        }
> +        if (nForwardIfs > 0 || forwardDev) {
>              int ii;
> 
>              /* allocate array to hold all the portgroups */
> @@ -1465,7 +1464,7 @@ char *virNetworkDefFormat(const virNetworkDefPtr
> def, unsigned int flags)
> 
>      if (def->forwardType != VIR_NETWORK_FORWARD_NONE) {
>          const char *dev = NULL;
> -        if (def->nForwardPfs < 1)
> +        if (!def->nForwardPfs)
>              dev = virNetworkDefForwardIf(def, 0);
>          const char *mode = virNetworkForwardTypeToString(def->forwardType);
> 
> @@ -1476,31 +1475,24 @@ char *virNetworkDefFormat(const virNetworkDefPtr
> def, unsigned int flags)
>              goto error;
>          }
>          virBufferAddLit(&buf, "  <forward");
> -        if (dev)
> -            virBufferEscapeString(&buf, " dev='%s'", dev);
> +        virBufferEscapeString(&buf, " dev='%s'", dev);
>          virBufferAsprintf(&buf, " mode='%s'%s>\n", mode,
> -                          def->nForwardIfs ? "" : "/");
> +                          (def->nForwardIfs || def->nForwardPfs) ? "" :
> "/");
> 
> -        if (def->nForwardPfs) {
> -            if (def->forwardPfs->dev) {
> -                virBufferEscapeString(&buf, "    <pf dev='%s'/>\n",
> -                                      def->forwardPfs[0].dev);
> -            }
> -        }
> +        /* For now, hard-coded to at most 1 forwardPfs */
> +        if (def->forwardPfs)
> +            virBufferEscapeString(&buf, "    <pf dev='%s'/>\n",
> +                                  def->forwardPfs[0].dev);
> 
> -        if (flags && def->nForwardPfs)
> -            goto escape;
> -
> -        if (def->nForwardIfs) {
> +        if (def->nForwardIfs &&
> +            (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) {
>              for (ii = 0; ii < def->nForwardIfs; ii++) {
> -                if (def->forwardIfs[ii].dev) {
> -                    virBufferEscapeString(&buf, "    <interface
> dev='%s'/>\n",
> -                                          def->forwardIfs[ii].dev);
> -                }
> +                virBufferEscapeString(&buf, "    <interface dev='%s'/>\n",
> +                                      def->forwardIfs[ii].dev);
>              }
>          }
> -    escape:
> -        virBufferAddLit(&buf, "  </forward>\n");
> +        if (def->nForwardPfs || def->nForwardIfs)
> +            virBufferAddLit(&buf, "  </forward>\n");
>      }
> 
>      if (def->forwardType == VIR_NETWORK_FORWARD_NONE ||
> diff --git a/src/libvirt.c b/src/libvirt.c
> index dbf0296..a540424 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -9835,11 +9835,16 @@ error:
>  /**
>   * virNetworkGetXMLDesc:
>   * @network: a network object
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virNetworkXMLFlags
>   *
>   * Provide an XML description of the network. The description may be reused
>   * later to relaunch the network with virNetworkCreateXML().
>   *
> + * Normally, if a network included a physical function, the output includes
> + * all virtual functions tied to that physical interface.  If @flags
> includes
> + * VIR_NETWORK_XML_INACTIVE, then the expansion of virtual interfaces is
> + * not performed.
> + *
>   * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
> of error.
>   *         the caller must free() the returned value.
>   */
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 1b5c0b8..2a98e7e 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1,7 +1,7 @@
>  /*
>   * test.c: A "mock" hypervisor for use by application unit tests
>   *
> - * Copyright (C) 2006-2011 Red Hat, Inc.
> + * Copyright (C) 2006-2012 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
> index 4249caa..6cb9d90 100644
> --- a/tests/networkxml2xmltest.c
> +++ b/tests/networkxml2xmltest.c
> @@ -14,7 +14,8 @@
>  #include "testutilsqemu.h"
> 
>  static int
> -testCompareXMLToXMLFiles(const char *inxml, const char *outxml)
> +testCompareXMLToXMLFiles(const char *inxml, const char *outxml,
> +                         unsigned int flags)
>  {
>      char *inXmlData = NULL;
>      char *outXmlData = NULL;
> @@ -30,7 +31,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char
> *outxml)
>      if (!(dev = virNetworkDefParseString(inXmlData)))
>          goto fail;
> 
> -    if (!(actual = virNetworkDefFormat(dev, 0)))
> +    if (!(actual = virNetworkDefFormat(dev, flags)))
>          goto fail;
> 
>      if (STRNEQ(outXmlData, actual)) {
> @@ -48,21 +49,27 @@ testCompareXMLToXMLFiles(const char *inxml, const
> char *outxml)
>      return ret;
>  }
> 
> +struct testInfo {
> +    const char *name;
> +    unsigned int flags;
> +};
> +
>  static int
>  testCompareXMLToXMLHelper(const void *data)
>  {
> +    const struct testInfo *info = data;
>      int result = -1;
>      char *inxml = NULL;
>      char *outxml = NULL;
> 
>      if (virAsprintf(&inxml, "%s/networkxml2xmlin/%s.xml",
> -                    abs_srcdir, (const char*)data) < 0 ||
> +                    abs_srcdir, info->name) < 0 ||
>          virAsprintf(&outxml, "%s/networkxml2xmlout/%s.xml",
> -                    abs_srcdir, (const char*)data) < 0) {
> +                    abs_srcdir, info->name) < 0) {
>          goto cleanup;
>      }
> 
> -    result = testCompareXMLToXMLFiles(inxml, outxml);
> +    result = testCompareXMLToXMLFiles(inxml, outxml, info->flags);
> 
>  cleanup:
>      free(inxml);
> @@ -76,10 +83,14 @@ mymain(void)
>  {
>      int ret = 0;
> 
> -#define DO_TEST(name) \
> -    if (virtTestRun("Network XML-2-XML " name, \
> -                    1, testCompareXMLToXMLHelper, (name)) < 0) \
> -        ret = -1
> +#define DO_TEST_FULL(name, flags)                                       \
> +    do {                                                                \
> +        const struct testInfo info = {name, flags};                     \
> +        if (virtTestRun("Network XML-2-XML " name,                      \
> +                        1, testCompareXMLToXMLHelper, &info) < 0)       \
> +            ret = -1;                                                   \
> +    } while (0)
> +#define DO_TEST(name) DO_TEST_FULL(name, 0)
> 
>      DO_TEST("isolated-network");
>      DO_TEST("routed-network");
> @@ -93,6 +104,7 @@ mymain(void)
>      DO_TEST("host-bridge-net");
>      DO_TEST("vepa-net");
>      DO_TEST("bandwidth-network");
> +    DO_TEST_FULL("passthrough-pf", VIR_NETWORK_XML_INACTIVE);
> 
>      return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
>  }
> diff --git a/tools/virsh.c b/tools/virsh.c
> index ba07e0f..3869d9d 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -7298,15 +7298,14 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd
> *cmd)
>      char *dump;
>      unsigned int flags = 0;
>      int inactive;
> -
> +
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return false;
> 
>      if (!(network = vshCommandOptNetwork(ctl, cmd, NULL)))
>          return false;
> -
> +
>      inactive = vshCommandOptBool (cmd, "inactive");
> -
>      if (inactive)
>          flags |= VIR_NETWORK_XML_INACTIVE;
> 
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 1abf448..f2793cd 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1523,9 +1523,11 @@ not instantiated.
>  Destroy (stop) a given virtual network specified by its name or UUID. This
>  takes effect immediately.
> 
> -=item B<net-dumpxml> I<network>
> +=item B<net-dumpxml> I<network> [I<--inactive>]
> 
>  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.
> 
>  =item B<net-edit> I<network>
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]