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.
> [...]
> +#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...
Regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|