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

[Shi Lei]

Okay. According to your comment, I will deal with another series first.
Then I get rid of that 'status' in this patch and post V3 of it.
Thanks!

Shi Lei

On 2020-12-08 at 06:54, Laine Stump wrote:
>On 11/22/20 10:28 PM, 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 | 85 ++++++++++++++++++++++++++--------------
>>   1 file changed, 56 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
>> index b3eee1af..b4074371 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
>>  
>> @@ -75,6 +76,55 @@ static int virNetDevVethGetFreeNum(int startDev)
>>       return -1;
>>   }
>>  
>> +#if defined(WITH_LIBNL)
>> +static int
>> +virNetDevVethCreateInternal(const char *veth1, const char *veth2, int *status)
>> +{
>> +    virNetlinkNewLinkData data = { .veth_peer = veth2 };
>> +
>> +    return virNetlinkNewLink(veth1, "veth", &data, status);
>> +}
>
>The only thing that makes me uncomfortable in this patch is that the two
>versions of virNetDevVethCreateInternal() each return something
>different for "status". In this first case, it is returning 0 on
>success, and -errno on failure...
>
>
>> [...]
>
>> +#else
>> +static int
>> +virNetDevVethCreateInternal(const char *veth1, const char *veth2, int *status)
>> +{
>> +    g_autoptr(virCommand) cmd = virCommandNew("ip");
>> +    virCommandAddArgList(cmd, "link", "add", veth1, "type", "veth",
>> +                         "peer", "name", veth2, NULL);
>> +
>> +    return virCommandRun(cmd, status);
>> +}
>
>
>But in this case it is returning the exit code of the "ip link add" command.
>
>
>It turns out that the value of status is only checked for 0 / not-0, so
>in practice it doesn't matter, but it could lead to confusion in the future.
>
>
>If we want these patches to be applied before your "netdevveth: Simplify
>virNetDevVethCreate by using virNetDevGenerateName" patch (which I'll
>get to as soon as I send this mail), then we do still need a way to
>differentiate between "The requested device already exists" and
>"Permanent Failure", and in that case I would suggest that "status" be
>replaced with a variable called "retry" which would be set to true if
>retrying with a different device name might lead to success (it would be
>set based on an interpretation of status made by each of the
>vir*Internal() functions)
>
>
>However, if we decide to apply the *other* patchset first, then we will
>never retry anyway (because we've already checked if the device exists
>before we try to create it, and there is therefore no loop) and so we
>could just eliminate the final argument completely, and keep each
>vir*Internal() function's status internal to itself. (As a matter of
>fact, status in the virCommandRun() version could simply be replaced
>with NULL in the arglist to virCommandRun(), and not defined at all;
>status in the case of virNetlinkNewLink() would still need to be defined
>and passed into the function, but its value would just be ignored).
>
>
>I think it may be cleaner to do the latter, and it looks like your other
>series was written to be applied without this series anyway; let me look
>at those patches and I'll reply a 2nd time to this based on the results
>of reviewing the other series...
>
>
>BTW, I appreciate your patience - I had looked at these patches nearly
>two weeks ago (soon after you sent them), but then a holiday got in the
>way, and I forgot to post my reply after I returned to work :-/
>
>
>> [...]
>> -
>> -        if (virCommandRun(cmd, &status) < 0)
>> +        if (virNetDevVethCreateInternal(*veth1 ? *veth1 : veth1auto,
>> +                                        *veth2 ? *veth2 : veth2auto,
>> +                                        &status) < 0)
>>               goto cleanup;
>>  
>>           if (status == 0) {
>
>
>This spot here ^ is the only place that status is examined, and I
>actually think it's not going to work as you'd like in the case of
>netlink - status will always be 0 if virNetDevVethCreateInternal()
>returns 0, and when the return is < 0, status may or may not be set to
>-errno of the attempted operation - if status is 0, that means there was
>a serious error before even trying to create the device (e.g. OOM), and
>if it is non-0, then it might be because a device with the desired name
>already exists, or it might be because we don't have enough privilege to
>create a new device.
>
>
>Anyway, let me look at the other patchset...
>