[PATCH 2/2] util:veth: Create veth device pair by netlink

[Laine Stump]

On 11/17/20 1:48 AM, Shi Lei wrote:
> When netlink is supported, use netlink to create veth device pair
> rather than 'ip link' command.
>
> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> ---
>   src/util/virnetdevveth.c | 39 ++++++++++++++++++++++++++++++---------
>   1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
> index b3eee1af..996bf5dd 100644
> --- a/src/util/virnetdevveth.c
> +++ b/src/util/virnetdevveth.c
> @@ -27,6 +27,7 @@
>   #include "virfile.h"
>   #include "virstring.h"
>   #include "virnetdev.h"
> +#include "virnetlink.h"
>   
>   #define VIR_FROM_THIS VIR_FROM_NONE
>   
> @@ -116,7 +117,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
>       for (i = 0; i < MAX_VETH_RETRIES; i++) {
>           g_autofree char *veth1auto = NULL;
>           g_autofree char *veth2auto = NULL;
> -        g_autoptr(virCommand) cmd = NULL;
>   
>           int status;
>           if (!*veth1) {
> @@ -136,15 +136,32 @@ int virNetDevVethCreate(char** veth1, char** veth2)
>               vethNum = veth2num + 1;
>           }
>   
> -        cmd = virCommandNew("ip");
> -        virCommandAddArgList(cmd, "link", "add",
> -                             *veth1 ? *veth1 : veth1auto,
> -                             "type", "veth", "peer", "name",
> -                             *veth2 ? *veth2 : veth2auto,
> -                             NULL);
> +#if defined(WITH_LIBNL)
> +        {
> +            int error = 0;
> +            virNetlinkNewLinkData data = {
> +                .veth_peer = *veth2 ? *veth2 : veth2auto,
> +            };
>   
> -        if (virCommandRun(cmd, &status) < 0)
> -            goto cleanup;
> +            status = virNetlinkNewLink(*veth1 ? *veth1 : veth1auto,
> +                                       "veth", &data, &error);
> +            if (status < 0)
> +                goto cleanup;
> +        }
> +#else
> +        {
> +            g_autoptr(virCommand) cmd = NULL;
> +            cmd = virCommandNew("ip");
> +            virCommandAddArgList(cmd, "link", "add",
> +                                 *veth1 ? *veth1 : veth1auto,
> +                                 "type", "veth", "peer", "name",
> +                                 *veth2 ? *veth2 : veth2auto,
> +                                 NULL);
> +
> +            if (virCommandRun(cmd, &status) < 0)
> +                goto cleanup;
> +        }
> +#endif /* WITH_LIBNL */


Although it isn't a hard/fast rule, we generally prefer to have complete 
functions within an #ifdefs rather then putting #ifdefs in the middle of 
a function. Could you put these bits into smaller functions, patterned 
like below, and then call the vir*Internal() functions from the existing 
functions?

(other than that this looks fine, although I haven't actually tried 
*running* it yet :-))


#if defined(WITH_LIBNL)

int

virNetDevVethCreateInternal(char *veth1, char *veth2)

{

     int error;

     virNetlinkNewLinkData data = { .veth_peer = veth2 };


     return virNetlinkNewLink(veth1, "veth", &data, &error);

}



int

virNetDevVethDeleteInternal(char *veth)

{

     return virNetlinkDelLink(veth, NULL);

}



#else



int

virNetDevVethCreateInternal(char *veth1, char *veth2)

{

     g_autoptr(virCommand) cmd = virCommandNew("ip");


     virCommandAddArgList(cmd, "link", "add", veth1, veth2);

     return virCommandRun(cmd, NULL);

}



int

virNetDevVethDeleteInternal(char *veth)

{

     int status;
     g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link", 
"del", veth, NULL);

     if (virCommandRun(cmd, &status) < 0)
         return -1;

     if (status != 0) {
         if (!virNetDevExists(veth)) {
             VIR_DEBUG("Device %s already deleted (by kernel namespace 
cleanup)", veth);
             return 0;
         }
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to delete veth device %s"), veth);
         return -1;
     }

     return 0;
}


>   
>           if (status == 0) {
>               if (veth1auto) {
> @@ -188,6 +205,9 @@ int virNetDevVethCreate(char** veth1, char** veth2)
>    */
>   int virNetDevVethDelete(const char *veth)
>   {
> +#if defined(WITH_LIBNL)
> +    return virNetlinkDelLink(veth, NULL);
> +#else
>       int status;
>       g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link",
>                                                          "del", veth, NULL);
> @@ -206,4 +226,5 @@ int virNetDevVethDelete(const char *veth)
>       }
>   
>       return 0;
> +#endif /* WITH_LIBNL */
>   }


(Aside from this, it looks like veth interface naming could benefit from 
the same simplifications that I did to tap and macvtap awhile back 
(basically changing the auto-generated names to never re-use a name - 
commit 95089f481 and commit d7f38beb2e). But that isn't something for 
this series, just another thing to add to the to-do list :-))