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

[Laine Stump]

On 12/8/20 4:19 AM, Daniel P. Berrangé wrote:
> On Mon, Dec 07, 2020 at 05:54:51PM -0500, 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...
> 
> Just change this to return -1, as we very rarely want to return -errno
> in libvirt, as we have formal error reporting.

I guess I should give a bit more detail here - the "-errno" returned by 
virNetlinkNewLink() isn't the value of errno in the libvirt process at 
the time it learns there was an error - it is the value of errno 
returned in the netlink response message, ie what errno was set to in 
the kernel context that is handling and responding to netlink messages. 
errno in the libvirt process will not be set to this value.

And since the libvirt functions that are decoding the response messages 
don't themselves log any errors, they aren't reporting that exact error...

...

Hmmm. this points out that we really need to be logging the exact error 
in virNetDevVethCreateInternal(), since once we return from that 
function we no longer have full info on the reason for the failure. This 
would have been a problem if we had any inclination to retry the 
operation (with a new device name, which the current code must do), but 
once the other series from Shi is applied (the one that changes veth 
name generation to mimic what is done for tap and macvtap), we won't 
need to worry about retrying, and so it will be acceptable to 
immediately log the error.


> 
>>
>>
>>> [...]
>>
>>> +#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.

Also in this case if status != NULL then virCommandRun() will only log 
an error if it failed to run the "ip" command; if ip runs but fails to 
create the device and returns an error exit code, virCommandRun() will 
silently return the exit code. In this case, we can just call 
"virCommandRun(cmd, NULL)" - when status is NULL, virCommandRun() will 
log an error if the exec fails, and also if the exec is successful but 
the command returns an error.

And since I'm already here typing - when I reviewed the other series 
(the ones that change the device name generation) I did arrive at the 
opinion that we should apply those patches first, and these patches on 
top of that - that way you won't have to worry about the possibility of 
needing to retry virNetDevVethCreateInternal with a new name, and it 
will be okay for its implementations to immediately log errors.