8:06 p.m.
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(a)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