[PULL v2 10/11] nbd/server: use bdrv_dirty_bitmap_next_dirty_area
From: Vladimir Sementsov-Ogievskiy
Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since
bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface,
we'll never exceed requested region with last chunk. So, we don't need
dont_fragment, and bitmap_to_extents() interface becomes clean enough
to not require any comment.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Eric Blake
Message-id: 20200205112041.6003-10-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
nbd/server.c | 59 +---
1 file changed, 19 insertions(+), 40 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index f90bb33a75..02b1ed0801 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2068,57 +2068,36 @@ static int nbd_co_send_block_status(NBDClient *client,
uint64_t handle,
return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
-/*
- * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
- * final extent may exceed the original @length.
- */
+/* Populate @ea from a dirty bitmap. */
static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
uint64_t offset, uint64_t length,
- NBDExtentArray *ea, bool dont_fragment)
+ NBDExtentArray *es)
{
-uint64_t begin = offset, end = offset;
-uint64_t overall_end = offset + length;
-BdrvDirtyBitmapIter *it;
-bool dirty;
+int64_t start, dirty_start, dirty_count;
+int64_t end = offset + length;
+bool full = false;
bdrv_dirty_bitmap_lock(bitmap);
-it = bdrv_dirty_iter_new(bitmap);
-dirty = bdrv_dirty_bitmap_get_locked(bitmap, offset);
-
-while (begin < overall_end) {
-bool next_dirty = !dirty;
-
-if (dirty) {
-end = bdrv_dirty_bitmap_next_zero(bitmap, begin, INT64_MAX);
-} else {
-bdrv_set_dirty_iter(it, begin);
-end = bdrv_dirty_iter_next(it);
-}
-if (end == -1 || end - begin > UINT32_MAX) {
-/* Cap to an aligned value < 4G beyond begin. */
-end = MIN(bdrv_dirty_bitmap_size(bitmap),
- begin + UINT32_MAX + 1 -
- bdrv_dirty_bitmap_granularity(bitmap));
-next_dirty = dirty;
-}
-if (dont_fragment && end > overall_end) {
-end = overall_end;
-}
-
-if (nbd_extent_array_add(ea, end - begin,
- dirty ? NBD_STATE_DIRTY : 0) < 0) {
+for (start = offset;
+ bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX,
+ _start, _count);
+ start = dirty_start + dirty_count)
+{
+if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) ||
+(nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0))
+{
+full = true;
break;
}
-begin = end;
-dirty = next_dirty;
}
-bdrv_dirty_iter_free(it);
+if (!full) {
+/* last non dirty extent */
+nbd_extent_array_add(es, end - start, 0);
+}
bdrv_dirty_bitmap_unlock(bitmap);
-
-assert(offset < end);
}
static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
@@ -2129,7 +2108,7 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t
handle,
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
-bitmap_to_extents(bitmap, offset, length, ea, dont_fragment);
+bitmap_to_extents(bitmap, offset, length, ea);
return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
--
2.21.1
[PULL v2 11/11] block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty
From: Vladimir Sementsov-Ogievskiy
store_bitmap_data() loop does bdrv_set_dirty_iter() on each iteration,
which means that we actually don't need iterator itself and we can use
simpler bitmap API.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-11-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
block/qcow2-bitmap.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 82c9f3..cb06954b4a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1288,7 +1288,6 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
uint8_t *buf = NULL;
-BdrvDirtyBitmapIter *dbi;
uint64_t *tb;
uint64_t tb_size =
size_to_clusters(s,
@@ -1307,12 +1306,14 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
return NULL;
}
-dbi = bdrv_dirty_iter_new(bitmap);
buf = g_malloc(s->cluster_size);
limit = bytes_covered_by_bitmap_cluster(s, bitmap);
assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
-while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
+offset = 0;
+while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, INT64_MAX))
+ >= 0)
+{
uint64_t cluster = offset / limit;
uint64_t end, write_size;
int64_t off;
@@ -1355,23 +1356,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
goto fail;
}
-if (end >= bm_size) {
-break;
-}
-
-bdrv_set_dirty_iter(dbi, end);
+offset = end;
}
*bitmap_table_size = tb_size;
g_free(buf);
-bdrv_dirty_iter_free(dbi);
return tb;
fail:
clear_bitmap_table(bs, tb, tb_size);
g_free(buf);
-bdrv_dirty_iter_free(dbi);
g_free(tb);
return NULL;
--
2.21.1
[PULL v2 04/11] hbitmap: unpublish hbitmap_iter_skip_words
From: Vladimir Sementsov-Ogievskiy
Function is internal and even commented as internal. Drop its
definition from .h file.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-4-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/qemu/hbitmap.h | 7 ---
util/hbitmap.c | 2 +-
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ab227b117f..15837a0e2d 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -297,13 +297,6 @@ void hbitmap_free(HBitmap *hb);
*/
void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
-/* hbitmap_iter_skip_words:
- * @hbi: HBitmapIter to operate on.
- *
- * Internal function used by hbitmap_iter_next and hbitmap_iter_next_word.
- */
-unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
-
/* hbitmap_next_zero:
*
* Find next not dirty bit within selected range. If not found, return -1.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index a368dc5ef7..26145d4b9e 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -104,7 +104,7 @@ struct HBitmap {
/* Advance hbi to the next nonzero word and return it. hbi->pos
* is updated. Returns zero if we reach the end of the bitmap.
*/
-unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
+static unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
{
size_t pos = hbi->pos;
const HBitmap *hb = hbi->hb;
--
2.21.1
[PULL v2 05/11] hbitmap: drop meta bitmaps as they are unused
From: Vladimir Sementsov-Ogievskiy
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-5-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/qemu/hbitmap.h | 21
tests/test-hbitmap.c | 115 -
util/hbitmap.c | 16 --
3 files changed, 152 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 15837a0e2d..df922d8517 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -325,27 +325,6 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t
start, uint64_t count);
bool hbitmap_next_dirty_area(const HBitmap *hb, uint64_t *start,
uint64_t *count);
-/* hbitmap_create_meta:
- * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
- * The caller owns the created bitmap and must call hbitmap_free_meta(hb) to
- * free it.
- *
- * Currently, we only guarantee that if a bit in the hbitmap is changed it
- * will be reflected in the meta bitmap, but we do not yet guarantee the
- * opposite.
- *
- * @hb: The HBitmap to operate on.
- * @chunk_size: How many bits in @hb does one bit in the meta track.
- */
-HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
-
-/* hbitmap_free_meta:
- * Free the meta bitmap of @hb.
- *
- * @hb: The HBitmap whose meta bitmap should be freed.
- */
-void hbitmap_free_meta(HBitmap *hb);
-
/**
* hbitmap_iter_next:
* @hbi: HBitmapIter to operate on.
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index e1f867085f..aeaa0b3f22 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -22,7 +22,6 @@
typedef struct TestHBitmapData {
HBitmap *hb;
-HBitmap *meta;
unsigned long *bits;
size_t size;
size_t old_size;
@@ -94,14 +93,6 @@ static void hbitmap_test_init(TestHBitmapData *data,
}
}
-static void hbitmap_test_init_meta(TestHBitmapData *data,
- uint64_t size, int granularity,
- int meta_chunk)
-{
-hbitmap_test_init(data, size, granularity);
-data->meta = hbitmap_create_meta(data->hb, meta_chunk);
-}
-
static inline size_t hbitmap_test_array_size(size_t bits)
{
size_t n = DIV_ROUND_UP(bits, BITS_PER_LONG);
@@ -144,9 +135,6 @@ static void hbitmap_test_teardown(TestHBitmapData *data,
const void *unused)
{
if (data->hb) {
-if (data->meta) {
-hbitmap_free_meta(data->hb);
-}
hbitmap_free(data->hb);
data->hb = NULL;
}
@@ -648,96 +636,6 @@ static void
test_hbitmap_truncate_shrink_large(TestHBitmapData *data,
hbitmap_test_truncate(data, size, -diff, 0);
}
-static void hbitmap_check_meta(TestHBitmapData *data,
- int64_t start, int count)
-{
-int64_t i;
-
-for (i = 0; i < data->size; i++) {
-if (i >= start && i < start + count) {
-g_assert(hbitmap_get(data->meta, i));
-} else {
-g_assert(!hbitmap_get(data->meta, i));
-}
-}
-}
-
-static void hbitmap_test_meta(TestHBitmapData *data,
- int64_t start, int count,
- int64_t check_start, int check_count)
-{
-hbitmap_reset_all(data->hb);
-hbitmap_reset_all(data->meta);
-
-/* Test "unset" -> "unset" will not update meta. */
-hbitmap_reset(data->hb, start, count);
-hbitmap_check_meta(data, 0, 0);
-
-/* Test "unset" -> "set" will update meta */
-hbitmap_set(data->hb, start, count);
-hbitmap_check_meta(data, check_start, check_count);
-
-/* Test "set" -> "set" will not update meta */
-hbitmap_reset_all(data->meta);
-hbitmap_set(data->hb, start, count);
-hbitmap_check_meta(data, 0, 0);
-
-/* Test "set" -> "unset" will update meta */
-hbitmap_reset_all(data->meta);
-hbitmap_reset(data->hb, start, count);
-hbitmap_check_meta(data, check_start, check_count);
-}
-
-static void hbitmap_test_meta_do(TestHBitmapData *data, int chunk_size)
-{
-uint64_t size = chunk_size * 100;
-hbitmap_test_init_meta(data, size, 0, chunk_size);
-
-hbitmap_test_meta(data, 0, 1, 0, chunk_size);
-hbitmap_test_meta(data, 0, chunk_size, 0, chunk_size);
-hbitmap_test_meta(data, chunk_size - 1, 1, 0, chunk_size);
-hbitmap_test_meta(data, chunk_size - 1, 2, 0, chunk_size * 2);
-hbitmap_test_meta(data, chunk_size - 1, chunk_size + 1, 0, chunk_size * 2);
-hbitmap_test_meta(data, chunk_size - 1, chunk_size + 2, 0, chunk_size * 3);
-hbitmap_test_meta(data, 7 * chunk_size - 1, chunk_size + 2,
- 6 * chunk_size, chunk_size * 3);
-hbitmap_test_meta(data, size - 1, 1, size - chunk_size, chunk_size);
-hbitmap_test_meta(data, 0, size, 0, size);
-}
-
-static void test_hbitmap_meta_byte(TestHBitmapData *data, const void *unused)
-{
-
[PULL v2 01/11] build: Silence clang warning on older glib autoptr usage
From: Eric Blake
glib's G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro defines several static
inline functions, often with some of them unused, but prior to 2.57.2
did not mark the functions as such. As a result, clang (but not gcc)
fails to build with older glib unless -Wno-unused-function is enabled.
Reported-by: Peter Maydell
Signed-off-by: Eric Blake
Reviewed-by: John Snow
Message-id: 20200317175534.196295-1-ebl...@redhat.com
Signed-off-by: John Snow
---
configure | 20
1 file changed, 20 insertions(+)
diff --git a/configure b/configure
index 06fcd070fb..479336bf6e 100755
--- a/configure
+++ b/configure
@@ -3851,6 +3851,26 @@ if ! compile_prog "$glib_cflags -Werror" "$glib_libs" ;
then
fi
fi
+# Silence clang warnings triggered by glib < 2.57.2
+cat > $TMPC << EOF
+#include
+typedef struct Foo {
+int i;
+} Foo;
+static void foo_free(Foo *f)
+{
+g_free(f);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Foo, foo_free);
+int main(void) { return 0; }
+EOF
+if ! compile_prog "$glib_cflags -Werror" "$glib_libs" ; then
+if cc_has_warning_flag "-Wno-unused-function"; then
+glib_cflags="$glib_cflags -Wno-unused-function"
+CFLAGS="$CFLAGS -Wno-unused-function"
+fi
+fi
+
#
# zlib check
--
2.21.1
[PULL v2 08/11] block/dirty-bitmap: improve _next_dirty_area API
From: Vladimir Sementsov-Ogievskiy
Firstly, _next_dirty_area is for scenarios when we may contiguously
search for next dirty area inside some limited region, so it is more
comfortable to specify "end" which should not be recalculated on each
iteration.
Secondly, let's add a possibility to limit resulting area size, not
limiting searching area. This will be used in NBD code in further
commit. (Note that now bdrv_dirty_bitmap_next_dirty_area is unused)
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-8-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/block/dirty-bitmap.h | 3 ++-
include/qemu/hbitmap.h | 23 ++
block/dirty-bitmap.c | 6 +++--
tests/test-hbitmap.c | 45 +++-
util/hbitmap.c | 44 +--
5 files changed, 75 insertions(+), 46 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index b1f0de12db..8a10029418 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -110,7 +110,8 @@ int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap
*bitmap, int64_t offset,
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- int64_t *offset, int64_t *bytes);
+int64_t start, int64_t end, int64_t max_dirty_count,
+int64_t *dirty_start, int64_t *dirty_count);
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6e9ae51ed3..5e71b6d6f7 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -324,18 +324,21 @@ int64_t hbitmap_next_zero(const HBitmap *hb, int64_t
start, int64_t count);
/* hbitmap_next_dirty_area:
* @hb: The HBitmap to operate on
- * @start: in-out parameter.
- * in: the offset to start from
- * out: (if area found) start of found area
- * @count: in-out parameter.
- * in: length of requested region
- * out: length of found area
+ * @start: the offset to start from
+ * @end: end of requested area
+ * @max_dirty_count: limit for out parameter dirty_count
+ * @dirty_start: on success: start of found area
+ * @dirty_count: on success: length of found area
*
- * If dirty area found within [@start, @start + @count), returns true and sets
- * @offset and @bytes appropriately. Otherwise returns false and leaves @offset
- * and @bytes unchanged.
+ * If dirty area found within [@start, @end), returns true and sets
+ * @dirty_start and @dirty_count appropriately. @dirty_count will not exceed
+ * @max_dirty_count.
+ * If dirty area was not found, returns false and leaves @dirty_start and
+ * @dirty_count unchanged.
*/
-bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t
*count);
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
+ int64_t max_dirty_count,
+ int64_t *dirty_start, int64_t *dirty_count);
/**
* hbitmap_iter_next:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 1b14c8eb26..063793e316 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -873,9 +873,11 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap
*bitmap, int64_t offset,
}
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- int64_t *offset, int64_t *bytes)
+int64_t start, int64_t end, int64_t max_dirty_count,
+int64_t *dirty_start, int64_t *dirty_count)
{
-return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
+return hbitmap_next_dirty_area(bitmap->bitmap, start, end, max_dirty_count,
+ dirty_start, dirty_count);
}
/**
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 8905b8a351..b6726cf76b 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -920,18 +920,19 @@ static void
test_hbitmap_next_x_after_truncate(TestHBitmapData *data,
test_hbitmap_next_x_check(data, 0);
}
-static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
- int64_t offset,
- int64_t count)
+static void test_hbitmap_next_dirty_area_check_limited(TestHBitmapData *data,
+ int64_t offset,
+ int64_t count,
+ int64_t max_dirty)
{
int64_t off1, off2;
int64_t len1 = 0, len2;
bool ret1, ret2;
int64_t end;
-off1 = offset;
-len1 = count;
[PULL v2 02/11] hbitmap: assert that we don't create bitmap larger than INT64_MAX
From: Vladimir Sementsov-Ogievskiy We have APIs which returns signed int64_t, to be able to return error. Therefore we can't handle bitmaps with absolute size larger than (INT64_MAX+1). Still, keep maximum to be INT64_MAX which is a bit safer. Note, that bitmaps are used to represent disk images, which can't exceed INT64_MAX anyway. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz Reviewed-by: Eric Blake Reviewed-by: John Snow Message-id: 20200205112041.6003-2-vsement...@virtuozzo.com Signed-off-by: John Snow --- util/hbitmap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/util/hbitmap.c b/util/hbitmap.c index 242c6e519c..7f9b3e0cd7 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -716,6 +716,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity) HBitmap *hb = g_new0(struct HBitmap, 1); unsigned i; +assert(size <= INT64_MAX); hb->orig_size = size; assert(granularity >= 0 && granularity < 64); @@ -746,6 +747,7 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size) uint64_t num_elements = size; uint64_t old; +assert(size <= INT64_MAX); hb->orig_size = size; /* Size comes in as logical elements, adjust for granularity. */ -- 2.21.1
[PULL v2 03/11] hbitmap: move hbitmap_iter_next_word to hbitmap.c
From: Vladimir Sementsov-Ogievskiy
The function is definitely internal (it's not used by third party and
it has complicated interface). Move it to .c file.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-3-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/qemu/hbitmap.h | 30 --
util/hbitmap.c | 29 +
2 files changed, 29 insertions(+), 30 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 1bf944ca3d..ab227b117f 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -362,34 +362,4 @@ void hbitmap_free_meta(HBitmap *hb);
*/
int64_t hbitmap_iter_next(HBitmapIter *hbi);
-/**
- * hbitmap_iter_next_word:
- * @hbi: HBitmapIter to operate on.
- * @p_cur: Location where to store the next non-zero word.
- *
- * Return the index of the next nonzero word that is set in @hbi's
- * associated HBitmap, and set *p_cur to the content of that word
- * (bits before the index that was passed to hbitmap_iter_init are
- * trimmed on the first call). Return -1, and set *p_cur to zero,
- * if all remaining words are zero.
- */
-static inline size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long
*p_cur)
-{
-unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-
-if (cur == 0) {
-cur = hbitmap_iter_skip_words(hbi);
-if (cur == 0) {
-*p_cur = 0;
-return -1;
-}
-}
-
-/* The next call will resume work from the next word. */
-hbi->cur[HBITMAP_LEVELS - 1] = 0;
-*p_cur = cur;
-return hbi->pos;
-}
-
-
#endif
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 7f9b3e0cd7..a368dc5ef7 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -298,6 +298,35 @@ uint64_t hbitmap_count(const HBitmap *hb)
return hb->count << hb->granularity;
}
+/**
+ * hbitmap_iter_next_word:
+ * @hbi: HBitmapIter to operate on.
+ * @p_cur: Location where to store the next non-zero word.
+ *
+ * Return the index of the next nonzero word that is set in @hbi's
+ * associated HBitmap, and set *p_cur to the content of that word
+ * (bits before the index that was passed to hbitmap_iter_init are
+ * trimmed on the first call). Return -1, and set *p_cur to zero,
+ * if all remaining words are zero.
+ */
+static size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long *p_cur)
+{
+unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
+
+if (cur == 0) {
+cur = hbitmap_iter_skip_words(hbi);
+if (cur == 0) {
+*p_cur = 0;
+return -1;
+}
+}
+
+/* The next call will resume work from the next word. */
+hbi->cur[HBITMAP_LEVELS - 1] = 0;
+*p_cur = cur;
+return hbi->pos;
+}
+
/* Count the number of set bits between start and end, not accounting for
* the granularity. Also an example of how to use hbitmap_iter_next_word.
*/
--
2.21.1
[PULL v2 09/11] nbd/server: introduce NBDExtentArray
From: Vladimir Sementsov-Ogievskiy
Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with fewer OUT parameters in functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Eric Blake
Message-id: 20200205112041.6003-9-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
nbd/server.c | 210 +--
1 file changed, 118 insertions(+), 92 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 3106aaf3b4..f90bb33a75 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1909,27 +1909,98 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
+typedef struct NBDExtentArray {
+NBDExtent *extents;
+unsigned int nb_alloc;
+unsigned int count;
+uint64_t total_length;
+bool can_add;
+bool converted_to_be;
+} NBDExtentArray;
+
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+{
+NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+
+ea->nb_alloc = nb_alloc;
+ea->extents = g_new(NBDExtent, nb_alloc);
+ea->can_add = true;
+
+return ea;
+}
+
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+g_free(ea->extents);
+g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
+
+/* Further modifications of the array after conversion are abandoned */
+static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
+{
+int i;
+
+assert(!ea->converted_to_be);
+ea->can_add = false;
+ea->converted_to_be = true;
+
+for (i = 0; i < ea->count; i++) {
+ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
+ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
+}
+}
+
/*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
+ * Add extent to NBDExtentArray. If extent can't be added (no available space),
+ * return -1.
+ * For safety, when returning -1 for the first time, .can_add is set to false,
+ * further call to nbd_extent_array_add() will crash.
+ * (to avoid the situation, when after failing to add an extent (returned -1),
+ * user miss this failure and add another extent, which is successfully added
+ * (array is full, but new extent may be squashed into the last one), then we
+ * have invalid array with skipped extent)
*/
+static int nbd_extent_array_add(NBDExtentArray *ea,
+uint32_t length, uint32_t flags)
+{
+assert(ea->can_add);
+
+if (!length) {
+return 0;
+}
+
+/* Extend previous extent if flags are the same */
+if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
+
+if (sum <= UINT32_MAX) {
+ea->extents[ea->count - 1].length = sum;
+ea->total_length += length;
+return 0;
+}
+}
+
+if (ea->count >= ea->nb_alloc) {
+ea->can_add = false;
+return -1;
+}
+
+ea->total_length += length;
+ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+ea->count++;
+
+return 0;
+}
+
static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t *bytes, NBDExtent *extents,
- unsigned int *nb_extents)
+ uint64_t bytes, NBDExtentArray *ea)
{
-uint64_t remaining_bytes = *bytes;
-NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
-bool first_extent = true;
-
-assert(*nb_extents);
-while (remaining_bytes) {
+while (bytes) {
uint32_t flags;
int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
- , NULL, NULL);
+int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -1938,60 +2009,37 @@ static int blockstatus_to_extents(BlockDriverState *bs,
uint64_t offset,
flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
-if (first_extent) {
-extent->flags = flags;
-extent->length = num;
-first_extent = false;
-} else if (flags == extent->flags) {
-/* extend current extent */
-extent->length += num;
-} else {
-if (extent + 1 == extents_end) {
-break;
-}
-
-/* start new extent */
-extent++;
-extent->flags = flags;
-extent->length = num;
+if
[PULL v2 07/11] block/dirty-bitmap: add _next_dirty API
From: Vladimir Sementsov-Ogievskiy
We have bdrv_dirty_bitmap_next_zero, let's add corresponding
bdrv_dirty_bitmap_next_dirty, which is more comfortable to use than
bitmap iterators in some cases.
For test modify test_hbitmap_next_zero_check_range to check both
next_zero and next_dirty and add some new checks.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-7-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/block/dirty-bitmap.h | 2 +
include/qemu/hbitmap.h | 13
block/dirty-bitmap.c | 6 ++
tests/test-hbitmap.c | 130 ---
util/hbitmap.c | 60
5 files changed, 126 insertions(+), 85 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 27c72cc56a..b1f0de12db 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -105,6 +105,8 @@ for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
bitmap = bdrv_dirty_bitmap_next(bitmap))
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes);
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index b6e85f3d5d..6e9ae51ed3 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -297,6 +297,19 @@ void hbitmap_free(HBitmap *hb);
*/
void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
+/*
+ * hbitmap_next_dirty:
+ *
+ * Find next dirty bit within selected range. If not found, return -1.
+ *
+ * @hb: The HBitmap to operate on
+ * @start: The bit to start from.
+ * @count: Number of bits to proceed. If @start+@count > bitmap size, the whole
+ * bitmap is looked through. You can use INT64_MAX as @count to search up to
+ * the bitmap end.
+ */
+int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count);
+
/* hbitmap_next_zero:
*
* Find next not dirty bit within selected range. If not found, return -1.
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index af9f5411a6..1b14c8eb26 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -860,6 +860,12 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap
*bitmap, Error **errp)
return hbitmap_sha256(bitmap->bitmap, errp);
}
+int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes)
+{
+return hbitmap_next_dirty(bitmap->bitmap, offset, bytes);
+}
+
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes)
{
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 9d210dc18c..8905b8a351 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -816,92 +816,108 @@ static void test_hbitmap_iter_and_reset(TestHBitmapData
*data,
hbitmap_iter_next();
}
-static void test_hbitmap_next_zero_check_range(TestHBitmapData *data,
- int64_t start,
- int64_t count)
+static void test_hbitmap_next_x_check_range(TestHBitmapData *data,
+int64_t start,
+int64_t count)
{
-int64_t ret1 = hbitmap_next_zero(data->hb, start, count);
-int64_t ret2 = start;
+int64_t next_zero = hbitmap_next_zero(data->hb, start, count);
+int64_t next_dirty = hbitmap_next_dirty(data->hb, start, count);
+int64_t next;
int64_t end = start >= data->size || data->size - start < count ?
data->size : start + count;
+bool first_bit = hbitmap_get(data->hb, start);
-for ( ; ret2 < end && hbitmap_get(data->hb, ret2); ret2++) {
+for (next = start;
+ next < end && hbitmap_get(data->hb, next) == first_bit;
+ next++)
+{
;
}
-if (ret2 == end) {
-ret2 = -1;
+
+if (next == end) {
+next = -1;
}
-g_assert_cmpint(ret1, ==, ret2);
+g_assert_cmpint(next_dirty, ==, first_bit ? start : next);
+g_assert_cmpint(next_zero, ==, first_bit ? next : start);
}
-static void test_hbitmap_next_zero_check(TestHBitmapData *data, int64_t start)
+static void test_hbitmap_next_x_check(TestHBitmapData *data, int64_t start)
{
-test_hbitmap_next_zero_check_range(data, start, INT64_MAX);
+test_hbitmap_next_x_check_range(data, start, INT64_MAX);
}
-static void test_hbitmap_next_zero_do(TestHBitmapData *data, int granularity)
+static void test_hbitmap_next_x_do(TestHBitmapData *data, int granularity)
{
hbitmap_test_init(data, L3,
[PULL v2 00/11] Bitmaps patches
The following changes since commit d649689a8ecb2e276cc20d3af6d416e3c299cb17: Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-03-17 18:33:05 +) are available in the Git repository at: https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request for you to fetch changes up to 2d00cbd8e222a4adc08f415c399e84590ee8ff9a: block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-18 14:03:46 -0400) Pull request Eric Blake (1): build: Silence clang warning on older glib autoptr usage Vladimir Sementsov-Ogievskiy (10): hbitmap: assert that we don't create bitmap larger than INT64_MAX hbitmap: move hbitmap_iter_next_word to hbitmap.c hbitmap: unpublish hbitmap_iter_skip_words hbitmap: drop meta bitmaps as they are unused block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t block/dirty-bitmap: add _next_dirty API block/dirty-bitmap: improve _next_dirty_area API nbd/server: introduce NBDExtentArray nbd/server: use bdrv_dirty_bitmap_next_dirty_area block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty configure| 20 +++ include/block/dirty-bitmap.h | 9 +- include/qemu/hbitmap.h | 95 +++ block/dirty-bitmap.c | 16 +- block/qcow2-bitmap.c | 15 +- nbd/server.c | 251 ++-- tests/test-hbitmap.c | 316 +-- util/hbitmap.c | 134 +-- 8 files changed, 395 insertions(+), 461 deletions(-) -- 2.21.1
[PULL v2 06/11] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t
From: Vladimir Sementsov-Ogievskiy
We are going to introduce bdrv_dirty_bitmap_next_dirty so that same
variable may be used to store its return value and to be its parameter,
so it would int64_t.
Similarly, we are going to refactor hbitmap_next_dirty_area to use
hbitmap_next_dirty together with hbitmap_next_zero, therefore we want
hbitmap_next_zero parameter type to be int64_t too.
So, for convenience update all parameters of *_next_zero and
*_next_dirty_area to be int64_t.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20200205112041.6003-6-vsement...@virtuozzo.com
Signed-off-by: John Snow
---
include/block/dirty-bitmap.h | 6 +++---
include/qemu/hbitmap.h | 7 +++
block/dirty-bitmap.c | 6 +++---
nbd/server.c | 2 +-
tests/test-hbitmap.c | 36 ++--
util/hbitmap.c | 13 -
6 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index e2b20ecab9..27c72cc56a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -105,10 +105,10 @@ for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
bitmap = bdrv_dirty_bitmap_next(bitmap))
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
-int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
-uint64_t bytes);
+int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
+int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- uint64_t *offset, uint64_t *bytes);
+ int64_t *offset, int64_t *bytes);
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index df922d8517..b6e85f3d5d 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -304,10 +304,10 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap
*hb, uint64_t first);
* @hb: The HBitmap to operate on
* @start: The bit to start from.
* @count: Number of bits to proceed. If @start+@count > bitmap size, the whole
- * bitmap is looked through. You can use UINT64_MAX as @count to search up to
+ * bitmap is looked through. You can use INT64_MAX as @count to search up to
* the bitmap end.
*/
-int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count);
+int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count);
/* hbitmap_next_dirty_area:
* @hb: The HBitmap to operate on
@@ -322,8 +322,7 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t
start, uint64_t count);
* @offset and @bytes appropriately. Otherwise returns false and leaves @offset
* and @bytes unchanged.
*/
-bool hbitmap_next_dirty_area(const HBitmap *hb, uint64_t *start,
- uint64_t *count);
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t
*count);
/**
* hbitmap_iter_next:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7039e82520..af9f5411a6 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -860,14 +860,14 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap
*bitmap, Error **errp)
return hbitmap_sha256(bitmap->bitmap, errp);
}
-int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
-uint64_t bytes)
+int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
+int64_t bytes)
{
return hbitmap_next_zero(bitmap->bitmap, offset, bytes);
}
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- uint64_t *offset, uint64_t *bytes)
+ int64_t *offset, int64_t *bytes)
{
return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
}
diff --git a/nbd/server.c b/nbd/server.c
index 11a31094ff..3106aaf3b4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2055,7 +2055,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap
*bitmap, uint64_t offset,
bool next_dirty = !dirty;
if (dirty) {
-end = bdrv_dirty_bitmap_next_zero(bitmap, begin, UINT64_MAX);
+end = bdrv_dirty_bitmap_next_zero(bitmap, begin, INT64_MAX);
} else {
bdrv_set_dirty_iter(it, begin);
end = bdrv_dirty_iter_next(it);
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index aeaa0b3f22..9d210dc18c 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -817,8 +817,8 @@ static void test_hbitmap_iter_and_reset(TestHBitmapData
*data,
}
static void
[PATCH 0/6] Couple of qemu NS improvements
The most important patch is 6/6 which relies on 5/6. The rest is just a few small nits I've noticed and had stuck on my local branch. Michal Prívozník (6): qemuDomainCreateDeviceRecursive: Report error if mkdir() fails qemuDomainBuildNamespace: Try harder to remove temp directories qemuDomainBuildNamespace: Make @devPath const virfile: Handle directories in virFileBindMountDevice() virprocess: Passthru error from virProcessRunInForkHelper security: Try harder to run transactions build-aux/syntax-check.mk | 2 +- src/qemu/qemu_domain.c | 13 +++-- src/security/security_dac.c | 16 --- src/security/security_selinux.c | 16 --- src/util/virfile.c | 13 +++-- src/util/virprocess.c | 51 ++--- tests/commandtest.c | 43 +++ 7 files changed, 136 insertions(+), 18 deletions(-) -- 2.24.1
[PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper
When running a function in a forked child, so far the only thing
we could report is exit status of the child and the error
message. However, it may be beneficial to the caller to know the
actual error that happened in the child.
Signed-off-by: Michal Privoznik
---
build-aux/syntax-check.mk | 2 +-
src/util/virprocess.c | 51 ---
tests/commandtest.c | 43 +
3 files changed, 91 insertions(+), 5 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 3020921be8..2a38c03ba9 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1987,7 +1987,7 @@ exclude_file_name_regexp--sc_flags_usage = \
exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \
^(src/rpc/gendispatch\.pl$$|tests/)
-exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$)
+exclude_file_name_regexp--sc_po_check =
^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$)
exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \
^(build-aux/syntax-check\.mk|include/libvirt/virterror\.h|src/remote/remote_daemon_dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 24135070b7..e8674402f9 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1126,6 +1126,23 @@ virProcessRunInMountNamespace(pid_t pid G_GNUC_UNUSED,
#ifndef WIN32
+typedef struct {
+int code;
+int domain;
+char message[VIR_ERROR_MAX_LENGTH];
+virErrorLevel level;
+char str1[VIR_ERROR_MAX_LENGTH];
+char str2[VIR_ERROR_MAX_LENGTH];
+/* str3 doesn't seem to be used. Ignore it. */
+int int1;
+int int2;
+} errorData;
+
+typedef union {
+errorData data;
+char bindata[sizeof(errorData)];
+} errorDataBin;
+
static int
virProcessRunInForkHelper(int errfd,
pid_t ppid,
@@ -1134,9 +1151,19 @@ virProcessRunInForkHelper(int errfd,
{
if (cb(ppid, opaque) < 0) {
virErrorPtr err = virGetLastError();
+errorDataBin bin = { 0 };
+
if (err) {
-size_t len = strlen(err->message) + 1;
-ignore_value(safewrite(errfd, err->message, len));
+bin.data.code = err->code;
+bin.data.domain = err->domain;
+ignore_value(virStrcpy(bin.data.message, err->message,
sizeof(bin.data.message)));
+bin.data.level = err->level;
+ignore_value(virStrcpy(bin.data.str1, err->str1,
sizeof(bin.data.str1)));
+ignore_value(virStrcpy(bin.data.str2, err->str2,
sizeof(bin.data.str2)));
+bin.data.int1 = err->int1;
+bin.data.int2 = err->int2;
+
+ignore_value(safewrite(errfd, bin.bindata, sizeof(bin)));
}
return -1;
@@ -1188,16 +1215,32 @@ virProcessRunInFork(virProcessForkCallback cb,
} else {
int status;
g_autofree char *buf = NULL;
+errorDataBin bin;
+int nread;
VIR_FORCE_CLOSE(errfd[1]);
-ignore_value(virFileReadHeaderFD(errfd[0], 1024, ));
+nread = virFileReadHeaderFD(errfd[0], sizeof(bin), );
ret = virProcessWait(child, , false);
if (ret == 0) {
ret = status == EXIT_CANCELED ? -1 : status;
if (ret) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("child reported (status=%d): %s"),
- status, NULLSTR(buf));
+ status, NULLSTR(bin.data.message));
+
+if (nread == sizeof(bin)) {
+memcpy(bin.bindata, buf, sizeof(bin));
+virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__,
+ bin.data.domain,
+ bin.data.code,
+ bin.data.level,
+ bin.data.str1,
+ bin.data.str2,
+ NULL,
+ bin.data.int1,
+ bin.data.int2,
+ "%s", bin.data.message);
+}
}
}
}
diff --git a/tests/commandtest.c b/tests/commandtest.c
index a64aa9ad33..f4a2c67c05 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED)
}
+static int
+test28Callback(pid_t pid G_GNUC_UNUSED,
+ void *opaque G_GNUC_UNUSED)
+{
+virReportSystemError(ENODATA, "%s", "some error message");
+return -1;
+}
+
+
+static int
+test28(const void *unused G_GNUC_UNUSED)
+{
+/* Not strictly a virCommand test, but this is the easiest place
+ * to test this lower-level interface. */
+virErrorPtr err;
+
+if (virProcessRunInFork(test28Callback, NULL) != -1) {
[PATCH 1/6] qemuDomainCreateDeviceRecursive: Report error if mkdir() fails
The virFileMakePathWithMode() which is our recursive version of
mkdir() fails, it simply just returns a negative value with errno
set. No error is reported (as compared to virFileTouch() for
instance).
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0e2252f6cf..48bf5ae559 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -14643,8 +14643,12 @@ qemuDomainCreateDeviceRecursive(const char *device,
* proper owner and mode. Bind mount only after that. */
} else if (isDir) {
if (create &&
-virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
+virFileMakePathWithMode(devicePath, sb.st_mode) < 0) {
+virReportSystemError(errno,
+ _("Unable to make dir %s"),
+ devicePath);
goto cleanup;
+}
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s 0%o"),
--
2.24.1
[PATCH 3/6] qemuDomainBuildNamespace: Make @devPath const
The @devPath variable is not modifiable. It merely just points to
string containing path where private devtmpfs is being
constructed. Make it const so it doesn't look weird that it's not
freed.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2724607311..a4c07fffcb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15176,7 +15176,7 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
virDomainObjPtr vm)
{
struct qemuDomainCreateDeviceData data;
-char *devPath = NULL;
+const char *devPath = NULL;
char **devMountsPath = NULL, **devMountsSavePath = NULL;
size_t ndevMountsPath = 0, i;
int ret = -1;
--
2.24.1
[PATCH 6/6] security: Try harder to run transactions
When a QEMU process dies in the middle of a hotplug, then we fail
to restore the seclabels on the device. The problem is that if
the thread doing hotplug locks the domain object first and thus
blocks the thread that wants to do qemuProcessStop(), the
seclabel cleanup code will see vm->pid still set and mount
namespace used and therefore try to enter the namespace
represented by the PID. But the PID is gone really and thus
entering will fail and no restore is done. What we can do is to
try enter the namespace (if requested to do so) but if entering
fails, fall back to no NS mode.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814481
Signed-off-by: Michal Privoznik
---
src/security/security_dac.c | 16
src/security/security_selinux.c | 16
2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9046b51004..11fff63bc7 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -640,15 +640,23 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
list->lock = lock;
+if (pid != -1) {
+rc = virProcessRunInMountNamespace(pid,
+ virSecurityDACTransactionRun,
+ list);
+if (rc < 0) {
+if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
+pid = -1;
+else
+goto cleanup;
+}
+}
+
if (pid == -1) {
if (lock)
rc = virProcessRunInFork(virSecurityDACTransactionRun, list);
else
rc = virSecurityDACTransactionRun(pid, list);
-} else {
-rc = virProcessRunInMountNamespace(pid,
- virSecurityDACTransactionRun,
- list);
}
if (rc < 0)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c94f31727c..8aeb6e45a5 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1163,15 +1163,23 @@
virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,
list->lock = lock;
+if (pid != -1) {
+rc = virProcessRunInMountNamespace(pid,
+ virSecuritySELinuxTransactionRun,
+ list);
+if (rc < 0) {
+if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
+pid = -1;
+else
+goto cleanup;
+}
+}
+
if (pid == -1) {
if (lock)
rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list);
else
rc = virSecuritySELinuxTransactionRun(pid, list);
-} else {
-rc = virProcessRunInMountNamespace(pid,
- virSecuritySELinuxTransactionRun,
- list);
}
if (rc < 0)
--
2.24.1
[PATCH 2/6] qemuDomainBuildNamespace: Try harder to remove temp directories
If building namespace fails somewhere in the middle (that is some
files exists under devMountsSavePath[i]), then plain rmdir() is
not enough to remove dir. Umount the temp location and use
virFileDeleteTree() to remove the directory.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 48bf5ae559..2724607311 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15311,9 +15311,12 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
ret = 0;
cleanup:
for (i = 0; i < ndevMountsPath; i++) {
+#if defined(__linux__)
+umount(devMountsSavePath[i]);
+#endif /* defined(__linux__) */
/* The path can be either a regular file or a dir. */
if (virFileIsDir(devMountsSavePath[i]))
-rmdir(devMountsSavePath[i]);
+virFileDeleteTree(devMountsSavePath[i]);
else
unlink(devMountsSavePath[i]);
}
--
2.24.1
[PATCH 4/6] virfile: Handle directories in virFileBindMountDevice()
The @src is not always a file. It may also be a directory (for
instance qemuDomainCreateDeviceRecursive() assumes that) - even
though it doesn't happen usually. Anyway, mount() can mount only
a dir onto a dir and a file onto a file.
Signed-off-by: Michal Privoznik
---
src/util/virfile.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 0f31554323..9c175b041f 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3743,8 +3743,17 @@ int
virFileBindMountDevice(const char *src,
const char *dst)
{
-if (virFileTouch(dst, 0666) < 0)
-return -1;
+if (!virFileExists(dst)) {
+if (virFileIsDir(src)) {
+if (virFileMakePath(dst)) {
+virReportSystemError(errno, _("Unable to make dir %s"), dst);
+return -1;
+}
+} else {
+if (virFileTouch(dst, 0666) < 0)
+return -1;
+}
+}
if (mount(src, dst, "none", MS_BIND, NULL) < 0) {
virReportSystemError(errno, _("Failed to bind %s on to %s"), src,
--
2.24.1
Re: qemu:///embed and isolation from global components
On 18. 3. 2020 16:47, Andrea Bolognani wrote: > On Mon, 2020-03-09 at 18:04 +, Daniel P. Berrangé wrote: >> At a high level the embedded QEMU driver >> >> - Isolated from any other instance of the QEMU driver > > Replying here because it looks as good a place as any. > > Now that Michal has made it so that identically-name domains defined > under different qemu:///embed roots no longer clash at the machined > level (thanks!), I have immediately bumped into the next issue: if > I use either one of > > > > > > > > > > both qemu:///embed instances try to use the same paths: > > /dev/hugepages/libvirt/qemu/$domid-$domname/ > > /dev/shm/libvirt/qemu/$domid-$domname/ This is because qemu driver uses virDomainDefGetShortName() to construct the path which is not embed driver aware. In fact, we use it on a few other places: libvirt.git $ git grep -C0 virDomainDefGetShortName -- src/qemu/ \ | wc -l 15 so we probably have more broken plaes. I will investigate and post a patch. Probably something among those lines that fixed machined name generation. > > which obviously breaks everything. > > In this case, is the application expected to set > > hugetlbfs_mount = "/dev/hugepages/$random" > > memory_backing_dir = "/dev/shm/$random" > > in qemu.conf beforehand (I'm not sure the former setting would even > work), or should libvirt take care of it automatically in a similar > way it now does for machine names? > The latter is true. Michal
Re: qemu:///embed and isolation from global components
On Mon, 2020-03-09 at 18:04 +, Daniel P. Berrangé wrote: > At a high level the embedded QEMU driver > > - Isolated from any other instance of the QEMU driver Replying here because it looks as good a place as any. Now that Michal has made it so that identically-name domains defined under different qemu:///embed roots no longer clash at the machined level (thanks!), I have immediately bumped into the next issue: if I use either one of both qemu:///embed instances try to use the same paths: /dev/hugepages/libvirt/qemu/$domid-$domname/ /dev/shm/libvirt/qemu/$domid-$domname/ which obviously breaks everything. In this case, is the application expected to set hugetlbfs_mount = "/dev/hugepages/$random" memory_backing_dir = "/dev/shm/$random" in qemu.conf beforehand (I'm not sure the former setting would even work), or should libvirt take care of it automatically in a similar way it now does for machine names? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] conf: Don't generate clashing machine names for embed driver
On Fri, 2020-03-13 at 17:59 +0100, Michal Privoznik wrote:
[...]
> -if (privileged) {
> +if (root) {
> +g_autofree char * hash = NULL;
> +
> +/* When two embed drivers start two domains with the same @name and
> @id
> + * we would generate a non-unique name. Include parts of hashed @root
> + * which guarantees uniqueness. The first 8 characters of SHA256
> ought
> + * to be enough for anybody. */
> +if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, root, ) < 0)
> +return NULL;
> +
> +virBufferAsprintf(, "%s-embed-%.8s-", drivername, hash);
When libvirt is non-privileged we use $username-$drivername, so there
would be a precedent for something like $hash-$drivername-embed; that
said, having $drivername first makes more sense to me, so if anything
I suggest we change the existing one to $drivername-$username.
> +} else if (privileged) {
> virBufferAsprintf(, "%s-", drivername);
> } else {
> +g_autofree char *username = NULL;
> if (!(username = virGetUserName(geteuid( {
> virBufferFreeAndReset();
> return NULL;
> }
> virBufferAsprintf(, "%s-%s-", username, drivername);
> -VIR_FREE(username);
This hunk is unrelated, so please drop it. It can be a standalone
trivial patch instead.
With that done,
Reviewed-by: Andrea Bolognani
Thank you so much for taking care of this!
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 1/2] qemu_conf: Track embed root dir
On Fri, 2020-03-13 at 17:59 +0100, Michal Privoznik wrote: > When initializing virQEMUDriverConfig structure we are given the > root directory for possible embed connection. Save it for future > use. While we could get it later from @uri member, it's not as > easy as dereferencing a pointer (virURIParse() + > virURIGetParam() + error reporting). > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_conf.c | 2 ++ > src/qemu/qemu_conf.h | 2 ++ > 2 files changed, 4 insertions(+) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] vmx: make 'fileName' optional for CD-ROMs
On Wed, Mar 18, 2020 at 01:44:18PM +0100, Pino Toscano wrote:
> (Apparently forgot to send it yesterday, so sending it with a small
> addedum.)
>
> On Tuesday, 17 March 2020 18:09:04 CET Richard W.M. Jones wrote:
> > My only worry about this patch is that it relies on fileName now
> > possibly being NULL, which means if there is any case that you've
> > missed now — or one added in future — which doesn't consider that
> > fileName might be NULL then it'll crash (libvirtd? or virsh? I'm not
> > sure).
>
> In case now (even in v2) fileName is used without checking, it will
> crash libvirt, as the esx/vmware drivers are built-in in the library.
>
> > I wonder if therefore it would be safer to set the string to a
> > known-good non-NULL value such as ‘strdup ("emptyBackingString")’?
>
> Thought about that, however "emptyBackingString" seems like a magic
> identifier, and it would be trading one hack with another, somehow.
Sure, I've no objections if you're happy with it, so you can
add my Reviewed-by tag to v2.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
Re: [PATCH] qemuDomainVcpuValidateConfig: Properly initialize 'firstcpu' variable
On 3/17/20 2:50 PM, Peter Krempa wrote: The loop which checks whether the vcpus are in proper configuration for the requested hot(un)plug skips the first modified vcpu. This means that 'firstvcpu' which is used to print the error message in case the configuration is not suitable would never point to the first modified vcpu. In cases such as: 8 # virsh setvcpu --config --disable upstream 1 error: invalid argument: vcpu '-1' can't be modified as it is followed by non-hotpluggable online vcpus After this fix the proper vcpu is reported in the error message: # virsh setvcpu --config --disable upstream 1 error: invalid argument: vcpu '1' can't be modified as it is followed by non-hotpluggable online vcpu https://bugzilla.redhat.com/show_bug.cgi?id=1611061 Signed-off-by: Peter Krempa --- Reviewed-by: Daniel Henrique Barboza
Re: [PULL 00/10] Bitmaps patches
On 3/17/20 9:00 AM, Peter Maydell wrote: block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400) Pull request --- Hi; this fails to compile with clang: As pointed out here, my recommendation is for John to send a v2 pull request with one more patch added: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg05969.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH] vmx: make 'fileName' optional for CD-ROMs
(Apparently forgot to send it yesterday, so sending it with a small
addedum.)
On Tuesday, 17 March 2020 18:09:04 CET Richard W.M. Jones wrote:
> My only worry about this patch is that it relies on fileName now
> possibly being NULL, which means if there is any case that you've
> missed now — or one added in future — which doesn't consider that
> fileName might be NULL then it'll crash (libvirtd? or virsh? I'm not
> sure).
In case now (even in v2) fileName is used without checking, it will
crash libvirt, as the esx/vmware drivers are built-in in the library.
> I wonder if therefore it would be safer to set the string to a
> known-good non-NULL value such as ‘strdup ("emptyBackingString")’?
Thought about that, however "emptyBackingString" seems like a magic
identifier, and it would be trading one hack with another, somehow.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 0/2] vmx: make 'fileName' optional for CD-ROMs
Works, so: Tested-by: Richard W.M. Jones I have no further comments about the patches themselves other than what I said on the first version. 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
[PATCH 2/5] qemuMonitorJSON(Add|Del)Object: Refactor cleanup
Use 'g_autoptr' and remove the cleanup label and ret variable.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 42 +---
1 file changed, 15 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 8ac8291d0a..00d7760a05 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4418,51 +4418,39 @@ int
qemuMonitorJSONAddObject(qemuMonitorPtr mon,
virJSONValuePtr props)
{
-int ret = -1;
-virJSONValuePtr cmd;
-virJSONValuePtr reply = NULL;
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props)))
-goto cleanup;
+return -1;
if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
-goto cleanup;
+return -1;
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
-goto cleanup;
+return -1;
-ret = 0;
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-return ret;
+return 0;
}
-int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
- const char *objalias)
+int
+qemuMonitorJSONDelObject(qemuMonitorPtr mon,
+ const char *objalias)
{
-int ret = -1;
-virJSONValuePtr cmd;
-virJSONValuePtr reply = NULL;
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
-cmd = qemuMonitorJSONMakeCommand("object-del",
- "s:id", objalias,
- NULL);
-if (!cmd)
+if (!(cmd = qemuMonitorJSONMakeCommand("object-del", "s:id", objalias,
NULL)))
return -1;
if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
-goto cleanup;
+return -1;
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
-goto cleanup;
+return -1;
-ret = 0;
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-return ret;
+return 0;
}
--
2.24.1
[PATCH 4/5] qemuMonitorJSONCheckError: Allow suppressing of error reporting
In some cases we'll need to check whether there was an error but avoid
reporting an actual libvirt error. Rename qemuMonitorJSONCheckError to
qemuMonitorJSONCheckErrorFull with a new flag to suppress the error
reporting and add a wrapper with the original name so that callers don't
need to be fixed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 31eb01006c..c18cef5c1a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -388,8 +388,9 @@ qemuMonitorJSONCommandName(virJSONValuePtr cmd)
}
static int
-qemuMonitorJSONCheckError(virJSONValuePtr cmd,
- virJSONValuePtr reply)
+qemuMonitorJSONCheckErrorFull(virJSONValuePtr cmd,
+ virJSONValuePtr reply,
+ bool report)
{
if (virJSONValueObjectHasKey(reply, "error")) {
virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
@@ -400,6 +401,9 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd,
VIR_DEBUG("unable to execute QEMU command %s: %s",
NULLSTR(cmdstr), NULLSTR(replystr));
+if (!report)
+return -1;
+
/* Only send the user the command name + friendly error */
if (!error)
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -418,6 +422,10 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd,
VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s:
%s",
NULLSTR(cmdstr), NULLSTR(replystr));
+
+if (!report)
+return -1;
+
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to execute QEMU command '%s'"),
qemuMonitorJSONCommandName(cmd));
@@ -427,6 +435,14 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd,
}
+static int
+qemuMonitorJSONCheckError(virJSONValuePtr cmd,
+ virJSONValuePtr reply)
+{
+return qemuMonitorJSONCheckErrorFull(cmd, reply, true);
+}
+
+
static int
qemuMonitorJSONCheckReply(virJSONValuePtr cmd,
virJSONValuePtr reply,
--
2.24.1
[PATCH 1/5] qemuDomainChangeEjectableMedia: Don't always remove managed PR daemon
When changing media we'd attempt to remove the managed pr daemon even if neither of the images involved in the media change used it. This caused libvirtd to log a spurious error: 2020-03-18 01:41:19.832+: 643207: error : qemuMonitorJSONCheckError:412 : internal error: unable to execute QEMU command 'object-del': object 'pr-helper0' not found With this patch we completely avoid calling the deletion code. https://bugzilla.redhat.com/show_bug.cgi?id=1814486 Signed-off-by: Peter Krempa --- src/qemu/qemu_hotplug.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 47069be900..1cab12fb2b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -590,6 +590,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourcePtr oldsrc = disk->src; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); bool sharedAdded = false; +bool managedpr = virStorageSourceChainHasManagedPR(oldsrc) || + virStorageSourceChainHasManagedPR(newsrc); int ret = -1; int rc; @@ -653,7 +655,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } /* remove PR manager object if unneeded */ -ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); +if (managedpr) +ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); /* revert old image do the disk definition */ if (oldsrc) -- 2.24.1
[PATCH 3/5] qemuMonitorJSONCheckError: Use g_autofree
Eliminate cleanup code by using g_autofree.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 00d7760a05..31eb01006c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -393,8 +393,8 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd,
{
if (virJSONValueObjectHasKey(reply, "error")) {
virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
-char *cmdstr = virJSONValueToString(cmd, false);
-char *replystr = virJSONValueToString(reply, false);
+g_autofree char *cmdstr = virJSONValueToString(cmd, false);
+g_autofree char *replystr = virJSONValueToString(reply, false);
/* Log the full JSON formatted command & error */
VIR_DEBUG("unable to execute QEMU command %s: %s",
@@ -411,20 +411,16 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd,
qemuMonitorJSONCommandName(cmd),
qemuMonitorJSONStringifyError(error));
-VIR_FREE(cmdstr);
-VIR_FREE(replystr);
return -1;
} else if (!virJSONValueObjectHasKey(reply, "return")) {
-char *cmdstr = virJSONValueToString(cmd, false);
-char *replystr = virJSONValueToString(reply, false);
+g_autofree char *cmdstr = virJSONValueToString(cmd, false);
+g_autofree char *replystr = virJSONValueToString(reply, false);
VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s:
%s",
NULLSTR(cmdstr), NULLSTR(replystr));
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to execute QEMU command '%s'"),
qemuMonitorJSONCommandName(cmd));
-VIR_FREE(cmdstr);
-VIR_FREE(replystr);
return -1;
}
return 0;
--
2.24.1
[PATCH 5/5] qemu: Suppress error reporting from qemuMonitorDelObject in cleanup paths
Many calls of qemuMonitorDelObject don't actually check the return value or report the error from the object deletion itself since they are on cleanup paths. In some cases this can lead to reporting of spurious errors e.g. when qemuMonitorDelObject is used to clean up a possibly pre-existing objects. Add a new argument for qemuMonitorDelObject which controls whether the internals report errors from qemu and fix all callers accordingly. Note that some of the cases on device unplug which check the error code don't in fact propagate the error to the user, but in this case it is important to add the log entry anyways for tracing that the device deletion failed. https://bugzilla.redhat.com/show_bug.cgi?id=1784040 Signed-off-by: Peter Krempa --- src/qemu/qemu_block.c| 10 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 29 +++-- src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 3 ++- 7 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index f95ebb6fa7..4ed17b6df3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1732,19 +1732,19 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon, ignore_value(qemuMonitorBlockdevDel(mon, data->storageNodeName)); if (data->prmgrAlias) -ignore_value(qemuMonitorDelObject(mon, data->prmgrAlias)); +ignore_value(qemuMonitorDelObject(mon, data->prmgrAlias, false)); if (data->authsecretAlias) -ignore_value(qemuMonitorDelObject(mon, data->authsecretAlias)); +ignore_value(qemuMonitorDelObject(mon, data->authsecretAlias, false)); if (data->encryptsecretAlias) -ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias)); +ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias, false)); if (data->httpcookiesecretAlias) -ignore_value(qemuMonitorDelObject(mon, data->httpcookiesecretAlias)); +ignore_value(qemuMonitorDelObject(mon, data->httpcookiesecretAlias, false)); if (data->tlsAlias) -ignore_value(qemuMonitorDelObject(mon, data->tlsAlias)); +ignore_value(qemuMonitorDelObject(mon, data->tlsAlias, false)); virErrorRestore(_err); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b62947fb4a..8a2cd35fa7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6047,7 +6047,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); -rc = qemuMonitorDelObject(priv->mon, alias); +rc = qemuMonitorDelObject(priv->mon, alias, true); exp_niothreads--; if (rc < 0) goto exit_monitor; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1cab12fb2b..bbe721c02c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -390,7 +390,8 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; -ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); +ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias(), + false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -471,7 +472,7 @@ qemuDomainDetachDBusVMState(virQEMUDriverPtr driver, qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; -ret = qemuMonitorDelObject(priv->mon, alias); +ret = qemuMonitorDelObject(priv->mon, alias, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; @@ -1685,10 +1686,10 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, goto cleanup; if (tlsAlias) -ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias, false)); if (secAlias) -ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); +ignore_value(qemuMonitorDelObject(priv->mon, secAlias, false)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); @@ -1850,9 +1851,9 @@ qemuDomainDelChardevTLSObjects(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); -ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias, false)); if (secAlias) -ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); +ignore_value(qemuMonitorDelObject(priv->mon, secAlias, false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; @@ -2307,7 +2308,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, exit_monitor: virErrorPreserveLast(_err); if (objAlias) -ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); +ignore_value(qemuMonitorDelObject(priv->mon,
[PATCH 0/5] qemu: Don't log spurious errors on qemuMonitorDelObject
qemuMonitorDelObject is often used in cleanup cases so we need to control whether to log errors. First patch actually prevents one of the spurious calls in cases we know it would be pointless. Peter Krempa (5): qemuDomainChangeEjectableMedia: Don't always remove managed PR daemon qemuMonitorJSON(Add|Del)Object: Refactor cleanup qemuMonitorJSONCheckError: Use g_autofree qemuMonitorJSONCheckError: Allow suppressing of error reporting qemu: Suppress error reporting from qemuMonitorDelObject in cleanup paths src/qemu/qemu_block.c| 10 ++--- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 34 +--- src/qemu/qemu_monitor.c | 5 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 77 ++-- src/qemu/qemu_monitor_json.h | 3 +- 7 files changed, 71 insertions(+), 63 deletions(-) -- 2.24.1
Re: [PATCH v2 1/1] cpu_map: Add more -noTSX x86 CPU models
On Tue, Mar 10, 2020 at 11:48:06 +0100, Christian Ehrhardt wrote: > One of the mitigation methods for TAA[1] is to disable TSX > support on the host system. Linux added a mechanism to disable > TSX globally through the kernel command line, and many Linux > distributions now default to tsx=off. This makes existing CPU > models that have HLE and RTM enabled not usable anymore. > > Add new versions of all CPU models that have the HLE and RTM > features enabled, that can be used when TSX is disabled in the > host system. > > On systems disabling the features without those types defined > in cpu-maps users end up without modern CPU types in the list > of usable CPUs to use in the likes of virsh domcapabilities > or tools higher in the stack like virt-manager. > > This adds: > -Cascadelake-Server-noTSX > -Icelake-Client-noTSX > -Icelake-Server-noTSX > -Skylake-Server-noTSX-IBRS > -Skylake-Client-noTSX-IBRS > > Introduced in QEMU by commit v4.2.0-rc2-3-g9ab2237f19 (function) > and commit v4.2.0-rc2-4-g02fa60d101 (names) > > References: > > [1] TAA, TSX asynchronous Abort: > > https://software.intel.com/security-software-guidance/insights/deep-dive-intel-transactional-synchronization-extensions-intel-tsx-asynchronous-abort > > https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1853200 > > Signed-off-by: Christian Ehrhardt Reviewed-by: Jiri Denemark I just resent this patch (with my Reviewed-by already applied) together with a few additional patches for not using the new noTSX model for host-model CPUs. Jirka
[libvirt PATCH 4/4] cpu_map: Don't use new noTSX models for host-model CPUs
Host-model CPU definitions (and domain capabilities) will use the original CPU models (without noTSX in their name) and explicitly disable hle and rtm features. This way domains with host-model CPUs will be migratable even to older versions of libvirt which do not support the noTSX model variants. The new models will be advertised in host capabilities and they may be used explicitly with custom CPUs. Signed-off-by: Jiri Denemark --- src/cpu_map/x86_Cascadelake-Server-noTSX.xml | 2 +- src/cpu_map/x86_Icelake-Client-noTSX.xml | 2 +- src/cpu_map/x86_Icelake-Server-noTSX.xml | 2 +- src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 2 +- src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 2 +- tests/cputestdata/x86_64-cpuid-Core-i7-8550U-guest.xml | 4 +++- tests/cputestdata/x86_64-cpuid-Core-i7-8550U-json.xml | 4 +++- 7 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/cpu_map/x86_Cascadelake-Server-noTSX.xml b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml index 5adea664e9..459174a30d 100644 --- a/src/cpu_map/x86_Cascadelake-Server-noTSX.xml +++ b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml @@ -1,6 +1,6 @@ - + diff --git a/src/cpu_map/x86_Icelake-Client-noTSX.xml b/src/cpu_map/x86_Icelake-Client-noTSX.xml index 540732af6f..65e648ae21 100644 --- a/src/cpu_map/x86_Icelake-Client-noTSX.xml +++ b/src/cpu_map/x86_Icelake-Client-noTSX.xml @@ -1,6 +1,6 @@ - + diff --git a/src/cpu_map/x86_Icelake-Server-noTSX.xml b/src/cpu_map/x86_Icelake-Server-noTSX.xml index 5a53da23c7..2fd6906406 100644 --- a/src/cpu_map/x86_Icelake-Server-noTSX.xml +++ b/src/cpu_map/x86_Icelake-Server-noTSX.xml @@ -1,6 +1,6 @@ - + diff --git a/src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml b/src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml index 0c2f1e6ac4..ffba34502a 100644 --- a/src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml +++ b/src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml @@ -1,6 +1,6 @@ - + diff --git a/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-guest.xml b/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-guest.xml index e03c4a06ba..92404e4d03 100644 --- a/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-guest.xml +++ b/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-guest.xml @@ -1,5 +1,5 @@ - Skylake-Client-noTSX-IBRS + Skylake-Client-IBRS Intel @@ -26,4 +26,6 @@ + + diff --git a/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-json.xml b/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-json.xml index 3d8e6775bf..645c0934c2 100644 --- a/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-json.xml +++ b/tests/cputestdata/x86_64-cpuid-Core-i7-8550U-json.xml @@ -1,5 +1,5 @@ - Skylake-Client-noTSX-IBRS + Skylake-Client-IBRS Intel @@ -14,4 +14,6 @@ + + -- 2.25.1
[libvirt PATCH 3/4] cpu_x86: Honor CPU models' element
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 366337ef57..ce15bb0454 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2044,10 +2044,23 @@ x86DecodeUseCandidate(virCPUx86ModelPtr current,
virCPUx86ModelPtr candidate,
virCPUDefPtr cpuCandidate,
uint32_t signature,
- const char *preferred,
- bool checkPolicy)
+ const char *preferred)
{
-if (checkPolicy) {
+if (cpuCandidate->type == VIR_CPU_TYPE_HOST &&
+!candidate->decodeHost) {
+VIR_DEBUG("%s is not supposed to be used for host CPU definition",
+ cpuCandidate->model);
+return 0;
+}
+
+if (cpuCandidate->type == VIR_CPU_TYPE_GUEST &&
+!candidate->decodeGuest) {
+VIR_DEBUG("%s is not supposed to be used for guest CPU definition",
+ cpuCandidate->model);
+return 0;
+}
+
+if (cpuCandidate->type == VIR_CPU_TYPE_HOST) {
size_t i;
for (i = 0; i < cpuCandidate->nfeatures; i++) {
if (cpuCandidate->features[i].policy == VIR_CPU_FEATURE_DISABLE)
@@ -2209,8 +,7 @@ x86Decode(virCPUDefPtr cpu,
if ((rc = x86DecodeUseCandidate(model, cpuModel,
candidate, cpuCandidate,
-signature, preferred,
-cpu->type == VIR_CPU_TYPE_HOST))) {
+signature, preferred))) {
virCPUDefFree(cpuModel);
cpuModel = cpuCandidate;
model = candidate;
--
2.25.1
[libvirt PATCH 2/4] cpu_map: Add element to x86 CPU model definitions
The element specifies whether a particular CPU model can be used when
creating a CPU definition from raw CPUID/MSR data. The @host attribute
determines whether the CPU model can be used (host='on') for creating
CPU definition for host capabilities. Usability of the model for domain
capabilities and host-model CPU definitions is controlled by the @guest
attribute.
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86.c | 43 +++
src/cpu_map/x86_486.xml | 1 +
src/cpu_map/x86_Broadwell-IBRS.xml| 1 +
src/cpu_map/x86_Broadwell-noTSX-IBRS.xml | 1 +
src/cpu_map/x86_Broadwell-noTSX.xml | 1 +
src/cpu_map/x86_Broadwell.xml | 1 +
src/cpu_map/x86_Cascadelake-Server-noTSX.xml | 1 +
src/cpu_map/x86_Cascadelake-Server.xml| 1 +
src/cpu_map/x86_Conroe.xml| 1 +
src/cpu_map/x86_Dhyana.xml| 1 +
src/cpu_map/x86_EPYC-IBPB.xml | 1 +
src/cpu_map/x86_EPYC.xml | 1 +
src/cpu_map/x86_Haswell-IBRS.xml | 1 +
src/cpu_map/x86_Haswell-noTSX-IBRS.xml| 1 +
src/cpu_map/x86_Haswell-noTSX.xml | 1 +
src/cpu_map/x86_Haswell.xml | 1 +
src/cpu_map/x86_Icelake-Client-noTSX.xml | 1 +
src/cpu_map/x86_Icelake-Client.xml| 1 +
src/cpu_map/x86_Icelake-Server-noTSX.xml | 1 +
src/cpu_map/x86_Icelake-Server.xml| 1 +
src/cpu_map/x86_IvyBridge-IBRS.xml| 1 +
src/cpu_map/x86_IvyBridge.xml | 1 +
src/cpu_map/x86_Nehalem-IBRS.xml | 1 +
src/cpu_map/x86_Nehalem.xml | 1 +
src/cpu_map/x86_Opteron_G1.xml| 1 +
src/cpu_map/x86_Opteron_G2.xml| 1 +
src/cpu_map/x86_Opteron_G3.xml| 1 +
src/cpu_map/x86_Opteron_G4.xml| 1 +
src/cpu_map/x86_Opteron_G5.xml| 1 +
src/cpu_map/x86_Penryn.xml| 1 +
src/cpu_map/x86_SandyBridge-IBRS.xml | 1 +
src/cpu_map/x86_SandyBridge.xml | 1 +
src/cpu_map/x86_Skylake-Client-IBRS.xml | 1 +
src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 1 +
src/cpu_map/x86_Skylake-Client.xml| 1 +
src/cpu_map/x86_Skylake-Server-IBRS.xml | 1 +
src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 1 +
src/cpu_map/x86_Skylake-Server.xml| 1 +
src/cpu_map/x86_Westmere-IBRS.xml | 1 +
src/cpu_map/x86_Westmere.xml | 1 +
src/cpu_map/x86_athlon.xml| 1 +
src/cpu_map/x86_core2duo.xml | 1 +
src/cpu_map/x86_coreduo.xml | 1 +
src/cpu_map/x86_cpu64-rhel5.xml | 1 +
src/cpu_map/x86_cpu64-rhel6.xml | 1 +
src/cpu_map/x86_kvm32.xml | 1 +
src/cpu_map/x86_kvm64.xml | 1 +
src/cpu_map/x86_n270.xml | 1 +
src/cpu_map/x86_pentium.xml | 1 +
src/cpu_map/x86_pentium2.xml | 1 +
src/cpu_map/x86_pentium3.xml | 1 +
src/cpu_map/x86_pentiumpro.xml| 1 +
src/cpu_map/x86_phenom.xml| 1 +
src/cpu_map/x86_qemu32.xml| 1 +
src/cpu_map/x86_qemu64.xml| 1 +
55 files changed, 97 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 7a8a2e3f3b..366337ef57 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -125,6 +125,8 @@ typedef struct _virCPUx86Model virCPUx86Model;
typedef virCPUx86Model *virCPUx86ModelPtr;
struct _virCPUx86Model {
char *name;
+bool decodeHost;
+bool decodeGuest;
virCPUx86VendorPtr vendor;
size_t nsignatures;
uint32_t *signatures;
@@ -1347,6 +1349,44 @@ x86ModelCompare(virCPUx86ModelPtr model1,
}
+static int
+x86ModelParseDecode(virCPUx86ModelPtr model,
+xmlXPathContextPtr ctxt)
+{
+g_autofree char *host = NULL;
+g_autofree char *guest = NULL;
+int val;
+
+if ((host = virXPathString("string(./decode/@host)", ctxt)))
+val = virTristateSwitchTypeFromString(host);
+else
+val = VIR_TRISTATE_SWITCH_ABSENT;
+
+if (val <= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid or missing decode/host attribute in CPU
model %s"),
+ model->name);
+return -1;
+}
+model->decodeHost = val == VIR_TRISTATE_SWITCH_ON;
+
+if ((guest = virXPathString("string(./decode/@guest)", ctxt)))
+val = virTristateSwitchTypeFromString(guest);
+else
+val = VIR_TRISTATE_SWITCH_ABSENT;
+
+if (val <= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid or missing decode/guest attribute in CPU
model %s"),
+ model->name);
+return -1;
+}
+model->decodeGuest = val ==
[libvirt PATCH 1/4] cpu_map: Add more -noTSX x86 CPU models
From: Christian Ehrhardt One of the mitigation methods for TAA[1] is to disable TSX support on the host system. Linux added a mechanism to disable TSX globally through the kernel command line, and many Linux distributions now default to tsx=off. This makes existing CPU models that have HLE and RTM enabled not usable anymore. Add new versions of all CPU models that have the HLE and RTM features enabled, that can be used when TSX is disabled in the host system. On systems disabling the features without those types defined in cpu-maps users end up without modern CPU types in the list of usable CPUs to use in the likes of virsh domcapabilities or tools higher in the stack like virt-manager. This adds: -Cascadelake-Server-noTSX -Icelake-Client-noTSX -Icelake-Server-noTSX -Skylake-Server-noTSX-IBRS -Skylake-Client-noTSX-IBRS Introduced in QEMU by commit v4.2.0-rc2-3-g9ab2237f19 (function) and commit v4.2.0-rc2-4-g02fa60d101 (names) References: [1] TAA, TSX asynchronous Abort: https://software.intel.com/security-software-guidance/insights/deep-dive-intel-transactional-synchronization-extensions-intel-tsx-asynchronous-abort https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1853200 Signed-off-by: Christian Ehrhardt Message-Id: <20200310104806.2723-2-christian.ehrha...@canonical.com> Reviewed-by: Jiri Denemark --- src/cpu_map/Makefile.inc.am | 5 ++ src/cpu_map/index.xml | 5 ++ src/cpu_map/x86_Cascadelake-Server-noTSX.xml | 78 src/cpu_map/x86_Icelake-Client-noTSX.xml | 81 + src/cpu_map/x86_Icelake-Server-noTSX.xml | 90 +++ src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 73 +++ src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 75 .../x86_64-cpuid-Core-i7-8550U-guest.xml | 4 +- .../x86_64-cpuid-Core-i7-8550U-host.xml | 11 +-- .../x86_64-cpuid-Core-i7-8550U-json.xml | 4 +- .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 5 ++ .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 5 ++ .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 5 ++ .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 5 ++ 16 files changed, 440 insertions(+), 16 deletions(-) create mode 100644 src/cpu_map/x86_Cascadelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Client-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml create mode 100644 src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am index e935178304..be64c9a0d4 100644 --- a/src/cpu_map/Makefile.inc.am +++ b/src/cpu_map/Makefile.inc.am @@ -20,6 +20,7 @@ cpumap_DATA = \ cpu_map/x86_Broadwell-noTSX.xml \ cpu_map/x86_Broadwell-noTSX-IBRS.xml \ cpu_map/x86_Cascadelake-Server.xml \ + cpu_map/x86_Cascadelake-Server-noTSX.xml \ cpu_map/x86_Conroe.xml \ cpu_map/x86_core2duo.xml \ cpu_map/x86_coreduo.xml \ @@ -33,7 +34,9 @@ cpumap_DATA = \ cpu_map/x86_Haswell-noTSX.xml \ cpu_map/x86_Haswell-noTSX-IBRS.xml \ cpu_map/x86_Icelake-Client.xml \ + cpu_map/x86_Icelake-Client-noTSX.xml \ cpu_map/x86_Icelake-Server.xml \ + cpu_map/x86_Icelake-Server-noTSX.xml \ cpu_map/x86_IvyBridge.xml \ cpu_map/x86_IvyBridge-IBRS.xml \ cpu_map/x86_kvm32.xml \ @@ -58,8 +61,10 @@ cpumap_DATA = \ cpu_map/x86_SandyBridge-IBRS.xml \ cpu_map/x86_Skylake-Client.xml \ cpu_map/x86_Skylake-Client-IBRS.xml \ + cpu_map/x86_Skylake-Client-noTSX-IBRS.xml \ cpu_map/x86_Skylake-Server.xml \ cpu_map/x86_Skylake-Server-IBRS.xml \ + cpu_map/x86_Skylake-Server-noTSX-IBRS.xml \ cpu_map/x86_Westmere.xml \ cpu_map/x86_Westmere-IBRS.xml \ $(NULL) diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml index ffb2f6fe1b..50b030de29 100644 --- a/src/cpu_map/index.xml +++ b/src/cpu_map/index.xml @@ -44,11 +44,16 @@ + + + + + diff --git a/src/cpu_map/x86_Cascadelake-Server-noTSX.xml b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml new file mode 100644 index 00..d24415ebce --- /dev/null +++ b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml @@ -0,0 +1,78 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
[libvirt PATCH 0/4] cpu_map: Add more -noTSX x86 CPU models
Christian Ehrhardt (1): cpu_map: Add more -noTSX x86 CPU models Jiri Denemark (3): cpu_map: Add element to x86 CPU model definitions cpu_x86: Honor CPU models' element cpu_map: Don't use new noTSX models for host-model CPUs src/cpu/cpu_x86.c | 65 - src/cpu_map/Makefile.inc.am | 5 + src/cpu_map/index.xml | 5 + src/cpu_map/x86_486.xml | 1 + src/cpu_map/x86_Broadwell-IBRS.xml| 1 + src/cpu_map/x86_Broadwell-noTSX-IBRS.xml | 1 + src/cpu_map/x86_Broadwell-noTSX.xml | 1 + src/cpu_map/x86_Broadwell.xml | 1 + src/cpu_map/x86_Cascadelake-Server-noTSX.xml | 79 src/cpu_map/x86_Cascadelake-Server.xml| 1 + src/cpu_map/x86_Conroe.xml| 1 + src/cpu_map/x86_Dhyana.xml| 1 + src/cpu_map/x86_EPYC-IBPB.xml | 1 + src/cpu_map/x86_EPYC.xml | 1 + src/cpu_map/x86_Haswell-IBRS.xml | 1 + src/cpu_map/x86_Haswell-noTSX-IBRS.xml| 1 + src/cpu_map/x86_Haswell-noTSX.xml | 1 + src/cpu_map/x86_Haswell.xml | 1 + src/cpu_map/x86_Icelake-Client-noTSX.xml | 82 + src/cpu_map/x86_Icelake-Client.xml| 1 + src/cpu_map/x86_Icelake-Server-noTSX.xml | 91 +++ src/cpu_map/x86_Icelake-Server.xml| 1 + src/cpu_map/x86_IvyBridge-IBRS.xml| 1 + src/cpu_map/x86_IvyBridge.xml | 1 + src/cpu_map/x86_Nehalem-IBRS.xml | 1 + src/cpu_map/x86_Nehalem.xml | 1 + src/cpu_map/x86_Opteron_G1.xml| 1 + src/cpu_map/x86_Opteron_G2.xml| 1 + src/cpu_map/x86_Opteron_G3.xml| 1 + src/cpu_map/x86_Opteron_G4.xml| 1 + src/cpu_map/x86_Opteron_G5.xml| 1 + src/cpu_map/x86_Penryn.xml| 1 + src/cpu_map/x86_SandyBridge-IBRS.xml | 1 + src/cpu_map/x86_SandyBridge.xml | 1 + src/cpu_map/x86_Skylake-Client-IBRS.xml | 1 + src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 74 +++ src/cpu_map/x86_Skylake-Client.xml| 1 + src/cpu_map/x86_Skylake-Server-IBRS.xml | 1 + src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 76 src/cpu_map/x86_Skylake-Server.xml| 1 + src/cpu_map/x86_Westmere-IBRS.xml | 1 + src/cpu_map/x86_Westmere.xml | 1 + src/cpu_map/x86_athlon.xml| 1 + src/cpu_map/x86_core2duo.xml | 1 + src/cpu_map/x86_coreduo.xml | 1 + src/cpu_map/x86_cpu64-rhel5.xml | 1 + src/cpu_map/x86_cpu64-rhel6.xml | 1 + src/cpu_map/x86_kvm32.xml | 1 + src/cpu_map/x86_kvm64.xml | 1 + src/cpu_map/x86_n270.xml | 1 + src/cpu_map/x86_pentium.xml | 1 + src/cpu_map/x86_pentium2.xml | 1 + src/cpu_map/x86_pentium3.xml | 1 + src/cpu_map/x86_pentiumpro.xml| 1 + src/cpu_map/x86_phenom.xml| 1 + src/cpu_map/x86_qemu32.xml| 1 + src/cpu_map/x86_qemu64.xml| 1 + .../x86_64-cpuid-Core-i7-8550U-host.xml | 11 +-- .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 5 + .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 5 + tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 5 + .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 5 + .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 5 + tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 5 + 64 files changed, 552 insertions(+), 15 deletions(-) create mode 100644 src/cpu_map/x86_Cascadelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Client-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml create mode 100644 src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml -- 2.25.1
Re: [PATCH v2 0/2] vmx: make 'fileName' optional for CD-ROMs
On a Wednesday in 2020, Pino Toscano wrote: v1 is: https://www.redhat.com/archives/libvir-list/2020-March/msg00616.html Changes from v1: - added a patch to shortcut few checks - use NULLSTR - handle floppies (poor guys) Pino Toscano (2): vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk() vmx: make 'fileName' optional for CD-ROMs src/vmx/vmx.c | 67 ++- .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 ++ .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++ tests/vmx2xmltest.c | 1 + 4 files changed, 62 insertions(+), 33 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH v2 1/2] vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk()
Move earlier the checks for skipping a hard disk when parsing a CD-DROM,
and for skipping a CD-ROM when parsing a hard disk. This should have no
behaviour changes, and avoids to add repeated checks in following
commits.
Signed-off-by: Pino Toscano
---
src/vmx/vmx.c | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index f0140129a2..bfc9bc7404 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2218,7 +2218,21 @@ virVMXParseDisk(virVMXContext *ctx,
virDomainXMLOptionPtr xmlopt, virConfPtr con
/* Setup virDomainDiskDef */
if (device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-if (virStringHasCaseSuffix(fileName, ".vmdk")) {
+if (virStringHasCaseSuffix(fileName, ".iso") ||
+STREQ(fileName, "emptyBackingString") ||
+(deviceType &&
+ (STRCASEEQ(deviceType, "atapi-cdrom") ||
+ STRCASEEQ(deviceType, "cdrom-raw") ||
+ (STRCASEEQ(deviceType, "scsi-passthru") &&
+ STRPREFIX(fileName, "/vmfs/devices/cdrom/") {
+/*
+ * This function was called in order to parse a harddisk device,
+ * but .iso files, 'atapi-cdrom', 'cdrom-raw', and 'scsi-passthru'
+ * CDROM devices are for CDROM devices only. Just ignore it,
another
+ * call to this function to parse a CDROM device may handle it.
+ */
+goto ignore;
+} else if (virStringHasCaseSuffix(fileName, ".vmdk")) {
char *tmp;
if (deviceType != NULL) {
@@ -2254,20 +2268,6 @@ virVMXParseDisk(virVMXContext *ctx,
virDomainXMLOptionPtr xmlopt, virConfPtr con
if (mode)
(*def)->transient = STRCASEEQ(mode,
"independent-nonpersistent");
-} else if (virStringHasCaseSuffix(fileName, ".iso") ||
- STREQ(fileName, "emptyBackingString") ||
- (deviceType &&
-(STRCASEEQ(deviceType, "atapi-cdrom") ||
- STRCASEEQ(deviceType, "cdrom-raw") ||
- (STRCASEEQ(deviceType, "scsi-passthru") &&
- STRPREFIX(fileName, "/vmfs/devices/cdrom/") {
-/*
- * This function was called in order to parse a harddisk device,
- * but .iso files, 'atapi-cdrom', 'cdrom-raw', and 'scsi-passthru'
- * CDROM devices are for CDROM devices only. Just ignore it,
another
- * call to this function to parse a CDROM device may handle it.
- */
-goto ignore;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
@@ -2277,7 +2277,15 @@ virVMXParseDisk(virVMXContext *ctx,
virDomainXMLOptionPtr xmlopt, virConfPtr con
goto cleanup;
}
} else if (device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
-if (virStringHasCaseSuffix(fileName, ".iso")) {
+if (virStringHasCaseSuffix(fileName, ".vmdk")) {
+/*
+ * This function was called in order to parse a CDROM device, but
+ * .vmdk files are for harddisk devices only. Just ignore it,
+ * another call to this function to parse a harddisk device may
+ * handle it.
+ */
+goto ignore;
+} else if (virStringHasCaseSuffix(fileName, ".iso")) {
char *tmp;
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
@@ -2295,14 +2303,6 @@ virVMXParseDisk(virVMXContext *ctx,
virDomainXMLOptionPtr xmlopt, virConfPtr con
goto cleanup;
}
VIR_FREE(tmp);
-} else if (virStringHasCaseSuffix(fileName, ".vmdk")) {
-/*
- * This function was called in order to parse a CDROM device, but
- * .vmdk files are for harddisk devices only. Just ignore it,
- * another call to this function to parse a harddisk device may
- * handle it.
- */
-goto ignore;
} else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
--
2.25.1
[PATCH v2 2/2] vmx: make 'fileName' optional for CD-ROMs
It seems like CD-ROMs may have no 'fileName' property specified in case
there is nothing configured as attachment for the drive. Hence, make
sure that virVMXParseDisk() do not consider it mandatory anymore,
considering it an empty block cdrom device. Sadly virVMXParseDisk() is
used also to parse disk and floppies, so make sure that a NULL fileName
is handled in cdrom- and floppy-related paths.
https://bugzilla.redhat.com/show_bug.cgi?id=1808610
Signed-off-by: Pino Toscano
---
src/vmx/vmx.c | 25 ++-
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 +++
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +
tests/vmx2xmltest.c | 1 +
4 files changed, 41 insertions(+), 12 deletions(-)
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index bfc9bc7404..6c6ef7acf3 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2207,7 +2207,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
goto cleanup;
/* vmx:fileName -> def:src, def:type */
-if (virVMXGetConfigString(conf, fileName_name, , false) < 0)
+if (virVMXGetConfigString(conf, fileName_name, , true) < 0)
goto cleanup;
/* vmx:writeThrough -> def:cachemode */
@@ -2218,7 +2218,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
/* Setup virDomainDiskDef */
if (device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-if (virStringHasCaseSuffix(fileName, ".iso") ||
+if (fileName == NULL ||
+virStringHasCaseSuffix(fileName, ".iso") ||
STREQ(fileName, "emptyBackingString") ||
(deviceType &&
(STRCASEEQ(deviceType, "atapi-cdrom") ||
@@ -2277,7 +2278,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
goto cleanup;
}
} else if (device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
-if (virStringHasCaseSuffix(fileName, ".vmdk")) {
+if (fileName && virStringHasCaseSuffix(fileName, ".vmdk")) {
/*
* This function was called in order to parse a CDROM device, but
* .vmdk files are for harddisk devices only. Just ignore it,
@@ -2285,7 +2286,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
* handle it.
*/
goto ignore;
-} else if (virStringHasCaseSuffix(fileName, ".iso")) {
+} else if (fileName && virStringHasCaseSuffix(fileName, ".iso")) {
char *tmp;
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
@@ -2306,7 +2307,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
} else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
-if (STRCASEEQ(fileName, "auto detect")) {
+if (fileName && STRCASEEQ(fileName, "auto detect")) {
ignore_value(virDomainDiskSetSource(*def, NULL));
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
@@ -2317,7 +2318,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
(*def)->device = VIR_DOMAIN_DISK_DEVICE_LUN;
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
-if (STRCASEEQ(fileName, "auto detect")) {
+if (fileName && STRCASEEQ(fileName, "auto detect")) {
ignore_value(virDomainDiskSetSource(*def, NULL));
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
@@ -2325,7 +2326,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
}
} else if (busType == VIR_DOMAIN_DISK_BUS_SCSI &&
deviceType && STRCASEEQ(deviceType, "scsi-passthru")) {
-if (STRPREFIX(fileName, "/vmfs/devices/cdrom/")) {
+if (fileName && STRPREFIX(fileName, "/vmfs/devices/cdrom/")) {
/* SCSI-passthru CD-ROMs actually are device='lun' */
(*def)->device = VIR_DOMAIN_DISK_DEVICE_LUN;
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
@@ -2341,7 +2342,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr
xmlopt, virConfPtr con
*/
goto ignore;
}
-} else if (STREQ(fileName, "emptyBackingString")) {
+} else if (fileName && STREQ(fileName, "emptyBackingString")) {
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
[PATCH v2 0/2] vmx: make 'fileName' optional for CD-ROMs
v1 is: https://www.redhat.com/archives/libvir-list/2020-March/msg00616.html Changes from v1: - added a patch to shortcut few checks - use NULLSTR - handle floppies (poor guys) Pino Toscano (2): vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk() vmx: make 'fileName' optional for CD-ROMs src/vmx/vmx.c | 67 ++- .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 ++ .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++ tests/vmx2xmltest.c | 1 + 4 files changed, 62 insertions(+), 33 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml -- 2.25.1
