Re: [PATCH 14/28] util: move IP route & address object-related functions to virnetdevip.c

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

 



On 06/24/2016 07:11 AM, John Ferlan wrote:

On 06/22/2016 01:37 PM, Laine Stump wrote:
These functions all need to be called from a utility function that
must be located in the util directory, so we move them all into
util/virnetdevip.[ch] now that it exists.

Function and struct names were appropriately changed for the new
location, but all code is unchanged aside from motion and renaming.
---
  src/conf/domain_conf.c            |  36 ++++++-------
  src/conf/domain_conf.h            |  16 ++----
  src/conf/network_conf.c           |  16 +++---
  src/conf/network_conf.h           |   4 +-
  src/conf/networkcommon_conf.c     | 107 ++++----------------------------------
  src/conf/networkcommon_conf.h     |  55 +++++++-------------
  src/libvirt_private.syms          |  16 +++---
  src/lxc/lxc_container.c           |  12 ++---
  src/lxc/lxc_native.c              |  12 ++---
  src/network/bridge_driver.c       |  14 ++---
  src/network/bridge_driver_linux.c |   6 +--
  src/util/virnetdevip.c            |  69 ++++++++++++++++++++++++
  src/util/virnetdevip.h            |  29 +++++++++++
  13 files changed, 191 insertions(+), 201 deletions(-)

The one naming thing that "could" have changed as well is to keep the
"Def" portion (virNetDevIPRouteDefFree, virNetDevIPRouteDefParseXML,
virNetDevIPRouteDefFormat, virNetDevIPRouteDefCreate).  Generally I'd
say it's a coin flip, but to be consistent since they're handling the
virNetworkRouteDefPtr, then I guess without "knowing" the API names I'd
start searching "NetworkRouteDef{Parse|Format}" in order to find the
code that dealt with it (the libvirt consistency argument).

But when the structures get below a certain level of complexity (or maybe it's that they're nested deep enough, dunno), they tend to lose the Def suffix - virNetDevBandwith, virNetDevVlan virDomainDeviceInfo virNetDevVPortProfile...


Maybe they'll happen in a later patch, but why not move the Create,
Parse and Format routines as well?

except that Parse and Format functions are traditionally kept in the conf directory. As for the Create, I remember thinking about that one, but on the first glance it just had so much test that it seemed more like something from the conf directory. Probably not the right choice, but I can make a patch to move it later.


  That'd remove networkcommon_conf it
seems.  Could really gone for overkill and generated
virnetdeviproute.{h,c} ~/~

Yeah, there's a few gigantic files, then several tiny ones. It would be nice if they were all in the middle somewhere.


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4802e03..f380271 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
[...]
@@ -6266,9 +6266,9 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED,
          if (nroutenodes) {
              size_t i;
              for (i = 0; i < nroutenodes; i++) {
-                virNetworkRouteDefPtr route = NULL;
+                virNetDevIPRoutePtr route = NULL;
- if (!(route = virNetworkRouteDefParseXML(_("Domain hostdev device"),
+                if (!(route = virNetDevIPRouteParseXML(_("Domain hostdev device"),
                                                           routenodes[i],
                                                           ctxt)))
The arguments need vertical alignment on line 2 and 3 under the _...

[...]

@@ -8955,9 +8955,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
      int ret, val;
      size_t i;
      size_t nips = 0;
-    virDomainNetIPDefPtr *ips = NULL;
+    virNetDevIPAddrPtr *ips = NULL;
      size_t nroutes = 0;
-    virNetworkRouteDefPtr *routes = NULL;
+    virNetDevIPRoutePtr *routes = NULL;
if (VIR_ALLOC(def) < 0)
          return NULL;
@@ -9074,7 +9074,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
                      ctxt->node = tmpnode;
                  }
              } else if (xmlStrEqual(cur->name, BAD_CAST "ip")) {
-                virDomainNetIPDefPtr ip = NULL;
+                virNetDevIPAddrPtr ip = NULL;
if (!(ip = virDomainNetIPParseXML(cur)))
                      goto error;
@@ -9082,13 +9082,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
                  if (VIR_APPEND_ELEMENT(ips, nips, ip) < 0)
                      goto error;
              } else if (xmlStrEqual(cur->name, BAD_CAST "route")) {
-                virNetworkRouteDefPtr route = NULL;
-                if (!(route = virNetworkRouteDefParseXML(_("Domain interface"),
+                virNetDevIPRoutePtr route = NULL;
+                if (!(route = virNetDevIPRouteParseXML(_("Domain interface"),
                                                           cur, ctxt)))
Vertical alignment...


[...]


  /* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 5ae2bdf..fb2a48d 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
[...]

@@ -2261,9 +2261,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
              goto error;
          /* parse each definition */
          for (i = 0; i < nRoutes; i++) {
-            virNetworkRouteDefPtr route = NULL;
+            virNetDevIPRoutePtr route = NULL;
- if (!(route = virNetworkRouteDefParseXML(def->name,
+            if (!(route = virNetDevIPRouteParseXML(def->name,
                                                       routeNodes[i],
                                                       ctxt)))
Vertical alignment

[...]

--- a/src/conf/networkcommon_conf.c
+++ b/src/conf/networkcommon_conf.c
@@ -32,34 +32,8 @@
[...]

-virNetworkRouteDefPtr
-virNetworkRouteDefCreate(const char *errorDetail,
+virNetDevIPRoutePtr
+virNetDevIPRouteCreate(const char *errorDetail,
                           char *family,
                           const char *address,
                           const char *netmask,
@@ -69,7 +43,7 @@ virNetworkRouteDefCreate(const char *errorDetail,
                           unsigned int metric,
                           bool hasMetric)
Vertical alignment

  {
-    virNetworkRouteDefPtr def = NULL;
+    virNetDevIPRoutePtr def = NULL;
      virSocketAddr testAddr;
if (VIR_ALLOC(def) < 0)
@@ -242,21 +216,21 @@ virNetworkRouteDefCreate(const char *errorDetail,
      return def;
error:
-    virNetworkRouteDefFree(def);
+    virNetDevIPRouteFree(def);
      return NULL;
  }
-virNetworkRouteDefPtr
-virNetworkRouteDefParseXML(const char *errorDetail,
+virNetDevIPRoutePtr
+virNetDevIPRouteParseXML(const char *errorDetail,
                             xmlNodePtr node,
                             xmlXPathContextPtr ctxt)
Vertical alignment

  {
      /*
-     * virNetworkRouteDef object is already allocated as part
+     * virNetDevIPRoute object is already allocated as part
       * of an array.  On failure clear: it out, but don't free it.
       */
- virNetworkRouteDefPtr def = NULL;
+    virNetDevIPRoutePtr def = NULL;
      xmlNodePtr save;
      char *family = NULL;
      char *address = NULL, *netmask = NULL;
@@ -302,7 +276,7 @@ virNetworkRouteDefParseXML(const char *errorDetail,
          }
      }
- def = virNetworkRouteDefCreate(errorDetail, family, address, netmask,
+    def = virNetDevIPRouteCreate(errorDetail, family, address, netmask,
                                     gateway, prefix, hasPrefix, metric,
                                     hasMetric);
Vertical alignment

@@ -316,8 +290,8 @@ virNetworkRouteDefParseXML(const char *errorDetail,
  }
int
-virNetworkRouteDefFormat(virBufferPtr buf,
-                         const virNetworkRouteDef *def)
+virNetDevIPRouteFormat(virBufferPtr buf,
+                         const virNetDevIPRoute *def)
Vertical alignment


[...]


diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 9ad1b08..8294d29 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -419,7 +419,7 @@ typedef struct {
      char *macvlanmode;
      char *vlanid;
      char *name;
-    virDomainNetIPDefPtr *ips;
+    virNetDevIPAddrPtr *ips;
      size_t nips;
      char *gateway_ipv4;
      char *gateway_ipv6;
@@ -430,10 +430,10 @@ typedef struct {
  static int
  lxcAddNetworkRouteDefinition(const char *address,
                               int family,
-                             virNetworkRouteDefPtr **routes,
+                             virNetDevIPRoutePtr **routes,
                               size_t *nroutes)
  {
-    virNetworkRouteDefPtr route = NULL;
+    virNetDevIPRoutePtr route = NULL;
      char *familyStr = NULL;
      char *zero = NULL;
@@ -444,7 +444,7 @@ lxcAddNetworkRouteDefinition(const char *address,
      if (VIR_STRDUP(familyStr, family == AF_INET ? "ipv4" : "ipv6") < 0)
          goto error;
- if (!(route = virNetworkRouteDefCreate(_("Domain interface"), familyStr,
+    if (!(route = virNetDevIPRouteCreate(_("Domain interface"), familyStr,
                                            zero, NULL, address, 0, false,
                                            0, false)))
Vertical alignment

          goto error;
@@ -460,7 +460,7 @@ lxcAddNetworkRouteDefinition(const char *address,
   error:
[...]


ACK - I think the Def should be added back in and the vertical alignment
things fixed.  A quick scan from yesterday and I found just one instance
in patch 2, src/conf/interface_conf.c for _virInterfaceDef and the
comment column off by 1 vertical char (no big deal).

John


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]