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@xxxxxxxxxxxxxx>
> > ---
> > 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 :|
