On 11/17/20 1:48 AM, 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@massclouds.com> --- src/util/virnetdevveth.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index b3eee1af..996bf5dd 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 @@ -116,7 +117,6 @@ int virNetDevVethCreate(char** veth1, char** veth2) for (i = 0; i < MAX_VETH_RETRIES; i++) { g_autofree char *veth1auto = NULL; g_autofree char *veth2auto = NULL; - g_autoptr(virCommand) cmd = NULL; int status; if (!*veth1) { @@ -136,15 +136,32 @@ int virNetDevVethCreate(char** veth1, char** veth2) vethNum = veth2num + 1; } - cmd = virCommandNew("ip"); - virCommandAddArgList(cmd, "link", "add", - *veth1 ? *veth1 : veth1auto, - "type", "veth", "peer", "name", - *veth2 ? *veth2 : veth2auto, - NULL); +#if defined(WITH_LIBNL) + { + int error = 0; + virNetlinkNewLinkData data = { + .veth_peer = *veth2 ? *veth2 : veth2auto, + }; - if (virCommandRun(cmd, &status) < 0) - goto cleanup; + status = virNetlinkNewLink(*veth1 ? *veth1 : veth1auto, + "veth", &data, &error); + if (status < 0) + goto cleanup; + } +#else + { + g_autoptr(virCommand) cmd = NULL; + cmd = virCommandNew("ip"); + virCommandAddArgList(cmd, "link", "add", + *veth1 ? *veth1 : veth1auto, + "type", "veth", "peer", "name", + *veth2 ? *veth2 : veth2auto, + NULL); + + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + } +#endif /* WITH_LIBNL */
Although it isn't a hard/fast rule, we generally prefer to have complete functions within an #ifdefs rather then putting #ifdefs in the middle of a function. Could you put these bits into smaller functions, patterned like below, and then call the vir*Internal() functions from the existing functions?
(other than that this looks fine, although I haven't actually tried *running* it yet :-))
Okay. It looks fine.
#if defined(WITH_LIBNL)
int
virNetDevVethCreateInternal(char *veth1, char *veth2)
{
int error;
virNetlinkNewLinkData data = { .veth_peer = veth2 };
return virNetlinkNewLink(veth1, "veth", &data, &error);
}
int
virNetDevVethDeleteInternal(char *veth)
{
return virNetlinkDelLink(veth, NULL);
}
#else
int
virNetDevVethCreateInternal(char *veth1, char *veth2)
{
g_autoptr(virCommand) cmd = virCommandNew("ip");
virCommandAddArgList(cmd, "link", "add", veth1, veth2);
return virCommandRun(cmd, NULL);
}
int
virNetDevVethDeleteInternal(char *veth)
{
int status; g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link", "del", veth, NULL);
if (virCommandRun(cmd, &status) < 0) return -1;
if (status != 0) { if (!virNetDevExists(veth)) { VIR_DEBUG("Device %s already deleted (by kernel namespace cleanup)", veth); return 0; } virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to delete veth device %s"), veth); return -1; }
return 0; }
if (status == 0) { if (veth1auto) { @@ -188,6 +205,9 @@ int virNetDevVethCreate(char** veth1, char** veth2) */ int virNetDevVethDelete(const char *veth) { +#if defined(WITH_LIBNL) + return virNetlinkDelLink(veth, NULL); +#else int status; g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link", "del", veth, NULL); @@ -206,4 +226,5 @@ int virNetDevVethDelete(const char *veth) } return 0; +#endif /* WITH_LIBNL */ }
(Aside from this, it looks like veth interface naming could benefit from the same simplifications that I did to tap and macvtap awhile back (basically changing the auto-generated names to never re-use a name - commit 95089f481 and commit d7f38beb2e). But that isn't something for this series, just another thing to add to the to-do list :-))
Oh. Perhaps I can try it in another series. Thanks! Shi Lei