[libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
Dmitrii Shcherbakov
dmitrii.shcherbakov at canonical.com
Wed Nov 17 13:47:11 UTC 2021
Hi Laine,
This is less effective than mocking netlink messages, however, even
with netlink messages responses would depend on providing the right
argument to trigger the right mock.
Even this testing was useful because I managed to make a mistake while
reworking one of the if statements and those tests at least trigger
the conditionals that check error codes.
As far as ideas go:
1. I could suggest returning the requestError coming from
virNetDevSendVfSetLinkRequest - so if there is a mistake in the mock
argument, the resulting error will be different from the one expected
in a test since there won't be a netdev on a system with a test name
and the real implementation of virNetDevSendVfSetLinkRequest will be
called.
2. Utilize netdevsim
https://github.com/torvalds/linux/blob/v5.15/drivers/net/netdevsim/netdev.c#L82-L109
Downsides:
* having to load the right kernel module (testing in docker containers
makes it difficult - I don't see any infrastructure in the current
test framework to do that);
* cannot test the EPERM case since nsim_set_vf_vlan does not return it
(it actually allows setting a VLAN for a simulated VF).
On the plus side, this doesn't require mocking netlink messages and
utilizes the netlink message sending/receiving infrastructure in
Libvirt in full.
3. Introduce netlink message mocking.
There will be some boilerplate to generate kernel-like responses or at
least partial messages that contain an error or lack thereof
(NLMSG_DONE, NLMSG_ERROR).
I'll start with (1) for now but I am open to discussing other
approaches further. I am certainly interested in having this tested on
every build so that it doesn't break for everybody else down the road.
Best Regards,
Dmitrii Shcherbakov
LP/oftc: dmitriis
On Wed, Nov 17, 2021 at 3:43 AM Laine Stump <laine at redhat.com> wrote:
>
> On 11/15/21 1:59 PM, Dmitrii Shcherbakov wrote:
> > [...]
> > diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
> > index aadbeb1ef4..bdaa94e83c 100644
> > --- a/tests/virnetdevtest.c
> > +++ b/tests/virnetdevtest.c
>
>
> Maybe I'm just looking at it too superficially, but it seems like these
> test cases are testing the test jig itself more than any of the library
> code (the one exception is that testVirNetDevSetVfConfig() checks that
> the real virNetDevSetVfConfig() returns success when setting the vlan
> failed after setting the MAC succeeded). I'm not saying that anything
> better could be accomplished without mocking netlink itself, just wonder
> if it's worth all this extra bulk for the small amount of testing of
> actual library code that's accomplished.
>
>
> > @@ -18,11 +18,17 @@
> >
> > #include <config.h>
> >
> > +#include "internal.h"
> > #include "testutils.h"
> >
> > +#include "virnetlink.h"
> > +
> > +#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
> > +
> > #ifdef __linux__
> >
> > -# include "virnetdev.h"
> > +# include "virmock.h"
> > +# include "virnetdevpriv.h"
> >
> > # define VIR_FROM_THIS VIR_FROM_NONE
> >
> > @@ -59,6 +65,211 @@ testVirNetDevGetLinkInfo(const void *opaque)
> > return 0;
> > }
> >
> > +int
> > +(*real_virNetDevSendVfSetLinkRequest)(const char *ifname, int vfInfoType,
> > + const void *payload, const size_t payloadLen);
> > +
> > +int
> > +(*real_virNetDevSetVfMac)(const char *ifname, int vf, const virMacAddr *macaddr, bool *allowRetry);
> > +
> > +int
> > +(*real_virNetDevSetVfVlan)(const char *ifname, int vf, int vlanid);
> > +
> > +static void
> > +init_syms(void)
> > +{
> > + VIR_MOCK_REAL_INIT(virNetDevSendVfSetLinkRequest);
> > + VIR_MOCK_REAL_INIT(virNetDevSetVfMac);
> > + VIR_MOCK_REAL_INIT(virNetDevSetVfVlan);
> > +}
> > +
> > +int
> > +virNetDevSetVfMac(const char *ifname, int vf,
> > + const virMacAddr *macaddr,
> > + bool *allowRetry)
> > +{
> > + init_syms();
> > +
> > + if (STREQ_NULLABLE(ifname, "fakeiface-macerror")) {
> > + return -1;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-altmacerror")) {
> > + return -2;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
> > + return -1;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
> > + return -1;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
> > + return 0;
> > + }
> > + return real_virNetDevSetVfMac(ifname, vf, macaddr, allowRetry);
> > +}
> > +
> > +int
> > +virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
> > +{
> > + init_syms();
> > +
> > + if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
> > + return -1;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
> > + return 0;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
> > + return 0;
> > + }
> > + return real_virNetDevSetVfVlan(ifname, vf, vlanid);
> > +}
> > +
> > +int
> > +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> > + const void *payload, const size_t payloadLen)
> > +{
> > + init_syms();
> > +
> > + if (STREQ_NULLABLE(ifname, "fakeiface-eperm")) {
> > + return -EPERM;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-eagain")) {
> > + return -EAGAIN;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-einval")) {
> > + return -EINVAL;
> > + } else if (STREQ_NULLABLE(ifname, "fakeiface-ok")) {
> > + return 0;
> > + }
> > + return real_virNetDevSendVfSetLinkRequest(ifname, vfInfoType, payload, payloadLen);
> > +}
> > +
> > +static int
> > +testVirNetDevSetVfMac(const void *opaque G_GNUC_UNUSED)
> > +{
> > + struct testCase {
> > + const char *ifname;
> > + const int vf_num;
> > + const virMacAddr macaddr;
> > + bool allow_retry;
> > + const int rc;
> > + };
> > + size_t i = 0;
> > + int rc = 0;
> > + struct testCase testCases[] = {
> > + { .ifname = "fakeiface-ok", .vf_num = 1,
> > + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = 0 },
> > + { .ifname = "fakeiface-ok", .vf_num = 2,
> > + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = 0 },
> > + { .ifname = "fakeiface-ok", .vf_num = 3,
> > + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = 0 },
> > + { .ifname = "fakeiface-ok", .vf_num = 4,
> > + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true, .rc = 0 },
> > + { .ifname = "fakeiface-eperm", .vf_num = 5,
> > + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -1 },
> > + { .ifname = "fakeiface-einval", .vf_num = 6,
> > + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -1 },
> > + { .ifname = "fakeiface-einval", .vf_num = 7,
> > + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = -2 },
> > + { .ifname = "fakeiface-einval", .vf_num = 8,
> > + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = -1 },
> > + { .ifname = "fakeiface-einval", .vf_num = 9,
> > + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true, .rc = -1 },
> > + };
> > +
> > + for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
> > + rc = virNetDevSetVfMac(testCases[i].ifname, testCases[i].vf_num,
> > + &testCases[i].macaddr, &testCases[i].allow_retry);
> > + if (rc != testCases[i].rc) {
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int
> > +testVirNetDevSetVfMissingMac(const void *opaque G_GNUC_UNUSED)
> > +{
> > + bool allowRetry = false;
> > + /* NULL MAC pointer. */
> > + if (virNetDevSetVfMac("fakeiface-ok", 1, NULL, &allowRetry) != -1) {
> > + return -1;
> > + }
> > + allowRetry = true;
> > + if (virNetDevSetVfMac("fakeiface-ok", 1, NULL, &allowRetry) != -1) {
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int
> > +testVirNetDevSetVfVlan(const void *opaque G_GNUC_UNUSED)
> > +{
> > + struct testCase {
> > + const char *ifname;
> > + const int vf_num;
> > + const int vlan_id;
> > + const int rc;
> > + };
> > + size_t i = 0;
> > + int rc = 0;
> > + const struct testCase testCases[] = {
> > + /* VLAN ID is out of range of valid values (0-4095). */
> > + { .ifname = "enxdeadbeefcafe", .vf_num = 1, .vlan_id = 4096, .rc = -1 },
> > + { .ifname = "enxdeadbeefcafe", .vf_num = 1, .vlan_id = -1, .rc = -1 },
> > + /* Requests with vlan id 0 (trying to clear a VLAN that return EPERM should
> > + * not result in an error. They simply need to be ignored because it may
> > + * indicate that the NIC switch functionality is not exposed to the host. */
> > + { .ifname = "fakeiface-eperm", .vf_num = 1, .vlan_id = 0, .rc = 0 },
> > + /* Requests with vlan id 0 with an error output different from EPERM are failures. */
> > + { .ifname = "fakeiface-eagain", .vf_num = 1, .vlan_id = 0, .rc = -1 },
> > + /* Successful requests with vlan id 0 need to have a zero return code. */
> > + { .ifname = "fakeiface-ok", .vf_num = 1, .vlan_id = 0, .rc = 0 },
> > + /* Requests with a non-zero VLAN ID that result in an EPERM need to result in failures.
> > + * failures. */
> > + { .ifname = "fakeiface-eperm", .vf_num = 1, .vlan_id = 42, .rc = -1 },
> > + /* Requests with a non-zero VLAN ID that result in some other errors need to result in
> > + * failures. */
> > + { .ifname = "fakeiface-eagain", .vf_num = 1, .vlan_id = 42, .rc = -1 },
> > + /* Successful requests with a non-zero VLAN ID */
> > + { .ifname = "fakeiface-ok", .vf_num = 1, .vlan_id = 42, .rc = 0 },
> > + };
> > +
> > + for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
> > + rc = virNetDevSetVfVlan(testCases[i].ifname, testCases[i].vf_num, testCases[i].vlan_id);
> > + if (rc != testCases[i].rc) {
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testVirNetDevSetVfConfig(const void *opaque G_GNUC_UNUSED)
> > +{
> > + struct testCase {
> > + const char *ifname;
> > + const int rc;
> > + };
> > + int rc = 0;
> > + size_t i = 0;
> > + /* Nested functions are mocked so dummy values are used. */
> > + const virMacAddr mac = { .addr = { 0xDE, 0xAD, 0xBE, 0xEF, 0xCA, 0xFE }};
> > + const int vfNum = 1;
> > + const int vlanid = 0;
> > + bool *allowRetry = NULL;
> > +
> > + const struct testCase testCases[] = {
> > + { .ifname = "fakeiface-macerror", .rc = -1 },
> > + { .ifname = "fakeiface-altmacerror", .rc = -2 },
> > + { .ifname = "fakeiface-macerror-novlanerror", .rc = -1 },
> > + { .ifname = "fakeiface-macerror-vlanerror", .rc = -1 },
> > + { .ifname = "fakeiface-nomacerror-novlanerror", .rc = 0 },
> > + };
> > +
> > + for (i = 0; i < sizeof(testCases) / sizeof(struct testCase); ++i) {
> > + rc = virNetDevSetVfConfig(testCases[i].ifname, vfNum, &mac, vlanid, allowRetry);
> > + if (rc != testCases[i].rc) {
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static int
> > mymain(void)
> > {
> > @@ -76,6 +287,15 @@ mymain(void)
> > DO_TEST_LINK("lo", VIR_NETDEV_IF_STATE_UNKNOWN, 0);
> > DO_TEST_LINK("eth0-broken", VIR_NETDEV_IF_STATE_DOWN, 0);
> >
> > + if (virTestRun("Set VF MAC", testVirNetDevSetVfMac, NULL) < 0)
> > + ret = -1;
> > + if (virTestRun("Set VF MAC: missing MAC pointer", testVirNetDevSetVfMissingMac, NULL) < 0)
> > + ret = -1;
> > + if (virTestRun("Set VF VLAN", testVirNetDevSetVfVlan, NULL) < 0)
> > + ret = -1;
> > + if (virTestRun("Set VF Config", testVirNetDevSetVfConfig, NULL) < 0)
> > + ret = -1;
> > +
> > return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> > }
> >
> >
>
More information about the libvir-list
mailing list