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

[Shi Lei]

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

Okay. It looks fine.

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

Oh. Perhaps I can try it in another series.

Thanks!

Shi Lei