7:47 a.m.
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...
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(a)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;
> }
>
>