8:41 a.m.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1844341
The "iface" parameter of the various "add-drive" actions has never
been
functional: the libvirt backend has always rejected a non-empty "iface",
and the direct backend has never handled it correctly. The parameter
itself is not useful for anything really, so let's just remove all the
logic related to it, beyond keeping the API compatible. Update the docs
as well.
Thanks,
Laszlo
Laszlo Ersek (4):
lib: launch-direct: ignore drive "iface" parameter
lib: drive_create_data, drive: remove field "iface"
lib: rename VALID_FORMAT_IFACE to VALID_FORMAT
tests/regressions: remove "iface"-based restrictions
generator/actions_core_deprecated.ml | 6 +-
lib/drives.c | 35 +++---------
lib/guestfs-internal.h | 1 -
lib/launch-direct.c | 59 +++++---------------
lib/launch-libvirt.c | 6 --
lib/libvirt-domain.c | 15 -----
lib/unit-tests.c | 16 +++---
tests/regressions/rhbz690819.sh | 10 +---
tests/regressions/rhbz975797.sh | 10 +---
9 files changed, 38 insertions(+), 120 deletions(-)
base-commit: 4864d21cb8eb991f0fc98d03a068173837cba50e
--
2.19.1.3.g30247aa5d201
8:41 a.m.
New subject: [libguestfs PATCH 1/4] lib: launch-direct: ignore drive "iface" parameter
Rich said in <https://bugzilla.redhat.com/show_bug.cgi?id=1844341#c1>;:
The libvirt backend has never allowed the iface parameter. We
should
probably ignore it in the direct backend since it's never been possible
to use this parameter correctly.
Remove the handling of "iface" in the direct (QEMU) backend. Refresh the
documentation regarding both backends.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1844341
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
generator/actions_core_deprecated.ml | 8 ++-
lib/launch-direct.c | 59 +++++---------------
2 files changed, 19 insertions(+), 48 deletions(-)
diff --git a/generator/actions_core_deprecated.ml b/generator/actions_core_deprecated.ml
index 00dde3d2a5be..f1040a0e9491 100644
--- a/generator/actions_core_deprecated.ml
+++ b/generator/actions_core_deprecated.ml
@@ -73,7 +73,9 @@ of C<guestfs_add_drive_ro>." };
shortdesc = "add a drive specifying the QEMU block emulation to use";
longdesc = "\
This is the same as C<guestfs_add_drive> but it allows you
-to specify the QEMU interface emulation to use at run time." };
+to specify the QEMU interface emulation to use at run time.
+The libvirt backend rejects a non-empty C<iface> argument.
+The direct backend ignores C<iface>." };
{ defaults with
name = "add_drive_ro_with_if"; added = (1, 0, 84);
@@ -83,7 +85,9 @@ to specify the QEMU interface emulation to use at run time." };
shortdesc = "add a drive read-only specifying the QEMU block emulation to
use";
longdesc = "\
This is the same as C<guestfs_add_drive_ro> but it allows you
-to specify the QEMU interface emulation to use at run time." };
+to specify the QEMU interface emulation to use at run time.
+The libvirt backend rejects a non-empty C<iface> argument.
+The direct backend ignores C<iface>." };
{ defaults with
name = "lstatlist"; added = (1, 0, 77);
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 5c91822fb52d..c07a8d78f1a0 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -296,52 +296,19 @@ static int
add_drive (guestfs_h *g, struct backend_direct_data *data,
struct qemuopts *qopts, size_t i, struct drive *drv)
{
- /* If there's an explicit 'iface', use it. Otherwise default to
- * virtio-scsi.
- */
- if (drv->iface && STREQ (drv->iface, "virtio")) { /* virtio-blk
*/
- start_list ("-drive") {
- if (add_drive_standard_params (g, data, qopts, i, drv) == -1)
- return -1;
- append_list ("if=none");
- } end_list ();
- start_list ("-device") {
- append_list (VIRTIO_DEVICE_NAME ("virtio-blk"));
- append_list_format ("drive=hd%zu", i);
- if (drv->disk_label)
- append_list_format ("serial=%s", drv->disk_label);
- if (add_device_blocksize_params (g, qopts, drv) == -1)
- return -1;
- } end_list ();
- }
-#if defined(__arm__) || defined(__aarch64__) || defined(__powerpc__)
- else if (drv->iface && STREQ (drv->iface, "ide")) {
- error (g, "'ide' interface does not work on ARM or PowerPC");
- return -1;
- }
-#endif
- else if (drv->iface) {
- start_list ("-drive") {
- if (add_drive_standard_params (g, data, qopts, i, drv) == -1)
- return -1;
- append_list_format ("if=%s", drv->iface);
- } end_list ();
- }
- else /* default case: virtio-scsi */ {
- start_list ("-drive") {
- if (add_drive_standard_params (g, data, qopts, i, drv) == -1)
- return -1;
- append_list ("if=none");
- } end_list ();
- start_list ("-device") {
- append_list ("scsi-hd");
- append_list_format ("drive=hd%zu", i);
- if (drv->disk_label)
- append_list_format ("serial=%s", drv->disk_label);
- if (add_device_blocksize_params (g, qopts, drv) == -1)
- return -1;
- } end_list ();
- }
+ start_list ("-drive") {
+ if (add_drive_standard_params (g, data, qopts, i, drv) == -1)
+ return -1;
+ append_list ("if=none");
+ } end_list ();
+ start_list ("-device") {
+ append_list ("scsi-hd");
+ append_list_format ("drive=hd%zu", i);
+ if (drv->disk_label)
+ append_list_format ("serial=%s", drv->disk_label);
+ if (add_device_blocksize_params (g, qopts, drv) == -1)
+ return -1;
+ } end_list ();
return 0;
--
2.19.1.3.g30247aa5d201
8:41 a.m.
New subject: [libguestfs PATCH 2/4] lib: drive_create_data, drive: remove field "iface"
Representing "iface" in the "drive_create_data" and "drive"
structures is
now useless; the direct backend ignores "iface", while the libvirt one
rejects it unless it is empty. Unify both backends -- make them both
ignore "iface". (Which only relaxes the libvirt backend, so it cannot
cause compatibility problems.) This lets us remove the fields. Update the
documentation as well.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1844341
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
lib/guestfs-internal.h | 1 -
generator/actions_core_deprecated.ml | 6 ++--
lib/drives.c | 31 ++++----------------
lib/launch-libvirt.c | 6 ----
lib/libvirt-domain.c | 15 ----------
5 files changed, 7 insertions(+), 52 deletions(-)
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 5bb00bc10149..16755cfb3370 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -298,7 +298,6 @@ struct drive {
/* Various per-drive flags. */
bool readonly;
- char *iface;
char *name;
char *disk_label;
char *cachemode;
diff --git a/generator/actions_core_deprecated.ml b/generator/actions_core_deprecated.ml
index f1040a0e9491..c23f4a330354 100644
--- a/generator/actions_core_deprecated.ml
+++ b/generator/actions_core_deprecated.ml
@@ -74,8 +74,7 @@ of C<guestfs_add_drive_ro>." };
longdesc = "\
This is the same as C<guestfs_add_drive> but it allows you
to specify the QEMU interface emulation to use at run time.
-The libvirt backend rejects a non-empty C<iface> argument.
-The direct backend ignores C<iface>." };
+Both the direct and the libvirt backends ignore C<iface>." };
{ defaults with
name = "add_drive_ro_with_if"; added = (1, 0, 84);
@@ -86,8 +85,7 @@ The direct backend ignores C<iface>." };
longdesc = "\
This is the same as C<guestfs_add_drive_ro> but it allows you
to specify the QEMU interface emulation to use at run time.
-The libvirt backend rejects a non-empty C<iface> argument.
-The direct backend ignores C<iface>." };
+Both the direct and the libvirt backends ignore C<iface>." };
{ defaults with
name = "lstatlist"; added = (1, 0, 77);
diff --git a/lib/drives.c b/lib/drives.c
index a6179fc367a6..8fe46a41c9cc 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -53,7 +53,6 @@ struct drive_create_data {
const char *secret;
bool readonly;
const char *format;
- const char *iface;
const char *name;
const char *disk_label;
const char *cachemode;
@@ -110,7 +109,6 @@ create_drive_file (guestfs_h *g,
drv->src.format = data->format ? safe_strdup (g, data->format) : NULL;
drv->readonly = data->readonly;
- drv->iface = data->iface ? safe_strdup (g, data->iface) : NULL;
drv->name = data->name ? safe_strdup (g, data->name) : NULL;
drv->disk_label = data->disk_label ? safe_strdup (g, data->disk_label) :
NULL;
drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode) : NULL;
@@ -147,7 +145,6 @@ create_drive_non_file (guestfs_h *g,
drv->src.format = data->format ? safe_strdup (g, data->format) : NULL;
drv->readonly = data->readonly;
- drv->iface = data->iface ? safe_strdup (g, data->iface) : NULL;
drv->name = data->name ? safe_strdup (g, data->name) : NULL;
drv->disk_label = data->disk_label ? safe_strdup (g, data->disk_label) :
NULL;
drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode) : NULL;
@@ -470,7 +467,6 @@ free_drive_struct (struct drive *drv)
{
free_drive_source (&drv->src);
free (drv->overlay);
- free (drv->iface);
free (drv->name);
free (drv->disk_label);
free (drv->cachemode);
@@ -511,14 +507,12 @@ drive_to_string (guestfs_h *g, const struct drive *drv)
s_blocksize = safe_asprintf (g, "%d", drv->blocksize);
return safe_asprintf
- (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s%s%s%s",
+ (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s%s",
drv->src.u.path,
drv->readonly ? " readonly" : "",
drv->src.format ? " format=" : "",
drv->src.format ? : "",
guestfs_int_drive_protocol_to_string (drv->src.protocol),
- drv->iface ? " iface=" : "",
- drv->iface ? : "",
drv->name ? " name=" : "",
drv->name ? : "",
drv->disk_label ? " label=" : "",
@@ -747,8 +741,6 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char *filename,
? optargs->readonly : false;
data.format = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK
? optargs->format : NULL;
- data.iface = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK
- ? optargs->iface : NULL;
data.name = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_NAME_BITMASK
? optargs->name : NULL;
data.disk_label = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_LABEL_BITMASK
@@ -804,12 +796,6 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char *filename,
free_drive_servers (data.servers, data.nr_servers);
return -1;
}
- if (data.iface && !VALID_FORMAT_IFACE (data.iface)) {
- error (g, _("%s parameter is empty or contains disallowed characters"),
- "iface");
- free_drive_servers (data.servers, data.nr_servers);
- return -1;
- }
if (data.disk_label && !VALID_DISK_LABEL (data.disk_label)) {
error (g, _("label parameter is empty, too long, or contains disallowed
characters"));
free_drive_servers (data.servers, data.nr_servers);
@@ -935,24 +921,17 @@ guestfs_impl_add_drive_ro (guestfs_h *g, const char *filename)
int
guestfs_impl_add_drive_with_if (guestfs_h *g, const char *filename,
- const char *iface)
+ const char *iface ATTRIBUTE_UNUSED)
{
- const struct guestfs_add_drive_opts_argv optargs = {
- .bitmask = GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK,
- .iface = iface,
- };
-
- return guestfs_add_drive_opts_argv (g, filename, &optargs);
+ return guestfs_add_drive_opts_argv (g, filename, NULL);
}
int
guestfs_impl_add_drive_ro_with_if (guestfs_h *g, const char *filename,
- const char *iface)
+ const char *iface ATTRIBUTE_UNUSED)
{
const struct guestfs_add_drive_opts_argv optargs = {
- .bitmask = GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK
- | GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK,
- .iface = iface,
+ .bitmask = GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK,
.readonly = true,
};
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index 44764f3cc022..87da2f40e019 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -1465,12 +1465,6 @@ construct_libvirt_xml_disk (guestfs_h *g,
const char *type, *uuid;
int r;
- /* XXX We probably could support this if we thought about it some more. */
- if (drv->iface) {
- error (g, _("‘iface’ parameter is not supported by the libvirt backend"));
- return -1;
- }
-
start_element ("disk") {
attribute ("device", "disk");
diff --git a/lib/libvirt-domain.c b/lib/libvirt-domain.c
index 3050680fab02..fafbf50ea0d1 100644
--- a/lib/libvirt-domain.c
+++ b/lib/libvirt-domain.c
@@ -68,7 +68,6 @@ guestfs_impl_add_domain (guestfs_h *g, const char *domain_name,
int live;
int allowuuid;
const char *readonlydisk;
- const char *iface;
const char *cachemode;
const char *discard;
bool copyonread;
@@ -78,8 +77,6 @@ guestfs_impl_add_domain (guestfs_h *g, const char *domain_name,
? optargs->libvirturi : NULL;
readonly = optargs->bitmask & GUESTFS_ADD_DOMAIN_READONLY_BITMASK
? optargs->readonly : 0;
- iface = optargs->bitmask & GUESTFS_ADD_DOMAIN_IFACE_BITMASK
- ? optargs->iface : NULL;
live = optargs->bitmask & GUESTFS_ADD_DOMAIN_LIVE_BITMASK
? optargs->live : 0;
allowuuid = optargs->bitmask & GUESTFS_ADD_DOMAIN_ALLOWUUID_BITMASK
@@ -136,10 +133,6 @@ guestfs_impl_add_domain (guestfs_h *g, const char *domain_name,
optargs2.bitmask |= GUESTFS_ADD_LIBVIRT_DOM_READONLY_BITMASK;
optargs2.readonly = readonly;
}
- if (iface) {
- optargs2.bitmask |= GUESTFS_ADD_LIBVIRT_DOM_IFACE_BITMASK;
- optargs2.iface = iface;
- }
if (live) {
error (g, _("libguestfs live support was removed in libguestfs 1.48"));
goto cleanup;
@@ -193,7 +186,6 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void *domvp,
virDomainPtr dom = domvp;
ssize_t r;
int readonly;
- const char *iface;
const char *cachemode;
const char *discard;
bool copyonread;
@@ -208,9 +200,6 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void *domvp,
readonly =
optargs->bitmask & GUESTFS_ADD_LIBVIRT_DOM_READONLY_BITMASK
? optargs->readonly : 0;
- iface =
- optargs->bitmask & GUESTFS_ADD_LIBVIRT_DOM_IFACE_BITMASK
- ? optargs->iface : NULL;
live =
optargs->bitmask & GUESTFS_ADD_LIBVIRT_DOM_LIVE_BITMASK
? optargs->live : 0;
@@ -289,10 +278,6 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void *domvp,
data.optargs.bitmask = 0;
data.readonly = readonly;
data.readonlydisk = readonlydisk;
- if (iface) {
- data.optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK;
- data.optargs.iface = iface;
- }
if (cachemode) {
data.optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_CACHEMODE_BITMASK;
data.optargs.cachemode = cachemode;
--
2.19.1.3.g30247aa5d201
8:41 a.m.
New subject: [libguestfs PATCH 3/4] lib: rename VALID_FORMAT_IFACE to VALID_FORMAT
We no longer use VALID_FORMAT_IFACE for validating "iface"; rename the
macro to reflect that we only check "format" with it.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1844341
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
lib/drives.c | 4 ++--
lib/unit-tests.c | 16 ++++++++--------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/lib/drives.c b/lib/drives.c
index 8fe46a41c9cc..c5a208468d6c 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -593,7 +593,7 @@ guestfs_int_free_drives (guestfs_h *g)
* Check string parameter matches regular expression
* C<^[-_[:alnum:]]+$> (in C locale).
*/
-#define VALID_FORMAT_IFACE(str) \
+#define VALID_FORMAT(str) \
guestfs_int_string_is_valid ((str), 1, 0, \
VALID_FLAG_ALPHA|VALID_FLAG_DIGIT, "-_")
@@ -790,7 +790,7 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char *filename,
return -1;
}
- if (data.format && !VALID_FORMAT_IFACE (data.format)) {
+ if (data.format && !VALID_FORMAT (data.format)) {
error (g, _("%s parameter is empty or contains disallowed characters"),
"format");
free_drive_servers (data.servers, data.nr_servers);
diff --git a/lib/unit-tests.c b/lib/unit-tests.c
index 62457ccba3fa..0e550cb98b7e 100644
--- a/lib/unit-tests.c
+++ b/lib/unit-tests.c
@@ -434,7 +434,7 @@ test_stringsbuf (void)
}
/* Use the same macros as in lib/drives.c */
-#define VALID_FORMAT_IFACE(str) \
+#define VALID_FORMAT(str) \
guestfs_int_string_is_valid ((str), 1, 0, \
VALID_FLAG_ALPHA|VALID_FLAG_DIGIT, "-_")
#define VALID_DISK_LABEL(str) \
@@ -446,18 +446,18 @@ test_stringsbuf (void)
static void
test_valid (void)
{
- assert (!VALID_FORMAT_IFACE (""));
+ assert (!VALID_FORMAT (""));
assert (!VALID_DISK_LABEL (""));
assert (!VALID_HOSTNAME (""));
assert (!VALID_DISK_LABEL ("012345678901234567890"));
- assert (VALID_FORMAT_IFACE ("abc"));
- assert (VALID_FORMAT_IFACE ("ABC"));
- assert (VALID_FORMAT_IFACE ("abc123"));
- assert (VALID_FORMAT_IFACE ("abc123-"));
- assert (VALID_FORMAT_IFACE ("abc123_"));
- assert (!VALID_FORMAT_IFACE ("abc123."));
+ assert (VALID_FORMAT ("abc"));
+ assert (VALID_FORMAT ("ABC"));
+ assert (VALID_FORMAT ("abc123"));
+ assert (VALID_FORMAT ("abc123-"));
+ assert (VALID_FORMAT ("abc123_"));
+ assert (!VALID_FORMAT ("abc123."));
assert (VALID_DISK_LABEL ("abc"));
assert (VALID_DISK_LABEL ("ABC"));
--
2.19.1.3.g30247aa5d201
8:41 a.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
Now that "iface" is ignored by both backends, the regression tests for
RHBZ 690819 and 975797 can be enabled on all arches (regardless of
backend).
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1844341
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
tests/regressions/rhbz690819.sh | 10 +++-------
tests/regressions/rhbz975797.sh | 10 +++-------
2 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/tests/regressions/rhbz690819.sh b/tests/regressions/rhbz690819.sh
index e6f61d00d3ce..9e1bcda845b4 100755
--- a/tests/regressions/rhbz690819.sh
+++ b/tests/regressions/rhbz690819.sh
@@ -19,18 +19,14 @@
# https://bugzilla.redhat.com/show_bug.cgi?id=690819
# mkfs fails creating a filesytem on a disk device when using a disk
# with 'ide' interface
+#
+# The 'iface' parameter is now ignored:
+# https://bugzilla.redhat.com/show_bug.cgi?id=1844341
set -e
$TEST_FUNCTIONS
skip_if_skipped
-# These architectures don't support the 'ide' interface.
-skip_if_arch arm
-skip_if_arch aarch64
-skip_if_arch ppc64
-skip_if_arch ppc64le
-skip_if_arch s390x
-skip_if_backend libvirt
rm -f rhbz690819.img
diff --git a/tests/regressions/rhbz975797.sh b/tests/regressions/rhbz975797.sh
index c676abfa3c87..feecf1f2b418 100755
--- a/tests/regressions/rhbz975797.sh
+++ b/tests/regressions/rhbz975797.sh
@@ -19,18 +19,14 @@
# Regression test for:
# https://bugzilla.redhat.com/show_bug.cgi?id=975797
# Ensure the appliance doesn't hang when using the 'iface' parameter.
+#
+# The 'iface' parameter is now ignored:
+# https://bugzilla.redhat.com/show_bug.cgi?id=1844341
set -e
$TEST_FUNCTIONS
skip_if_skipped
-# These architectures don't support the 'ide' interface.
-skip_if_arch arm
-skip_if_arch aarch64
-skip_if_arch ppc64
-skip_if_arch ppc64le
-skip_if_arch s390x
-skip_if_backend libvirt
rm -f rhbz975797-*.img
--
2.19.1.3.g30247aa5d201
1:50 p.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
For the whole series:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Do you think it makes sense to completely drop the two regression
tests in patch 4? They're testing something that (probably) cannot go
wrong now.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
1:25 a.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
On 05/04/22 20:50, Richard W.M. Jones wrote:
For the whole series:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Do you think it makes sense to completely drop the two regression
tests in patch 4? They're testing something that (probably) cannot go
wrong now.
We can do that, but then a few more test cases should be modified
perhaps, for consistency. If you grep the tree for "iface", you'll find
a few test cases that pass various "iface" params (off-hand I recall
Ruby, OCaml, and Java-language test cases -- I think those are all
"bindtests"). I figured those tests still made sense (for ensuring API
compat), so I didn't remove the iface params, and/or the test cases that
only differrerd from the rest in their iface params. And then I figured
the rhbz regression tests should stay too (just be relaxed).
If we drop the latter (the rhbz regression tests), should we still keep
the other (language bindings) tests?
Thanks,
Laszlo
3:48 a.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
On Thu, May 05, 2022 at 08:25:35AM +0200, Laszlo Ersek wrote:
On 05/04/22 20:50, Richard W.M. Jones wrote:
>
> For the whole series:
>
> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
>
> Do you think it makes sense to completely drop the two regression
> tests in patch 4? They're testing something that (probably) cannot go
> wrong now.
We can do that, but then a few more test cases should be modified
perhaps, for consistency. If you grep the tree for "iface", you'll find
a few test cases that pass various "iface" params (off-hand I recall
Ruby, OCaml, and Java-language test cases -- I think those are all
"bindtests"). I figured those tests still made sense (for ensuring API
compat), so I didn't remove the iface params, and/or the test cases that
only differrerd from the rest in their iface params. And then I figured
the rhbz regression tests should stay too (just be relaxed).
We cannot drop the parameter, it's part of the ABI.
If we drop the latter (the rhbz regression tests), should we still
keep
the other (language bindings) tests?
The bindtests are mainly about testing the binding between language
and the C library so without actually looking IIRC 'iface' is only
used to check that strings are converted correctly.
Anyway, it's fine to leave it.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
6:06 a.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
On 05/05/22 10:48, Richard W.M. Jones wrote:
On Thu, May 05, 2022 at 08:25:35AM +0200, Laszlo Ersek wrote:
> On 05/04/22 20:50, Richard W.M. Jones wrote:
>>
>> For the whole series:
>>
>> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
>>
>> Do you think it makes sense to completely drop the two regression
>> tests in patch 4? They're testing something that (probably) cannot go
>> wrong now.
>
> We can do that, but then a few more test cases should be modified
> perhaps, for consistency. If you grep the tree for "iface", you'll
find
> a few test cases that pass various "iface" params (off-hand I recall
> Ruby, OCaml, and Java-language test cases -- I think those are all
> "bindtests"). I figured those tests still made sense (for ensuring API
> compat), so I didn't remove the iface params, and/or the test cases that
> only differrerd from the rest in their iface params. And then I figured
> the rhbz regression tests should stay too (just be relaxed).
We cannot drop the parameter, it's part of the ABI.
I didn't mean dropping the parameter; I meant dropping the testing of
the parameter in the bind tests.
However:
> If we drop the latter (the rhbz regression tests), should we
still keep
> the other (language bindings) tests?
The bindtests are mainly about testing the binding between language
and the C library so without actually looking IIRC 'iface' is only
used to check that strings are converted correctly.
You are right that as long as the parameter is part of the API
(regardless of whether it is used for anything useful, or just ignored),
we should continue testing *its binding*.
Anyway, it's fine to leave it.
Commit range 4864d21cb8eb..ddf276884c04.
Thank you!
Laszlo
6:12 a.m.
New subject: [libguestfs PATCH 4/4] tests/regressions: remove "iface"-based restrictions
On Thu, May 05, 2022 at 01:06:46PM +0200, Laszlo Ersek wrote:
On 05/05/22 10:48, Richard W.M. Jones wrote:
> On Thu, May 05, 2022 at 08:25:35AM +0200, Laszlo Ersek wrote:
>> On 05/04/22 20:50, Richard W.M. Jones wrote:
>>>
>>> For the whole series:
>>>
>>> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
>>>
>>> Do you think it makes sense to completely drop the two regression
>>> tests in patch 4? They're testing something that (probably) cannot go
>>> wrong now.
>>
>> We can do that, but then a few more test cases should be modified
>> perhaps, for consistency. If you grep the tree for "iface", you'll
find
>> a few test cases that pass various "iface" params (off-hand I recall
>> Ruby, OCaml, and Java-language test cases -- I think those are all
>> "bindtests"). I figured those tests still made sense (for ensuring
API
>> compat), so I didn't remove the iface params, and/or the test cases that
>> only differrerd from the rest in their iface params. And then I figured
>> the rhbz regression tests should stay too (just be relaxed).
>
> We cannot drop the parameter, it's part of the ABI.
I didn't mean dropping the parameter; I meant dropping the testing of
the parameter in the bind tests.
OK, I misunderstood.
Anyway thanks for fixing this.
Rich.
However:
>> If we drop the latter (the rhbz regression tests), should we still keep
>> the other (language bindings) tests?
>
> The bindtests are mainly about testing the binding between language
> and the C library so without actually looking IIRC 'iface' is only
> used to check that strings are converted correctly.
You are right that as long as the parameter is part of the API
(regardless of whether it is used for anything useful, or just ignored),
we should continue testing *its binding*.
> Anyway, it's fine to leave it.
Commit range 4864d21cb8eb..ddf276884c04.
Thank you!
Laszlo
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
935
days inactive
936
days old
9 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones