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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.

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.





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux