9:28 p.m.
V1 here: https://www.redhat.com/archives/libvir-list/2020-November/msg00898.html
Since V1:
* Include <linux/veth.h> rather than introducing enum VETH_INFO_*
* Have complete functions within an #ifdefs rather than
putting #ifdefs in function
Shi Lei (2):
util:netlink: Enable virNetlinkNewLink to support veth
util:veth: Create veth device pair by netlink
src/util/virnetdevveth.c | 85 ++++++++++++++++++++++++++--------------
src/util/virnetlink.c | 14 +++++++
src/util/virnetlink.h | 1 +
3 files changed, 71 insertions(+), 29 deletions(-)
--
2.25.1
9:28 p.m.
New subject: [PATCHv2 1/2] util:netlink: Enable virNetlinkNewLink to support veth
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/util/virnetlink.c | 14 ++++++++++++++
src/util/virnetlink.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fdd3a6a4..8625b896 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -24,6 +24,7 @@
#include <config.h>
#include <unistd.h>
+#include <linux/veth.h>
#include "virnetlink.h"
#include "virnetdev.h"
@@ -535,6 +536,19 @@ virNetlinkNewLink(const char *ifname,
NETLINK_MSG_NEST_END(nl_msg, infodata);
}
+ if (STREQ(type, "veth") && extra_args &&
extra_args->veth_peer) {
+ struct nlattr *infoveth = NULL;
+
+ NETLINK_MSG_NEST_START(nl_msg, infodata, IFLA_INFO_DATA);
+ NETLINK_MSG_NEST_START(nl_msg, infoveth, VETH_INFO_PEER);
+ nlmsg_reserve(nl_msg, sizeof(struct ifinfomsg), 0);
+ NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME,
+ (strlen(extra_args->veth_peer) + 1),
+ extra_args->veth_peer);
+ NETLINK_MSG_NEST_END(nl_msg, infoveth);
+ NETLINK_MSG_NEST_END(nl_msg, infodata);
+ }
+
NETLINK_MSG_NEST_END(nl_msg, linkinfo);
if (extra_args) {
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 7121eac4..7c4ed202 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -84,6 +84,7 @@ struct _virNetlinkNewLinkData {
const int *ifindex; /* The index for the 'link' device */
const virMacAddr *mac; /* The MAC address of the device */
const uint32_t *macvlan_mode; /* The mode of macvlan */
+ const char *veth_peer; /* The peer name for veth */
};
int virNetlinkNewLink(const char *ifname,
--
2.25.1
9:28 p.m.
New subject: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
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);
+}
+
+static int
+virNetDevVethDeleteInternal(const char *veth)
+{
+ return virNetlinkDelLink(veth, NULL);
+}
+#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);
+}
+
+static int
+virNetDevVethDeleteInternal(const 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;
+}
+#endif /* WITH_LIBNL */
+
/**
* virNetDevVethCreate:
* @veth1: pointer to name for parent end of veth pair
@@ -100,7 +150,7 @@ static int virNetDevVethGetFreeNum(int startDev)
*
* Returns 0 on success or -1 in case of error
*/
-int virNetDevVethCreate(char** veth1, char** veth2)
+int virNetDevVethCreate(char **veth1, char **veth2)
{
int ret = -1;
int vethNum = 0;
@@ -116,9 +166,8 @@ 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;
+ int status = 0;
if (!*veth1) {
int veth1num;
if ((veth1num = virNetDevVethGetFreeNum(vethNum)) < 0)
@@ -136,14 +185,9 @@ 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 (virCommandRun(cmd, &status) < 0)
+ if (virNetDevVethCreateInternal(*veth1 ? *veth1 : veth1auto,
+ *veth2 ? *veth2 : veth2auto,
+ &status) < 0)
goto cleanup;
if (status == 0) {
@@ -188,22 +232,5 @@ int virNetDevVethCreate(char** veth1, char** veth2)
*/
int virNetDevVethDelete(const 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;
+ return virNetDevVethDeleteInternal(veth);
}
--
2.25.1
4:54 p.m.
New subject: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
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...
[...]
+#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...
1:33 a.m.
New subject: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
Okay. According to your comment, I will deal with another series first.
Then I get rid of that 'status' in this patch and post V3 of it.
Thanks!
Shi Lei
On 2020-12-08 at 06:54, 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...
> [...]
> +#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...
3:19 a.m.
New subject: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
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 :|
9:04 a.m.
New subject: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
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(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.
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.
1475
days inactive
1490
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Shi Lei