Re: [libvirt] [PATCH v4 12/12] tests: add a test for driver.c:virConnectValidateURIPath()

[Cole Robinson]

On 9/27/19 5:32 PM, Daniel Henrique Barboza wrote:



On 9/27/19 5:33 PM, Cole Robinson wrote:

On 9/26/19 10:56 AM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza 
---

I've made this test file to make sure I wasn't messing
up with the logic at patch 8. The idea of having this
test seems okay, but probably I could do it shorter/cleaner.

Feel free to discard it if it's not applicable or
unnecessary. Adding this new file make the whole patch series,
aimed at reduce code repetition and lines of code, add more
lines than before. The irony is real.



It will reduce lines if you fold the data.entityName = X bit into the 
TEST_ calls, passing the string value as the first argument for example


Got it.



We don't have a ton of fine grained unittesting like this in libvirt, 
but it doesn't hurt. Though in this case it seems kinda overkill IMO 
to call it for possible combo that it's used in. I think it's better 
to just call it in all the invocations to give full code coverage. If 
we want to test the individual driver usage of the function then we 
would want to find a way to test those entry points directly IMO. 
Maybe others have opinions.


Do you mean we should just test the actual usage of the function in the 
code
instead of testing all possible fail scenarios? For example, the code 
does not
call virConnectValidateURIPath("/system", X , false) anywhere, so it's 
ok to

not test them since it's not being exercised anyway - otherwise we're
entering TDD territory (which I don't mind - kind of a fan TBH), testing 
all

possible stuff just for sake of testing.


Is that what you're saying? I'm fine with it, and it will cut a good 
chunk of

lines in this file too.



I was thinking, add test cases that are needed to hit every bit of logic 
in the function. So


privileged=false, /session path
privileged=false, non-/session path failure
privileged=true, /system path
privileged=true, non-/system path failure
privileged=true, vbox /session
privileged=true, qemu /session
privileged=true, other driver /session failure

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/12] tests: add a test for driver.c:virConnectValidateURIPath()

[Daniel Henrique Barboza]

On 9/27/19 5:33 PM, Cole Robinson wrote:

On 9/26/19 10:56 AM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza 
---

I've made this test file to make sure I wasn't messing
up with the logic at patch 8. The idea of having this
test seems okay, but probably I could do it shorter/cleaner.

Feel free to discard it if it's not applicable or
unnecessary. Adding this new file make the whole patch series,
aimed at reduce code repetition and lines of code, add more
lines than before. The irony is real.



It will reduce lines if you fold the data.entityName = X bit into the 
TEST_ calls, passing the string value as the first argument for example


Got it.



We don't have a ton of fine grained unittesting like this in libvirt, 
but it doesn't hurt. Though in this case it seems kinda overkill IMO 
to call it for possible combo that it's used in. I think it's better 
to just call it in all the invocations to give full code coverage. If 
we want to test the individual driver usage of the function then we 
would want to find a way to test those entry points directly IMO. 
Maybe others have opinions.


Do you mean we should just test the actual usage of the function in the code
instead of testing all possible fail scenarios? For example, the code 
does not

call virConnectValidateURIPath("/system", X , false) anywhere, so it's ok to
not test them since it's not being exercised anyway - otherwise we're
entering TDD territory (which I don't mind - kind of a fan TBH), testing all
possible stuff just for sake of testing.


Is that what you're saying? I'm fine with it, and it will cut a good 
chunk of

lines in this file too.




I'll let you decide what to do for v2 though


Thanks for the review. I'll wait to see if more people want to join in this
discussion before sending the v2.



DHB



- Cole



  tests/Makefile.am |   7 +-
  tests/virdriverconnvalidatetest.c | 186 ++
  2 files changed, 192 insertions(+), 1 deletion(-)
  create mode 100644 tests/virdriverconnvalidatetest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d88ad7f686..c7f563d24d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -433,7 +433,8 @@ test_scripts += $(libvirtd_test_scripts)
    test_programs += \
  eventtest \
-    virdrivermoduletest
+    virdrivermoduletest \
+    virdriverconnvalidatetest
  else ! WITH_LIBVIRTD
  EXTRA_DIST += $(libvirtd_test_scripts)
  endif ! WITH_LIBVIRTD
@@ -1485,6 +1486,10 @@ if WITH_LIBVIRTD
  virdrivermoduletest_SOURCES = \
  virdrivermoduletest.c testutils.h testutils.c
  virdrivermoduletest_LDADD = $(LDADDS)
+
+virdriverconnvalidatetest_SOURCES = \
+    virdriverconnvalidatetest.c testutils.h testutils.c
+virdriverconnvalidatetest_LDADD = $(LDADDS)
  endif WITH_LIBVIRTD
    if WITH_LIBVIRTD
diff --git a/tests/virdriverconnvalidatetest.c 
b/tests/virdriverconnvalidatetest.c

new file mode 100644
index 00..599150dc08
--- /dev/null
+++ b/tests/virdriverconnvalidatetest.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "testutils.h"
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "driver.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("tests.driverconnvalidatetest");
+
+struct testDriverConnValidateData {
+    const char *uriPath;
+    const char *entityName;
+    bool privileged;
+    bool expectFailure;
+};
+
+
+static int testDriverConnValidate(const void *args)
+{
+    const struct testDriverConnValidateData *data = args;
+
+    bool ret = virConnectValidateURIPath(data->uriPath,
+ data->entityName,
+ data->privileged);
+    if (data->expectFailure)
+    ret = !ret;
+
+    return ret ? 0 : -1;
+}
+
+
+static int
+mymain(void)
+{
+    int ret = 0;
+    struct testDriverConnValidateData data;
+
+#define TEST_SUCCESS(name) \
+    do  { \
+    data.expectFailure = false; \
+    if (virTestRun("Test conn URI path validate ok " name, \
+   testDriverConnValidate, ) < 0) \
+    ret = -1; \
+    } while (0)
+
+#define TEST_FAIL(name) \
+    do  { \
+    data.expectFailure = true; \
+    if (virTestRun("Test conn URI path validate fail " 

Re: [libvirt] [PATCH v4 12/12] tests: add a test for driver.c:virConnectValidateURIPath()

[Cole Robinson]

On 9/26/19 10:56 AM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza 
---

I've made this test file to make sure I wasn't messing
up with the logic at patch 8. The idea of having this
test seems okay, but probably I could do it shorter/cleaner.

Feel free to discard it if it's not applicable or
unnecessary. Adding this new file make the whole patch series,
aimed at reduce code repetition and lines of code, add more
lines than before. The irony is real.



It will reduce lines if you fold the data.entityName = X bit into the 
TEST_ calls, passing the string value as the first argument for example


We don't have a ton of fine grained unittesting like this in libvirt, 
but it doesn't hurt. Though in this case it seems kinda overkill IMO to 
call it for possible combo that it's used in. I think it's better to 
just call it in all the invocations to give full code coverage. If we 
want to test the individual driver usage of the function then we would 
want to find a way to test those entry points directly IMO. Maybe others 
have opinions.


I'll let you decide what to do for v2 though

- Cole



  tests/Makefile.am |   7 +-
  tests/virdriverconnvalidatetest.c | 186 ++
  2 files changed, 192 insertions(+), 1 deletion(-)
  create mode 100644 tests/virdriverconnvalidatetest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d88ad7f686..c7f563d24d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -433,7 +433,8 @@ test_scripts += $(libvirtd_test_scripts)
  
  test_programs += \

eventtest \
-   virdrivermoduletest
+   virdrivermoduletest \
+   virdriverconnvalidatetest
  else ! WITH_LIBVIRTD
  EXTRA_DIST += $(libvirtd_test_scripts)
  endif ! WITH_LIBVIRTD
@@ -1485,6 +1486,10 @@ if WITH_LIBVIRTD
  virdrivermoduletest_SOURCES = \
virdrivermoduletest.c testutils.h testutils.c
  virdrivermoduletest_LDADD = $(LDADDS)
+
+virdriverconnvalidatetest_SOURCES = \
+   virdriverconnvalidatetest.c testutils.h testutils.c
+virdriverconnvalidatetest_LDADD = $(LDADDS)
  endif WITH_LIBVIRTD
  
  if WITH_LIBVIRTD

diff --git a/tests/virdriverconnvalidatetest.c 
b/tests/virdriverconnvalidatetest.c
new file mode 100644
index 00..599150dc08
--- /dev/null
+++ b/tests/virdriverconnvalidatetest.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "testutils.h"
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "driver.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("tests.driverconnvalidatetest");
+
+struct testDriverConnValidateData {
+const char *uriPath;
+const char *entityName;
+bool privileged;
+bool expectFailure;
+};
+
+
+static int testDriverConnValidate(const void *args)
+{
+const struct testDriverConnValidateData *data = args;
+
+bool ret = virConnectValidateURIPath(data->uriPath,
+ data->entityName,
+ data->privileged);
+if (data->expectFailure)
+ret = !ret;
+
+return ret ? 0 : -1;
+}
+
+
+static int
+mymain(void)
+{
+int ret = 0;
+struct testDriverConnValidateData data;
+
+#define TEST_SUCCESS(name) \
+do  { \
+data.expectFailure = false; \
+if (virTestRun("Test conn URI path validate ok " name, \
+   testDriverConnValidate, ) < 0) \
+ret = -1; \
+} while (0)
+
+#define TEST_FAIL(name) \
+do  { \
+data.expectFailure = true; \
+if (virTestRun("Test conn URI path validate fail " name, \
+   testDriverConnValidate, ) < 0) \
+ret = -1; \
+} while (0)
+
+/* Validation should always succeed with privileged user
+ * and '/system' URI path
+ */
+data.uriPath = "/system";
+data.privileged = true;
+
+data.entityName = "interface";
+TEST_SUCCESS("interface privileged /system");
+
+data.entityName = "network";
+TEST_SUCCESS("network privileged /system");
+
+data.entityName = "nodedev";
+TEST_SUCCESS("nodedev privileged /system");
+
+data.entityName = "secret";
+TEST_SUCCESS("secret privileged /system");
+
+data.entityName = "storage";
+TEST_SUCCESS("storage privileged 

[libvirt] [PATCH v4 12/12] tests: add a test for driver.c:virConnectValidateURIPath()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza 
---

I've made this test file to make sure I wasn't messing
up with the logic at patch 8. The idea of having this
test seems okay, but probably I could do it shorter/cleaner.

Feel free to discard it if it's not applicable or
unnecessary. Adding this new file make the whole patch series,
aimed at reduce code repetition and lines of code, add more
lines than before. The irony is real.


 tests/Makefile.am |   7 +-
 tests/virdriverconnvalidatetest.c | 186 ++
 2 files changed, 192 insertions(+), 1 deletion(-)
 create mode 100644 tests/virdriverconnvalidatetest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d88ad7f686..c7f563d24d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -433,7 +433,8 @@ test_scripts += $(libvirtd_test_scripts)
 
 test_programs += \
eventtest \
-   virdrivermoduletest
+   virdrivermoduletest \
+   virdriverconnvalidatetest
 else ! WITH_LIBVIRTD
 EXTRA_DIST += $(libvirtd_test_scripts)
 endif ! WITH_LIBVIRTD
@@ -1485,6 +1486,10 @@ if WITH_LIBVIRTD
 virdrivermoduletest_SOURCES = \
virdrivermoduletest.c testutils.h testutils.c
 virdrivermoduletest_LDADD = $(LDADDS)
+
+virdriverconnvalidatetest_SOURCES = \
+   virdriverconnvalidatetest.c testutils.h testutils.c
+virdriverconnvalidatetest_LDADD = $(LDADDS)
 endif WITH_LIBVIRTD
 
 if WITH_LIBVIRTD
diff --git a/tests/virdriverconnvalidatetest.c 
b/tests/virdriverconnvalidatetest.c
new file mode 100644
index 00..599150dc08
--- /dev/null
+++ b/tests/virdriverconnvalidatetest.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "testutils.h"
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "driver.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("tests.driverconnvalidatetest");
+
+struct testDriverConnValidateData {
+const char *uriPath;
+const char *entityName;
+bool privileged;
+bool expectFailure;
+};
+
+
+static int testDriverConnValidate(const void *args)
+{
+const struct testDriverConnValidateData *data = args;
+
+bool ret = virConnectValidateURIPath(data->uriPath,
+ data->entityName,
+ data->privileged);
+if (data->expectFailure)
+ret = !ret;
+
+return ret ? 0 : -1;
+}
+
+
+static int
+mymain(void)
+{
+int ret = 0;
+struct testDriverConnValidateData data;
+
+#define TEST_SUCCESS(name) \
+do  { \
+data.expectFailure = false; \
+if (virTestRun("Test conn URI path validate ok " name, \
+   testDriverConnValidate, ) < 0) \
+ret = -1; \
+} while (0)
+
+#define TEST_FAIL(name) \
+do  { \
+data.expectFailure = true; \
+if (virTestRun("Test conn URI path validate fail " name, \
+   testDriverConnValidate, ) < 0) \
+ret = -1; \
+} while (0)
+
+/* Validation should always succeed with privileged user
+ * and '/system' URI path
+ */
+data.uriPath = "/system";
+data.privileged = true;
+
+data.entityName = "interface";
+TEST_SUCCESS("interface privileged /system");
+
+data.entityName = "network";
+TEST_SUCCESS("network privileged /system");
+
+data.entityName = "nodedev";
+TEST_SUCCESS("nodedev privileged /system");
+
+data.entityName = "secret";
+TEST_SUCCESS("secret privileged /system");
+
+data.entityName = "storage";
+TEST_SUCCESS("storage privileged /system");
+
+data.entityName = "qemu";
+TEST_SUCCESS("qemu privileged /system");
+
+data.entityName = "vbox";
+TEST_SUCCESS("vbox privileged /system");
+
+/* Fail URI path validation for all '/system' path with
+ * unprivileged user
+ */
+data.privileged = false;
+
+data.entityName = "interface";
+TEST_FAIL("interface unprivileged /system");
+
+data.entityName = "network";
+TEST_FAIL("network unprivileged /system");
+
+data.entityName = "nodedev";
+TEST_FAIL("nodedev unprivileged /system");
+
+data.entityName = "secret";
+TEST_FAIL("secret unprivileged /system");
+
+data.entityName = "storage";
+TEST_FAIL("storage unprivileged /system");
+
+