Re: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
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(a)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.