[Libguestfs] [nbdkit PATCH 8/9] vector: introduce DEFINE_POINTER_VECTOR_TYPE()
(Commit message below adapted from libnbd commit fda38988b86b, "vector:
introduce DEFINE_POINTER_VECTOR_TYPE()", 2023-02-28).
The "name##_iter" function is used 5 times in nbdkit as follows:
string_vector_iter (..., (void *) free);
Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)
Furthermore, in those 5 cases, the freeing of the vector's strings is
immediately followed by the release of the vector's array-of-pointers too.
(This additional step is not expressed consistently across nbdkit: it's
sometimes spelled as free(vec.ptr), sometimes as
string_vector_reset().)
Introduce "name##_empty", which performs both steps at the same time.
Expose the "name##_empty" function definition with a new, separate macro:
ADD_VECTOR_EMPTY_METHOD(). The existent DEFINE_VECTOR_TYPE() macro permits
such element types that are not pointers, or are pointers to const- and/or
volatile-qualified objects. Whereas "name##_empty" requires that the
elements be pointers to dynamically allocated, non-const, non-volatile
objects.
Add DEFINE_POINTER_VECTOR_TYPE() that expands to both DEFINE_VECTOR_TYPE()
and the additive ADD_VECTOR_EMPTY_METHOD().
(
For example, after
typedef char foobar[5];
the following compiles (as expected):
DEFINE_VECTOR_TYPE (foobar_vector, foobar);
and the following fails to build (as expected):
DEFINE_POINTER_VECTOR_TYPE (foobar_vector_bad, foobar);
with the diagnostics including
> In function ‘foobar_vector_bad_empty’:
> error: size of array ‘_vector_contains_pointers1’ is negative
)
Switch "string_vector" to DEFINE_POINTER_VECTOR_TYPE(). The 5
string_vector_iter() call sites continue working; they will be converted
to string_vector_empty() in a subsequent patch.
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit fda38988b86bb133b56fc6251b2d581e809b3b67)
---
Makefile.am | 1 +
common/utils/string-vector.h | 2 +-
common/utils/vector.h| 35
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am
index caa206064008..9bd942538d0d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -58,6 +58,7 @@ noinst_PROGRAMS = nbdkit
nbdkit_SOURCES = wrapper.c server/options.h
nbdkit_CPPFLAGS = \
-I$(top_srcdir)/server \
+ -I$(top_srcdir)/common/include \
-I$(top_srcdir)/common/utils \
-I$(top_srcdir)/common/replacements \
-Dbuilddir=\"$(abs_top_builddir)\" \
diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
index 7309ca4aa13d..40446008b6a9 100644
--- a/common/utils/string-vector.h
+++ b/common/utils/string-vector.h
@@ -37,7 +37,7 @@
#include "vector.h"
-DEFINE_VECTOR_TYPE (string_vector, char *);
+DEFINE_POINTER_VECTOR_TYPE (string_vector, char *);
/* This frees both the array and the strings. */
#define CLEANUP_FREE_STRING_VECTOR \
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 5f5f96e97ab2..9549fa9e6f2f 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -44,6 +44,9 @@
#include
#include
+#include "compiler-macros.h"
+#include "static-assert.h"
+
#ifdef __clang__
#pragma clang diagnostic ignored "-Wunused-function"
#pragma clang diagnostic ignored "-Wduplicate-decl-specifier"
@@ -200,6 +203,38 @@
#define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
+/* This macro should only be used if:
+ * - the vector contains pointers, and
+ * - the pointed-to objects are:
+ * - neither const- nor volatile-qualified, and
+ * - allocated with malloc() or equivalent.
+ */
+#define ADD_VECTOR_EMPTY_METHOD(name) \
+ /* Call free() on each element of the vector, then reset the vector. \
+ */ \
+ static inline void __attribute__ ((__unused__)) \
+ name##_empty (name *v) \
+ {\
+size_t i; \
+for (i = 0; i < v->len; ++i) { \
+ STATIC_ASSERT (TYPE_IS_POINTER (v->ptr[i]), \
+ _vector_contains_pointers); \
+ free (v->ptr[i]);\
+} \
+name##_reset (v); \
+ }\
+ \
+ /* Force callers to supply ';'. */ \
+ struct name
+
+/* Convenience macro tying together DEFINE_VECTOR_TYPE() and
+ * ADD_VECTOR_EMPTY_METHOD(). Inherit and forward the requirement for a
+ * trailing semicolon from ADD_VECTOR_EMPTY_METHOD() to the caller.
+ */
+#define
[Libguestfs] [nbdkit PATCH 9/9] convert string_vector_(iter(free) + reset()) to string_vector_empty()
(Reimplement libnbd commit c7ff70101e8c, "convert
string_vector_(iter(free) + reset()) to string_vector_empty()",
2023-02-28.)
Convert the 5 string_vector_(iter(free) + reset()) call sites mentioned
previously to string_vector_empty().
Note that the converted code performs more cleanup steps in some cases
than strictly necessary, but the extra work is harmless, and arguably
beneficial for clarity / consistency.
Also note that the CLEANUP_FREE_*() macros remain unique to nbdkit; libnbd
does not use __attribute__((cleanup)), per libnbd commit f306e231d294
("common/utils: Add extensible string, based on vector", 2022-03-12).
Signed-off-by: Laszlo Ersek
---
common/utils/string-vector.h | 9 +
server/public.c | 3 +--
common/utils/environ.c | 3 +--
plugins/iso/iso.c| 3 +--
plugins/split/split.c| 3 +--
5 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
index 40446008b6a9..8245c5292e1a 100644
--- a/common/utils/string-vector.h
+++ b/common/utils/string-vector.h
@@ -41,13 +41,6 @@ DEFINE_POINTER_VECTOR_TYPE (string_vector, char *);
/* This frees both the array and the strings. */
#define CLEANUP_FREE_STRING_VECTOR \
- __attribute__ ((cleanup (cleanup_free_string_vector)))
-
-static void __attribute__ ((__unused__))
-cleanup_free_string_vector (string_vector *v)
-{
- string_vector_iter (v, (void*)free);
- string_vector_reset (v);
-}
+ __attribute__ ((cleanup (string_vector_empty)))
#endif /* STRING_VECTOR_H */
diff --git a/server/public.c b/server/public.c
index 5dfdfbf92ad9..d3fde6142a2d 100644
--- a/server/public.c
+++ b/server/public.c
@@ -1013,8 +1013,7 @@ free_interns (void)
struct connection *conn = threadlocal_get_conn ();
string_vector *list = conn ? >interns : _interns;
- string_vector_iter (list, (void *) free);
- string_vector_reset (list);
+ string_vector_empty (list);
}
static const char *
diff --git a/common/utils/environ.c b/common/utils/environ.c
index ddcf02666ca3..6777d4d48452 100644
--- a/common/utils/environ.c
+++ b/common/utils/environ.c
@@ -111,7 +111,6 @@ copy_environ (char **env, ...)
return ret.ptr;
error:
- string_vector_iter (, (void *) free);
- free (ret.ptr);
+ string_vector_empty ();
return NULL;
}
diff --git a/plugins/iso/iso.c b/plugins/iso/iso.c
index e8c915a4677e..14d4bd504801 100644
--- a/plugins/iso/iso.c
+++ b/plugins/iso/iso.c
@@ -128,8 +128,7 @@ make_iso (void)
static void
iso_unload (void)
{
- string_vector_iter (, (void *) free);
- free (dirs.ptr);
+ string_vector_empty ();
if (fd >= 0)
close (fd);
diff --git a/plugins/split/split.c b/plugins/split/split.c
index 3d909cdf6b12..2d12df66782e 100644
--- a/plugins/split/split.c
+++ b/plugins/split/split.c
@@ -64,8 +64,7 @@ static pthread_mutex_t lseek_lock = PTHREAD_MUTEX_INITIALIZER;
static void
split_unload (void)
{
- string_vector_iter (, (void *) free);
- free (filenames.ptr);
+ string_vector_empty ();
}
static int
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 6/9] common/include: extract STATIC_ASSERT() macro
We already have two use cases for static assertions (and soon we'll have
yet another). Namely:
- STATIC_ASSERT_UNSIGNED_INT() in "checked-overflow.h". Here, we use our
own trick, based on a negative-sized array typedef that's named with
NBDKIT_UNIQUE_NAME.
- static_assert() in "test-array-size.c". This uses the C11 macro called
static_assert() from , which wraps the C11 _Static_assert().
This is not really great: our baseline is C99, not C11 (per commit
762f7c9e5166, "tests: Set minimum compiler to ISO C99.", 2021-04-08) --
which is why the same assertions are repeated in the code as normal
runtime assert() calls, in case static_assert() is not defined.
Factor out our own STATIC_ASSERT(), from STATIC_ASSERT_UNSIGNED_INT().
Put it to use in "test-array-size.c", replacing both the runtime assert()s
and the compile-time static_assert()s. Note that for the latter, in order
to remain consistent with STATIC_ASSERT_UNSIGNED_INT(), we no longer
provide the *complaint* that we want the compiler to emit upon assertion
failure, but an identifier that stands for the predicate that we *expect*.
When uncommenting the negative test case in "test-array-size.c", the
resultant wall of compiler diagnostics includes the following entry:
> test-array-size.c:83:39: error: size of array
> ‘_array_size_macro_is_applied_to_array13’ is negative
This patch will have to be ported to nbdkit. (IMO we can implement the
change in libnbd first -- the "common" subdir is meant to be common, so no
particular precedence should be assumed.)
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit c593baab3c9c8b17317daece0694ec8b5fc6fb46)
---
common/include/Makefile.am| 1 +
common/include/checked-overflow.h | 8 ++--
common/include/static-assert.h| 48
common/include/test-array-size.c | 45 ++
4 files changed, 67 insertions(+), 35 deletions(-)
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index a9933e819908..bbf00641cd5c 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -49,6 +49,7 @@ EXTRA_DIST = \
nextnonzero.h \
random.h \
rounding.h \
+ static-assert.h \
tvdiff.h \
unique-name.h \
unix-path-max.h \
diff --git a/common/include/checked-overflow.h
b/common/include/checked-overflow.h
index a1852adcdc7a..4ec72387b332 100644
--- a/common/include/checked-overflow.h
+++ b/common/include/checked-overflow.h
@@ -53,6 +53,7 @@
#include
#include
+#include "static-assert.h"
#include "unique-name.h"
/* Add "a" and "b", both of (possibly different) unsigned integer types, and
@@ -173,11 +174,8 @@
*
* The expression "x" is not evaluated, unless it has variably modified type.
*/
-#define STATIC_ASSERT_UNSIGNED_INT(x)
\
- do {
\
-typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)[(typeof (x))-1 > 0 ? 1
: -1] \
- __attribute__ ((__unused__));
\
- } while (0)
+#define STATIC_ASSERT_UNSIGNED_INT(x) \
+ STATIC_ASSERT ((typeof (x))-1 > 0, _x_has_uint_type)
/* Assign the sum "a + b" to "*r", using uintmax_t modular arithmetic.
*
diff --git a/common/include/static-assert.h b/common/include/static-assert.h
new file mode 100644
index ..5a564f8946e5
--- /dev/null
+++ b/common/include/static-assert.h
@@ -0,0 +1,48 @@
+/* nbdkit
+ * Copyright (C) 2013-2023 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS
[Libguestfs] [nbdkit PATCH 3/9] build: Silence some cppcheck warnings [partial port]
From: Eric Blake
Original commit message:
I ran:
$ cppcheck --cppcheck-build-dir=cache/cppcheck -q -I . -I common/include/ \
-I include/ -I lib/ -I common/utils/ -i generator/ .
then inspected the output. cppcheck correctly flags:
copy/file-ops.c:423:10: portability: 'data' is of type 'void *'. When using
void pointers in calculations, the behaviour is undefined. Arithmetic
operations on 'void *' is a GNU C extension, which defines the 'sizeof(void)'
to be 1. [arithOperationsOnVoidPointer]
examples/copy-libev.c:312:10: style: Local variable 'is_zero' shadows outer
function [shadowFunction]
ocaml/nbd-c.c:419:16: portability: Shifting signed 32-bit value by 31 bits is
implementation-defined behaviour. See condition at line 416.
[shiftTooManyBitsSigned]
It also has several easy-to-fix false positives such as:
lib/opt.c:98:17: error: Uninitialized variable: err [uninitvar]
where the tool failed to see that err was always assigned by the
callback function, or
python/methods.c:357:7: style: Unused variable: ret [unusedVariable]
where the tool prefers redundant ; to delimit use of Python macros
from actual assignments. I even addressed some of its style
complaints, like:
copy/nbd-ops.c:466:16: style: The scope of the variable 'd' can be reduced...
examples/open-qcow2.c:12:23: style: Parameter 'argv' can be declared with const
[constParameter]
common/utils/test-vector.c:53:0: style: The function 'int64_vector_append' is
never used. [unusedFunction]
Then there are other false positives that are harder to silence (for
example, it complains about the macros ARRAY_SIZE(), ADD_OVERFLOW(),
MUL_OVERFLOW(); incorrectly suggesting to add const to callback
function signatures; ...) which I don't try to address here. So at
this point, it's not yet worth automating a run of cppcheck into our
CI.
Porting notes:
- Only pick the "common/utils/vector.h" and "common/utils/test-vector.c"
code changes, for bringing nbdkit's common/ subdir closer to that of
libnbd.
- Extend "__attribute__((__unused__))" to name##_duplicate(). This
function comes from nbdkit commit 78b72746e547 ("vector: Add
vector_duplicate function", 2021-08-22), and was not present in libnbd
at the time of libnbd commit 8fb8ffb53477 ("build: Silence some cppcheck
warnings", 2022-11-02).
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit 8fb8ffb534774e4fc9115b484277713285238078)
---
common/utils/vector.h | 20 ++--
common/utils/test-vector.c | 3 +--
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 347a85735e81..d3a669251fa8 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -94,7 +94,7 @@
* allocated and capacity is increased, but the vector length is \
* not increased and the new elements are not initialized.\
*/ \
- static inline int \
+ static inline int __attribute__((__unused__)) \
name##_reserve (name *v, size_t n)\
{ \
return generic_vector_reserve ((struct generic_vector *)v, n, \
@@ -104,7 +104,7 @@
/* Same as _reserve, but the allocation will be page aligned. Note \
* that the machine page size must be divisible by sizeof (type). \
*/ \
- static inline int \
+ static inline int __attribute__((__unused__)) \
name##_reserve_page_aligned (name *v, size_t n) \
{ \
return generic_vector_reserve_page_aligned ((struct generic_vector *)v, \
@@ -112,7 +112,7 @@
} \
\
/* Insert at i'th element. i=0 => beginning i=len => append */ \
- static inline int \
+ static inline int __attribute__((__unused__)) \
name##_insert (name *v, type elem, size_t i) \
{ \
assert (i <= v->len); \
@@ -126,14 +126,14 @@
} \
\
/* Append a new element to the end of the vector. */ \
- static inline int \
+ static inline int __attribute__((__unused__)) \
name##_append
[Libguestfs] [nbdkit PATCH 5/9] common/include: add TYPE_IS_POINTER() macro
Because the current definition of TYPE_IS_ARRAY() already contains a negation, defining TYPE_IS_POINTER() in terms of TYPE_IS_ARRAY() would lead to double negation, which I consider less than ideal style. Therefore, introduce TYPE_IS_POINTER() from scratch, and rebase TYPE_IS_ARRAY() on top of TYPE_IS_POINTER(). Suggested-by: Richard W.M. Jones Signed-off-by: Laszlo Ersek (cherry picked from libnbd commit 59dc9b7e3937b472d7203e6e0c96def120a1a54b) --- common/include/compiler-macros.h | 31 ++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/common/include/compiler-macros.h b/common/include/compiler-macros.h index 7933bb87a5bf..beb3f4fe4e5c 100644 --- a/common/include/compiler-macros.h +++ b/common/include/compiler-macros.h @@ -50,12 +50,39 @@ #define BUILD_BUG_UNLESS_TRUE(cond) \ (BUILD_BUG_STRUCT_SIZE (cond) - BUILD_BUG_STRUCT_SIZE (cond)) -#define TYPE_IS_ARRAY(a) \ - (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) +/* Each of TYPE_IS_POINTER() and TYPE_IS_ARRAY() produces a build failure if it + * is invoked with an object that has neither pointer-to-object type nor array + * type. + * + * C99 6.5.2.1 constrains one of the operands of the subscript operator to have + * pointer-to-object type, and the other operand to have integer type. In the + * replacement text of TYPE_IS_POINTER(), we use [0] as subscript (providing the + * integer operand), therefore the macro argument (p) is constrained to have + * pointer-to-object type. + * + * If TYPE_IS_POINTER() is invoked with a pointer that has pointer-to-object + * type, the constraint is directly satisfied, and TYPE_IS_POINTER() evaluates, + * at compile time, to 1. + * + * If TYPE_IS_POINTER() is invoked with an array, the constraint of the + * subscript operator is satisfied again -- because the array argument "decays" + * to a pointer to the array's initial element (C99 6.3.2p3) --, and + * TYPE_IS_POINTER() evaluates, at compile time, to 0. + * + * If TYPE_IS_POINTER() is invoked with an argument having any other type, then + * the subscript operator constraint is not satisfied, and C99 5.1.1.3p1 + * requires the emission of a diagnostic message -- the build breaks. Therefore, + * TYPE_IS_ARRAY() can be defined simply as the logical negation of + * TYPE_IS_POINTER(). + */ +#define TYPE_IS_POINTER(p) \ + (__builtin_types_compatible_p (typeof (p), typeof (&(p)[0]))) +#define TYPE_IS_ARRAY(a) (!TYPE_IS_POINTER (a)) #else /* __cplusplus */ #define BUILD_BUG_UNLESS_TRUE(cond) 0 +#define TYPE_IS_POINTER(p) 1 #define TYPE_IS_ARRAY(a) 1 #endif /* __cplusplus */ ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 7/9] force semicolon after DEFINE_VECTOR_TYPE() macro invocations
Original commit message:
Currently, a semicolon after a DEFINE_VECTOR_TYPE(...) macro invocation is
not needed, nor does our documentation in "common/utils/vector.h"
prescribe one. Furthermore, it breaks ISO C, which gcc reports with
"-Wpedantic":
> warning: ISO C does not allow extra ‘;’ outside of a function
> [-Wpedantic]
Our current usage is inconsistent; a proper subset of all our
DEFINE_VECTOR_TYPE() invocations is succeeded by a semicolon. We could
remove these semicolons, but that doesn't play nicely with Emacs's C
language parser. Thus, force the semicolon instead.
Porting notes:
- The call sites needing an update in nbdkit are distinct from those
updated in libnbd commit f3bc106dbc17 ("force semicolon after
DEFINE_VECTOR_TYPE() macro invocations", 2023-02-28). Thus,
"cherry-picking f3bc106dbc17" only applies to "common/utils/vector.h";
the rest has to be reimplemented from zero.
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit f3bc106dbc178a993db0e2006469da9ae0bf8db2)
---
common/utils/vector.h| 4 +++-
common/replacements/open_memstream.c | 2 +-
plugins/vddk/vddk.h | 2 +-
plugins/blkio/blkio.c| 2 +-
plugins/curl/pool.c | 2 +-
plugins/vddk/stats.c | 2 +-
filters/protect/protect.c| 2 +-
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 90bbabd64c56..5f5f96e97ab2 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -52,7 +52,7 @@
/* Use of this macro defines a new type called ‘name’ containing an
* extensible vector of ‘type’ elements. For example:
*
- * DEFINE_VECTOR_TYPE (string_vector, char *)
+ * DEFINE_VECTOR_TYPE (string_vector, char *);
*
* defines a new type called ‘string_vector’ as a vector of ‘char *’.
* You can create variables of this type:
@@ -195,6 +195,8 @@
return 0; \
} \
\
+ /* End with duplicate declaration, so callers must supply ';'. */ \
+ struct name
#define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
diff --git a/common/replacements/open_memstream.c
b/common/replacements/open_memstream.c
index e2d556e59fa2..bff880d3d9ae 100644
--- a/common/replacements/open_memstream.c
+++ b/common/replacements/open_memstream.c
@@ -62,7 +62,7 @@ struct file_to_memstream {
char **ptr;
size_t *size;
};
-DEFINE_VECTOR_TYPE (file_vector, struct file_to_memstream)
+DEFINE_VECTOR_TYPE (file_vector, struct file_to_memstream);
static file_vector files = empty_vector;
FILE *
diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h
index e8cd4bfde1f8..1f33a1e179e1 100644
--- a/plugins/vddk/vddk.h
+++ b/plugins/vddk/vddk.h
@@ -146,7 +146,7 @@ struct command {
enum { SUBMITTED, SUCCEEDED, FAILED } status;
};
-DEFINE_VECTOR_TYPE (command_queue, struct command *)
+DEFINE_VECTOR_TYPE (command_queue, struct command *);
/* The per-connection handle. */
struct vddk_handle {
diff --git a/plugins/blkio/blkio.c b/plugins/blkio/blkio.c
index 68430d222650..6eafa2877e50 100644
--- a/plugins/blkio/blkio.c
+++ b/plugins/blkio/blkio.c
@@ -57,7 +57,7 @@ struct property {
char *value;
bool value_needs_free;
};
-DEFINE_VECTOR_TYPE (properties, struct property)
+DEFINE_VECTOR_TYPE (properties, struct property);
static const char *driver = NULL; /* driver name - required */
static properties props = empty_vector; /* other command line params */
diff --git a/plugins/curl/pool.c b/plugins/curl/pool.c
index 1eb4db6404bf..b903cdc07d3e 100644
--- a/plugins/curl/pool.c
+++ b/plugins/curl/pool.c
@@ -84,7 +84,7 @@ static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
* handles are requested. Currently it does not shrink. It may grow
* up to 'connections' in length.
*/
-DEFINE_VECTOR_TYPE (curl_handle_list, struct curl_handle *)
+DEFINE_VECTOR_TYPE (curl_handle_list, struct curl_handle *);
static curl_handle_list curl_handles = empty_vector;
/* The condition is used when the curl handles vector is full and
diff --git a/plugins/vddk/stats.c b/plugins/vddk/stats.c
index fb23c6a5bbfe..c16338b1e774 100644
--- a/plugins/vddk/stats.c
+++ b/plugins/vddk/stats.c
@@ -60,7 +60,7 @@ pthread_mutex_t stats_lock = PTHREAD_MUTEX_INITIALIZER;
#undef STUB
#undef OPTIONAL_STUB
-DEFINE_VECTOR_TYPE (statlist, struct vddk_stat)
+DEFINE_VECTOR_TYPE (statlist, struct vddk_stat);
static int
stat_compare (const void *vp1, const void *vp2)
diff --git a/filters/protect/protect.c b/filters/protect/protect.c
index 4e22a24bbcc9..b9edd062a367 100644
--- a/filters/protect/protect.c
+++ b/filters/protect/protect.c
@@ -57,7 +57,7 @@
* 'range_list' stores the list of protected ranges, unsorted.
*/
struct range { uint64_t start, end; const char
[Libguestfs] [nbdkit PATCH 2/9] common/include: Add a function to compute log2(unsigned long)
From: "Richard W.M. Jones"
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit 27a9bb45c70dbb1112cab6bdc1a61b21f78bc6f4)
---
configure.ac | 3 +++
common/include/ispowerof2.h | 13 +
common/include/test-ispowerof2.c | 10 ++
3 files changed, 26 insertions(+)
diff --git a/configure.ac b/configure.ac
index 9075b0eabda1..597097922831 100644
--- a/configure.ac
+++ b/configure.ac
@@ -344,6 +344,9 @@ test (void)
]
)
+dnl Check sizeof long.
+AC_CHECK_SIZEOF(long)
+
dnl Check for other headers, all optional.
AC_CHECK_HEADERS([\
alloca.h \
diff --git a/common/include/ispowerof2.h b/common/include/ispowerof2.h
index b9a5cc7caf01..c5bcb0c81077 100644
--- a/common/include/ispowerof2.h
+++ b/common/include/ispowerof2.h
@@ -46,4 +46,17 @@ is_power_of_2 (unsigned long v)
return v && ((v & (v - 1)) == 0);
}
+/* Calculate log2(v) which is the size of the equivalent bit shift
+ * for a power of 2. For example log_2_bits (512) == 9.
+ *
+ * Note this is undefined for v == 0.
+ *
+ * __builtin_clzl is available in GCC and clang.
+ */
+static inline int
+log_2_bits (unsigned long v)
+{
+ return SIZEOF_LONG*8 - __builtin_clzl (v) - 1;
+}
+
#endif /* NBDKIT_ISPOWEROF2_H */
diff --git a/common/include/test-ispowerof2.c b/common/include/test-ispowerof2.c
index 9f904b2fc5e8..fd880fa3b551 100644
--- a/common/include/test-ispowerof2.c
+++ b/common/include/test-ispowerof2.c
@@ -58,5 +58,15 @@ main (void)
for (i = 4; i <= 0x8000; i <<= 1)
assert (! is_power_of_2 (i-1));
+ /* Check log_2_bits on some known values. */
+ assert (log_2_bits (1) == 0);
+ assert (log_2_bits (512) == 9);
+ assert (log_2_bits (4096) == 12);
+ assert (log_2_bits (0x8000) == 31);
+#if SIZEOF_LONG == 8
+ assert (log_2_bits (0x1) == 32);
+ assert (log_2_bits (0x8000) == 63);
+#endif
+
exit (EXIT_SUCCESS);
}
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 1/9] common/utils/vector.h: Remove stale reference to `size`
From: Nir Soffer
Original commit message:
Update stale comments to use `len` instead of `size`.
Fixes: commit cc0567e9aed7e6b40a44bf8eac0a262ac7314fec
Signed-off-by: Nir Soffer
Porting notes:
(1) Libnbd commit cc0567e9aed7 ("common/utils/vector: Rename `size` to
`len`", 2021-10-31) renamed the "size" field of our vector data structure
to "len", but forgot to update the field name in the comments of
"vector.h".
(2) Libnbd commit cc0567e9aed7 was ported to nbdkit as commit 0b0eece73f04
("common/utils/vector: Rename `size` to `len`", 2021-11-07). The port did
*more* than the original: the port also renamed the field in the comments
of "vector.h", plus it clarified the leading comment on name##_reserve().
(3) Libnbd commit 3d6be922e701 ("common/utils/vector.h: Remove stale
reference to `size`", 2021-11-05) fixed up earlier libnbd commit
cc0567e9aed7 by cleaning up the comments, probably striving for synching
libnbd's "vector.h" with nbdkit's. However, this libnbd fixup introduced a
*new* wrapping difference, relative to nbdkit, to the previously mentioned
comment above name##_reserve().
(4) By porting libnbd commit 3d6be922e701 to nbdkit, we can unify the
wrapping of the name##_reserve() comment.
Signed-off-by: Laszlo Ersek
(cherry picked from libnbd commit 3d6be922e70153a220c7e5c7ae2425d1ff7197f8)
---
common/utils/vector.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 29cd0bddfc74..347a85735e81 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -91,8 +91,8 @@
typedef struct name name; \
\
/* Reserve n elements at the end of the vector. Note space is\
- * allocated and capacity is increased, but the vector length \
- * is not increased and the new elements are not initialized. \
+ * allocated and capacity is increased, but the vector length is \
+ * not increased and the new elements are not initialized.\
*/ \
static inline int \
name##_reserve (name *v, size_t n)\
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH 0/9] common: catch up with libnbd
This is the counterpart series for [libnbd PATCH 0/6] common: catch up with nbdkit 20230301114027.336230-1-lersek@redhat.com">http://mid.mail-archive.com/20230301114027.336230-1-lersek@redhat.com Contrarily to my prognosis in that cover letter, this series does ultimately extend up to libnbd v1.15.11 in one go. As a result, once this series is merged, the common subdirs between libnbd @ d169661119f6 and nbdkit @ end-of-this-series will be in sync as much as reasonable. Laszlo Eric Blake (1): build: Silence some cppcheck warnings [partial port] Laszlo Ersek (6): use space consistently in function and function-like macro invocations common/include: add TYPE_IS_POINTER() macro common/include: extract STATIC_ASSERT() macro force semicolon after DEFINE_VECTOR_TYPE() macro invocations vector: introduce DEFINE_POINTER_VECTOR_TYPE() convert string_vector_(iter(free) + reset()) to string_vector_empty() Nir Soffer (1): common/utils/vector.h: Remove stale reference to `size` Richard W.M. Jones (1): common/include: Add a function to compute log2(unsigned long) Makefile.am| 1 + common/allocators/allocator-internal.h | 2 +- common/allocators/allocator.c | 2 +- common/allocators/allocator.h | 18 +- common/allocators/malloc.c | 4 +- common/allocators/sparse.c | 4 +- common/allocators/zstd.c | 4 +- common/bitmap/bitmap.c | 2 +- common/bitmap/bitmap.h | 16 +- common/gpt/efi-crc32.h | 2 +- common/include/Makefile.am | 1 + common/include/array-size.h| 2 +- common/include/byte-swapping.h | 24 +-- common/include/checked-overflow.h | 42 ++--- common/include/compiler-macros.h | 33 +++- common/include/ispowerof2.h| 13 ++ common/include/iszero.h| 2 +- common/include/minmax.h| 4 +- common/include/nextnonzero.h | 2 +- common/include/random.h| 4 +- common/include/static-assert.h | 48 + common/include/test-array-size.c | 71 +++- common/include/test-ispowerof2.c | 10 + common/include/test-random.c | 192 ++-- common/protocol/nbd-protocol.h | 12 +- common/regions/regions.c | 2 +- common/regions/regions.h | 12 +- common/replacements/open_memstream.c | 4 +- common/replacements/strndup.c | 2 +- common/replacements/strndup.h | 2 +- common/utils/cleanup.h | 24 +-- common/utils/const-string-vector.h | 4 +- common/utils/environ.c | 3 +- common/utils/exit-with-parent.c| 6 +- common/utils/nbdkit-string.h | 4 +- common/utils/string-vector.h | 11 +- common/utils/test-vector.c | 7 +- common/utils/utils.c | 2 +- common/utils/utils.h | 2 +- common/utils/vector.h | 63 +-- configure.ac | 3 + filters/blocksize-policy/policy.c | 2 +- filters/blocksize/blocksize.c | 2 +- filters/cache/blk.h| 12 +- filters/cache/cache.c | 2 +- filters/cache/reclaim.c| 2 +- filters/checkwrite/checkwrite.c| 2 +- filters/cow/blk.h | 10 +- filters/cow/cow.c | 2 +- filters/ddrescue/ddrescue.c| 4 +- filters/delay/delay.c | 4 +- filters/error/error.c | 2 +- filters/exitlast/exitlast.c| 2 +- filters/exitwhen/exitwhen.c| 8 +- filters/exportname/exportname.c| 2 +- filters/ext2/ext2.c| 2 +- filters/ext2/io.h | 4 +- filters/extentlist/extentlist.c| 4 +- filters/fua/fua.c | 2 +- filters/gzip/gzip.c| 2 +- filters/ip/ip.c| 12 +- filters/limit/limit.c | 2 +- filters/log/log.c | 2 +- filters/log/log.h | 8 +- filters/luks/luks-encryption.c | 4 +- filters/luks/luks.c| 2 +- filters/multi-conn/multi-conn.c| 6 +- filters/nocache/nocache.c | 2 +- filters/noextents/noextents.c | 2 +- filters/nofilter/nofilter.c| 2 +- filters/noparallel/noparallel.c| 2 +- filters/nozero/nozero.c| 2 +- filters/offset/offset.c| 2 +- filters/partition/partition-gpt.c | 4 +- filters/partition/partition.c | 2 +- filters/pause/pause.c | 2 +- filters/protect/protect.c | 4 +- filters/rate/rate.c| 4 +- filters/readahead/readahead.c | 2
Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
On 3/2/23 22:59, Richard W.M. Jones wrote:
> On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote:
>> On 3/2/23 20:36, Richard W.M. Jones wrote:
>>> I can probably do an outline of a patch if you need it, but
>>> not today.
>>
>> Yes, sure. But speaking of the ways to influence the behaviour of a
>> command line tool: there're also env variables, which are being used by
>> v2v as well (for instance, for setting the tools ISO location). So what
>> if we introduce an env variable here instead? I presume it would be
>> much easier to implement. Smth like:
>
> The environment variable is a bit of a mistake too. It'd be better to
> specify that through the command line; and in fact that's what I went
> with for the new virt-customize --inject-virtio-win option:
>
>--inject-virtio-win METHOD
>Inject virtio-win drivers into a Windows guest. These drivers add
>virtio accelerated drivers suitable when running on top of a
>hypervisor that supports virtio (such as qemu/KVM). The operation
>also adjusts the Windows Registry so that the drivers are installed
>when the guest boots.
>
>The parameter can be one of:
>
>ISO The path to the ISO image containing the virtio-win drivers
>(eg. /usr/share/virtio-win/virtio-win.iso).
>[etc]
>
>>> diff --git a/mlcustomize/inject_virtio_win.ml
>>> b/mlcustomize/inject_virtio_win.ml
>>> index 62f7710..562f055 100644
>>> --- a/mlcustomize/inject_virtio_win.ml
>>> +++ b/mlcustomize/inject_virtio_win.ml
>>> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg =
>>> (* Can we install the block driver? *)
>>> let block : block_type =
>>>let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in
>>> + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in
>>> + let filenames =
>>> +match viostor_drv_env with
>>> +| None -> filenames
>>> +| Some str -> str :: filenames in
>>>let viostor_driver = try (
>>> Some (
>>>List.find (
>>
>> As you can see here the default doesn't change if the variable isn't
>> provided at all (or it is, but is set to a non-existing driver).
>> What do you think of such a solution?
>
> I prefer the command line option even though it's a little bit more
> work. Offer above still stands if you can't work out the complexities
> of OCaml command line parsing.
It's not about the command line parsing complexities, but rather about
the proper way to pass that option from the top level of the call stack
(convert ()) down to the inject_virtio_win_drivers (). That'd require
adding one more extra argument to all the calls along this call stack,
which might look a bit clumsy. Isn't there a more elegant way?
So having a sketch in that regard would be indeed much appreciated.
>
> Rich.
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote:
> On 3/2/23 20:36, Richard W.M. Jones wrote:
> > I can probably do an outline of a patch if you need it, but
> > not today.
>
> Yes, sure. But speaking of the ways to influence the behaviour of a
> command line tool: there're also env variables, which are being used by
> v2v as well (for instance, for setting the tools ISO location). So what
> if we introduce an env variable here instead? I presume it would be
> much easier to implement. Smth like:
The environment variable is a bit of a mistake too. It'd be better to
specify that through the command line; and in fact that's what I went
with for the new virt-customize --inject-virtio-win option:
--inject-virtio-win METHOD
Inject virtio-win drivers into a Windows guest. These drivers add
virtio accelerated drivers suitable when running on top of a
hypervisor that supports virtio (such as qemu/KVM). The operation
also adjusts the Windows Registry so that the drivers are installed
when the guest boots.
The parameter can be one of:
ISO The path to the ISO image containing the virtio-win drivers
(eg. /usr/share/virtio-win/virtio-win.iso).
[etc]
> > diff --git a/mlcustomize/inject_virtio_win.ml
> > b/mlcustomize/inject_virtio_win.ml
> > index 62f7710..562f055 100644
> > --- a/mlcustomize/inject_virtio_win.ml
> > +++ b/mlcustomize/inject_virtio_win.ml
> > @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg =
> > (* Can we install the block driver? *)
> > let block : block_type =
> >let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in
> > + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in
> > + let filenames =
> > +match viostor_drv_env with
> > +| None -> filenames
> > +| Some str -> str :: filenames in
> >let viostor_driver = try (
> > Some (
> >List.find (
>
> As you can see here the default doesn't change if the variable isn't
> provided at all (or it is, but is set to a non-existing driver).
> What do you think of such a solution?
I prefer the command line option even though it's a little bit more
work. Offer above still stands if you can't work out the complexities
of OCaml command line parsing.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
On 3/2/23 20:36, Richard W.M. Jones wrote:
> On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev wrote:
>> On 2/28/23 16:39, Richard W.M. Jones wrote:
>>> On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:
On 2/28/23 14:01, Richard W.M. Jones wrote:
> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote:
>> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439
>> ("Remove guestcaps_block_type Virtio_SCSI") support for installing
>> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for
>> bringing this feature back have been out there for a while. E.g. I've
>> found a corresponding issue which is still open [1].
>>
>> The code in b28cd1dc, f0afc439 was removed due to removing the old
>> in-place
>> support. However, having the new in-place support present and bringing
>> this same code (partially) back with several additions and improvements,
>> I'm able to successfully convert and boot a Win guest with a virtio-scsi
>> disk controller. So please consider the following implementation of
>> this feature.
>>
>> [1] https://github.com/libguestfs/virt-v2v/issues/12
>
> So you're right that we ended up removing virtio-scsi support because
> in-place conversion was temporarily removed, and it wasn't restored
> when the new virt-v2v-in-place tool was added.
>
> I looked at the patches, and I don't have any objection to 1-4.
>
> Patch 5 is the tricky one because it changes the default, but we
> (Red Hat) are likely to stick with virtio-blk for better or worse.
>
> Could we make this configurable? We've traditionally shied away from
> adding virt-v2v command line options which are purely for fine-tuning
> conversions. The reason for this is that when you're doing bulk
> conversions of hundreds of VMs from VMware, you want virt-v2v to do
> "the best it can", and it's not really feasible to hand configure
> every conversion. (There are some historical exceptions to this, like
> --root, but don't look at those!) I know the bulk conversion case
> doesn't apply to Virtuozzo, but it does matter for us.
>
> It could be argued that this isn't the same, because if we're bulk
> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM,
> but per target hypervisor. So a command line option would be OK in
> this case.
>
> BTW previously we "configured" this preference through the input
> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this
> time around.
Ah, that's new information to me. I thought the "rcaps" (requested caps,
derived from the input XML) was exactly what was missing here. Not
because of the particular reason(s) that may have motivated rcaps in the
past, but because in the use case at hand, the tweaking of the domain
XML precedes the conversion (more like, the injection of the desired
virtio drivers).
Why was it a terrible idea?
>>>
>>> It was a hack. For copying (normal) conversions -i libvirtxml
>>> describes the input metadata. For in-place conversions "input" is a
>>> fuzzy concept, so it was overloaded to describe the conversion
>>> preferences. We can do better with a command line parameter.
>>
>>
>> Could you please elaborate on why is that a fuzzy concept? I understand
>> that bringing back the entire requested capabilities (rcaps) framework
>> might be undesirable, but the idea of choosing the block driver based on
>> the source libvirt XML doesn't seem very wrong to me. Even if we
>> perform some modifications on that source XML (which I'm not sure of) we
>> end up with a VM config having a particular set of devices, and by that
>> point all fuzziness should be eliminated. Am I missing something?
>
> It's mixing up two concepts, the input metadata and the conversion
> parameters. Plus it's plain weird. And hard for end users to do.
> Command line options would be how you normally influence the behaviour
> of a command line tool.
>
>>>
> Anyway if you can think of a better version of patch 5 then I think we
> can accept this.
ITYM "if you *cannot* think of a better version"...
But anyway, if a command line option is acceptable to you, I'm perfectly
fine with it as well. I'd like to avoid changing the default. I quite
foresee tens of QE test runs breaking, resulting in as many bug reports,
and then us eventually reverting the change-of-the-default downstream.
I'd rather favor an experimental command line switch.
>>>
>>> Yes I'm sure we don't want to change the default; or at least we
>>> (meaning Red Hat, downstream) want to choose whether and when to
>>> change the default.
>>
>> Speaking of the command line option: if we end up with such a solution,
>> do you think it should be specific to and only applicable in the
>>
Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev wrote:
> On 2/28/23 16:39, Richard W.M. Jones wrote:
> > On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:
> >> On 2/28/23 14:01, Richard W.M. Jones wrote:
> >>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote:
> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439
> ("Remove guestcaps_block_type Virtio_SCSI") support for installing
> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for
> bringing this feature back have been out there for a while. E.g. I've
> found a corresponding issue which is still open [1].
>
> The code in b28cd1dc, f0afc439 was removed due to removing the old
> in-place
> support. However, having the new in-place support present and bringing
> this same code (partially) back with several additions and improvements,
> I'm able to successfully convert and boot a Win guest with a virtio-scsi
> disk controller. So please consider the following implementation of
> this feature.
>
> [1] https://github.com/libguestfs/virt-v2v/issues/12
> >>>
> >>> So you're right that we ended up removing virtio-scsi support because
> >>> in-place conversion was temporarily removed, and it wasn't restored
> >>> when the new virt-v2v-in-place tool was added.
> >>>
> >>> I looked at the patches, and I don't have any objection to 1-4.
> >>>
> >>> Patch 5 is the tricky one because it changes the default, but we
> >>> (Red Hat) are likely to stick with virtio-blk for better or worse.
> >>>
> >>> Could we make this configurable? We've traditionally shied away from
> >>> adding virt-v2v command line options which are purely for fine-tuning
> >>> conversions. The reason for this is that when you're doing bulk
> >>> conversions of hundreds of VMs from VMware, you want virt-v2v to do
> >>> "the best it can", and it's not really feasible to hand configure
> >>> every conversion. (There are some historical exceptions to this, like
> >>> --root, but don't look at those!) I know the bulk conversion case
> >>> doesn't apply to Virtuozzo, but it does matter for us.
> >>>
> >>> It could be argued that this isn't the same, because if we're bulk
> >>> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM,
> >>> but per target hypervisor. So a command line option would be OK in
> >>> this case.
> >>>
> >>> BTW previously we "configured" this preference through the input
> >>> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this
> >>> time around.
> >>
> >> Ah, that's new information to me. I thought the "rcaps" (requested caps,
> >> derived from the input XML) was exactly what was missing here. Not
> >> because of the particular reason(s) that may have motivated rcaps in the
> >> past, but because in the use case at hand, the tweaking of the domain
> >> XML precedes the conversion (more like, the injection of the desired
> >> virtio drivers).
> >>
> >> Why was it a terrible idea?
> >
> > It was a hack. For copying (normal) conversions -i libvirtxml
> > describes the input metadata. For in-place conversions "input" is a
> > fuzzy concept, so it was overloaded to describe the conversion
> > preferences. We can do better with a command line parameter.
>
>
> Could you please elaborate on why is that a fuzzy concept? I understand
> that bringing back the entire requested capabilities (rcaps) framework
> might be undesirable, but the idea of choosing the block driver based on
> the source libvirt XML doesn't seem very wrong to me. Even if we
> perform some modifications on that source XML (which I'm not sure of) we
> end up with a VM config having a particular set of devices, and by that
> point all fuzziness should be eliminated. Am I missing something?
It's mixing up two concepts, the input metadata and the conversion
parameters. Plus it's plain weird. And hard for end users to do.
Command line options would be how you normally influence the behaviour
of a command line tool.
> >
> >>> Anyway if you can think of a better version of patch 5 then I think we
> >>> can accept this.
> >>
> >> ITYM "if you *cannot* think of a better version"...
> >>
> >> But anyway, if a command line option is acceptable to you, I'm perfectly
> >> fine with it as well. I'd like to avoid changing the default. I quite
> >> foresee tens of QE test runs breaking, resulting in as many bug reports,
> >> and then us eventually reverting the change-of-the-default downstream.
> >> I'd rather favor an experimental command line switch.
> >
> > Yes I'm sure we don't want to change the default; or at least we
> > (meaning Red Hat, downstream) want to choose whether and when to
> > change the default.
>
> Speaking of the command line option: if we end up with such a solution,
> do you think it should be specific to and only applicable in the
> in-place mode?
I think it would apply to all of the virt-v2v* tools,
Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows
On 2/28/23 16:39, Richard W.M. Jones wrote:
> On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:
>> On 2/28/23 14:01, Richard W.M. Jones wrote:
>>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote:
Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439
("Remove guestcaps_block_type Virtio_SCSI") support for installing
virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for
bringing this feature back have been out there for a while. E.g. I've
found a corresponding issue which is still open [1].
The code in b28cd1dc, f0afc439 was removed due to removing the old in-place
support. However, having the new in-place support present and bringing
this same code (partially) back with several additions and improvements,
I'm able to successfully convert and boot a Win guest with a virtio-scsi
disk controller. So please consider the following implementation of
this feature.
[1] https://github.com/libguestfs/virt-v2v/issues/12
>>>
>>> So you're right that we ended up removing virtio-scsi support because
>>> in-place conversion was temporarily removed, and it wasn't restored
>>> when the new virt-v2v-in-place tool was added.
>>>
>>> I looked at the patches, and I don't have any objection to 1-4.
>>>
>>> Patch 5 is the tricky one because it changes the default, but we
>>> (Red Hat) are likely to stick with virtio-blk for better or worse.
>>>
>>> Could we make this configurable? We've traditionally shied away from
>>> adding virt-v2v command line options which are purely for fine-tuning
>>> conversions. The reason for this is that when you're doing bulk
>>> conversions of hundreds of VMs from VMware, you want virt-v2v to do
>>> "the best it can", and it's not really feasible to hand configure
>>> every conversion. (There are some historical exceptions to this, like
>>> --root, but don't look at those!) I know the bulk conversion case
>>> doesn't apply to Virtuozzo, but it does matter for us.
>>>
>>> It could be argued that this isn't the same, because if we're bulk
>>> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM,
>>> but per target hypervisor. So a command line option would be OK in
>>> this case.
>>>
>>> BTW previously we "configured" this preference through the input
>>> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this
>>> time around.
>>
>> Ah, that's new information to me. I thought the "rcaps" (requested caps,
>> derived from the input XML) was exactly what was missing here. Not
>> because of the particular reason(s) that may have motivated rcaps in the
>> past, but because in the use case at hand, the tweaking of the domain
>> XML precedes the conversion (more like, the injection of the desired
>> virtio drivers).
>>
>> Why was it a terrible idea?
>
> It was a hack. For copying (normal) conversions -i libvirtxml
> describes the input metadata. For in-place conversions "input" is a
> fuzzy concept, so it was overloaded to describe the conversion
> preferences. We can do better with a command line parameter.
Could you please elaborate on why is that a fuzzy concept? I understand
that bringing back the entire requested capabilities (rcaps) framework
might be undesirable, but the idea of choosing the block driver based on
the source libvirt XML doesn't seem very wrong to me. Even if we
perform some modifications on that source XML (which I'm not sure of) we
end up with a VM config having a particular set of devices, and by that
point all fuzziness should be eliminated. Am I missing something?
>
>>> Anyway if you can think of a better version of patch 5 then I think we
>>> can accept this.
>>
>> ITYM "if you *cannot* think of a better version"...
>>
>> But anyway, if a command line option is acceptable to you, I'm perfectly
>> fine with it as well. I'd like to avoid changing the default. I quite
>> foresee tens of QE test runs breaking, resulting in as many bug reports,
>> and then us eventually reverting the change-of-the-default downstream.
>> I'd rather favor an experimental command line switch.
>
> Yes I'm sure we don't want to change the default; or at least we
> (meaning Red Hat, downstream) want to choose whether and when to
> change the default.
Speaking of the command line option: if we end up with such a solution,
do you think it should be specific to and only applicable in the
in-place mode?
>
> BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora).
>
> Thanks,
>
> Rich.
>
P.S. Resent, the original mail ended up off the mailing list.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Checksums and other verification
On Thu, Mar 2, 2023 at 10:46 AM Richard W.M. Jones wrote: > > On Mon, Feb 27, 2023 at 07:09:33PM +0200, Nir Soffer wrote: > > On Mon, Feb 27, 2023 at 6:41 PM Richard W.M. Jones > > wrote: > > > I think it would be more useful if (or in addition) it could compute > > > the checksum of a stream which is being converted with 'qemu-img > > > convert'. Extra points if it can compute the checksum over either the > > > input or output stream. > > > > I thought about this, it could be a filter that you add in the graph > > that gives you checksum as a side effect of copying. But this requires > > disabling unordered writes, which is pretty bad for performance. > > > > But even if you compute the checksum during a transfer, you want to > > verify it by reading the transferred data from storage. Once you computed > > the checksum you can keep it for verifying the same image in the future. > > The use-case I have in mind is being able to verify a download when > you already know the checksum and are copying / converting the image > in flight. > > eg: You are asked to download https://example.com/distro-cloud.qcow2 > with some published checksum and you will on the fly download and > convert this to raw, but want to verify the checksum (of the qcow2) > during the conversion step. (Or at some point, but during the convert > avoids having to spool the image locally.) I'm thinking about the same flow. I think the best way to verify is: 1. The remote server publishes a block-checksum of the image 2. The system gets the block-checksum from the server (from http header?) 3. The system pulls data from the server, pushes to the target disk in the wanted format 4. The system computes a checksum of the target disk This way you verify the entire pipeline including the storage. If we compute a checksum during the conversion, we verify only that we got the correct data from the server. If we care only about verifying the transfer from the server, we can compute the checksum during the download, which is likely to be sequential (so easy to integrate with blkhash) If we want to validate nbdcopy, it will be much harder to compute a checksum inside nbdcopy because it does not stream the data in order. Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit PATCH 0/5] ci: Get to green status on FreeBSD and MacOS
On 3/1/23 23:10, Eric Blake wrote: > On Wed, Mar 01, 2023 at 06:43:11PM +0100, Laszlo Ersek wrote: >> On 3/1/23 17:54, Eric Blake wrote: >>> I took the easy route of crippling what I couldn't get working, on the >>> grounds that partial coverage is better than none now that we have >>> Cirrus CI checking commits on additional platforms. >>> >>> This series got me to a green checkmark: >>> https://gitlab.com/ebblake/nbdkit/-/pipelines/793156983 >>> >>> but depends on an as-yet uncommitted patch in libvirt-ci: >>> https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/360 >>> >>> Eric Blake (5): >>> ci: Expose more env vars needed by build.sh >>> ci: Another attempt at MacOS >>> rust: Skip CI builds on MacOS >>> golang: Skip CI builds on MacOS and newer FreeBSD >>> perl: Skip CI builds on newer FreeBSD >>> >>> .gitlab-ci.yml| 3 +++ >>> ci/cirrus/build.yml | 3 +++ >>> ci/cirrus/macos-12.vars | 4 ++-- >>> ci/gitlab.yml | 7 +++ >>> ci/gitlab/build-templates.yml | 31 ++- >>> ci/gitlab/builds.yml | 11 --- >>> ci/manifest.yml | 20 +++- >>> 7 files changed, 68 insertions(+), 11 deletions(-) >>> >> >> series >> Acked-by: Laszlo Ersek > > Thanks; now in as 99c57eef..648a7909 > >> >> One question (for my understanding) about the context of patch#1: >> >>-e "s|[@]PYPI_PKGS@|$PYPI_PKGS|g" >> >> what's this [@] notation? Why do we need to sink the at-sign into a >> bracket expression? > > In the context of this patch, copy-and-paste from existing paradigm. > More historically, I can think of two possible sources, both from GNU > heritage: > > For a project using autoconf, it is explicitly documented that any > @STR@ expression in a .in file will be substituted with the contents > of the shell variable $STR at the time that ./configure has finished > learning about the current system. But it can be expensive to write a > lot of .in templates, so autoconf actually encourages a style where > you only need to generate Makefile.in (which DOES use @FOO@ > substitution from autoconf) where the resulting Makefile in turn > applies subsequent @FOO@ substitution in other target files not under > autoconf's control. But when the make rule to do that substitution > lives in a file that itself undergoes @ expansion, you need a way to > quote which instances get expanded at make time rather than configure > time. For example, look under 'make install' in: > https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.71/autoconf.html#Installation-Directory-Variables > > | Some details are noteworthy: > | > | ‘@bindir[@]’ > | > | The brackets prevent configure from replacing ‘@bindir@’ in the Sed > expression itself. Brackets are preferable to a backslash here, since Posix > says ‘\@’ is not portable. > > The other place where I've seen common use of a single-character > bracket expression as an escape for an otherwise literal regex is in > gnulib's tree-wide syntax-checking. And of course, you don't want to > hit a false-positive of the regex itself being an otherwise forbidden > string. For example: > https://git.savannah.gnu.org/cgit/gnulib.git/tree/top/maint.mk#n371 > > And since libvirt used to use gnulib's maint.mk regex-based syntax > checking, before weaning itself off gnulib, this may be why libvirt-ci > (the source of lcitool) still utilizes @FOO@ regex with [@] escaping. > > Or, it could be because gitlab and/or Cirrus do their own @FOO@ > expansions at job kickoff, borrowing from the earlier practice of > autoconf. I don't see any other @FOO@ listings in .gitlab-ci.yml or > ci/gitlab.yml, but I do see that the resulting ci/cirrus/$NAME.yml > produced by the sed substitution during gitlab time DOES contain other > things like @CIRRUS_VM_INSTANCE_TYPE@ that we are NOT touching on the > gitlab side, so it seems fairly certain that Cirrus does some @FOO@ > expansion, and timing which part of the job does which expansions may > matter. At this point, erring on the side of caution and coding by > copy/paste is simpler than determining if dropping the quoting is > feasible. > Thanks! :) Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [nbdkit PATCH 0/5] ci: Get to green status on FreeBSD and MacOS
ACK series, thanks for fixing this. 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Checksums and other verification
On Mon, Feb 27, 2023 at 07:09:33PM +0200, Nir Soffer wrote: > On Mon, Feb 27, 2023 at 6:41 PM Richard W.M. Jones wrote: > > I think it would be more useful if (or in addition) it could compute > > the checksum of a stream which is being converted with 'qemu-img > > convert'. Extra points if it can compute the checksum over either the > > input or output stream. > > I thought about this, it could be a filter that you add in the graph > that gives you checksum as a side effect of copying. But this requires > disabling unordered writes, which is pretty bad for performance. > > But even if you compute the checksum during a transfer, you want to > verify it by reading the transferred data from storage. Once you computed > the checksum you can keep it for verifying the same image in the future. The use-case I have in mind is being able to verify a download when you already know the checksum and are copying / converting the image in flight. eg: You are asked to download https://example.com/distro-cloud.qcow2 with some published checksum and you will on the fly download and convert this to raw, but want to verify the checksum (of the qcow2) during the conversion step. (Or at some point, but during the convert avoids having to spool the image locally.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
