Re: [libvirt] [PATCH v2 6/6] block/dirty-bitmaps: move comment block
On 2/13/19 5:23 PM, John Snow wrote: > Simply move the big status enum comment block to above the status > function, and document it as being deprecated. The whole confusing > block can get deleted in three releases time. > > Signed-off-by: John Snow > --- > block/dirty-bitmap.c | 36 +++- > 1 file changed, 19 insertions(+), 17 deletions(-) > > -/* Called with BQL taken. */ > +/** > + * bdrv_dirty_bitmap_status: This API is now deprecated. > + * Called with BQL taken. > + * > + * A BdrvDirtyBitmap can be in four possible user-visible states: > + * (1) Active: successor is NULL, and disabled is false: full r/w mode > + * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode, > + * guest writes are dropped, but monitor writes are possible, > + * through commands like merge and clear. > + * (3) Frozen: successor is not NULL. > + * A frozen bitmap cannot be renamed, deleted, cleared, set, > + * enabled, merged to, etc. A frozen bitmap can only abdicate() > + * or reclaim(). > + * In this state, the anonymous successor bitmap may be either > + * Active and recording writes from the guest (e.g. backup > jobs), > + * but it can be Disabled and not recording writes. Pre-existing due to code motion, but you could improve the grammar with s/but/or/ > + * (4) Locked: Whether Active or Disabled, the user cannot modify this > bitmap > + * in any way from the monitor. > + */ Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper
On 2/13/19 5:23 PM, John Snow wrote: > "Frozen" was a good description a long time ago, but it isn't adequate now. > Rename the frozen predicate to has_successor to make the semantics of the > predicate more clear to outside callers. > > In the process, remove some calls to frozen() that no longer semantically > make sense. For enabled and disabled in particular, it's actually okay for > the internals to do this but only forbidden for users to invoke them, and > all of the QMP entry uses already check against qmp_locked. > > Several other assertions really want to check that the bitmap isn't in-use > by another operation -- use the qmp_locked function for this instead, which > presently also checks for has_successor. > > Signed-off-by: John Snow > --- > block/dirty-bitmap.c | 32 +--- > include/block/dirty-bitmap.h | 2 +- > migration/block-dirty-bitmap.c | 2 +- > 3 files changed, 19 insertions(+), 17 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/6] block/dirty-bitmap: add recording and busy properties
On 2/13/19 5:23 PM, John Snow wrote: > The current API allows us to report a single status, which we've defined as: > > Frozen: has a successor, treated as qmp_locked, may or may not be enabled. > Locked: no successor, qmp_locked. may or may not be enabled. > Disabled: Not frozen or locked, disabled. > Active: Not frozen, locked, or disabled. > > The problem is that both "Frozen" and "Locked" mean nearly the same thing, > and that both of them do not intuit whether they are recording guest writes > or not. > > This patch deprecates that status field and introduces two orthogonal > properties instead to replace it. > > Signed-off-by: John Snow > --- > block/dirty-bitmap.c | 9 + > qapi/block-core.json | 10 +- > qemu-deprecated.texi | 6 ++ > tests/qemu-iotests/236.out | 28 > 4 files changed, 52 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 6/6] block/dirty-bitmaps: move comment block
Simply move the big status enum comment block to above the status
function, and document it as being deprecated. The whole confusing
block can get deleted in three releases time.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ab048385a..fc390cae94 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -28,22 +28,6 @@
#include "block/block_int.h"
#include "block/blockjob.h"
-/**
- * A BdrvDirtyBitmap can be in four possible user-visible states:
- * (1) Active: successor is NULL, and disabled is false: full r/w mode
- * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
- * guest writes are dropped, but monitor writes are possible,
- * through commands like merge and clear.
- * (3) Frozen: successor is not NULL.
- * A frozen bitmap cannot be renamed, deleted, cleared, set,
- * enabled, merged to, etc. A frozen bitmap can only abdicate()
- * or reclaim().
- * In this state, the anonymous successor bitmap may be either
- * Active and recording writes from the guest (e.g. backup jobs),
- * but it can be Disabled and not recording writes.
- * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap
- * in any way from the monitor.
- */
struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap;/* Dirty bitmap implementation */
@@ -205,7 +189,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
return !bitmap->disabled;
}
-/* Called with BQL taken. */
+/**
+ * bdrv_dirty_bitmap_status: This API is now deprecated.
+ * Called with BQL taken.
+ *
+ * A BdrvDirtyBitmap can be in four possible user-visible states:
+ * (1) Active: successor is NULL, and disabled is false: full r/w mode
+ * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
+ * guest writes are dropped, but monitor writes are possible,
+ * through commands like merge and clear.
+ * (3) Frozen: successor is not NULL.
+ * A frozen bitmap cannot be renamed, deleted, cleared, set,
+ * enabled, merged to, etc. A frozen bitmap can only abdicate()
+ * or reclaim().
+ * In this state, the anonymous successor bitmap may be either
+ * Active and recording writes from the guest (e.g. backup jobs),
+ * but it can be Disabled and not recording writes.
+ * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap
+ * in any way from the monitor.
+ */
DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
{
if (bdrv_dirty_bitmap_has_successor(bitmap)) {
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
These mean the same thing now. Unify them and rename the merged call
bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
as well as help disambiguate from the various _locked and _unlocked
versions of bitmap helpers that refer to mutex locks.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
block/dirty-bitmap.c | 41 +++---
blockdev.c | 18 +++
include/block/dirty-bitmap.h | 5 ++---
migration/block-dirty-bitmap.c | 6 ++---
nbd/server.c | 6 ++---
5 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 2042c62602..8ab048385a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap;/* Dirty bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
-bool qmp_locked;/* Bitmap is locked, it can't be modified
- through QMP */
-BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state
*/
+bool busy; /* Bitmap is busy, it can't be modified through
+ QMP */
+BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
bool disabled; /* Bitmap is disabled. It ignores all writes to
@@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
*bitmap)
return bitmap->successor;
}
-bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_qmp_locked(bitmap);
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+return bitmap->busy;
}
-void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
+void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
{
qemu_mutex_lock(bitmap->mutex);
-bitmap->qmp_locked = qmp_locked;
+bitmap->busy = busy;
qemu_mutex_unlock(bitmap->mutex);
}
-bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
-{
-return bitmap->qmp_locked;
-}
-
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
@@ -215,7 +210,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap
*bitmap)
{
if (bdrv_dirty_bitmap_has_successor(bitmap)) {
return DIRTY_BITMAP_STATUS_FROZEN;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+} else if (bdrv_dirty_bitmap_busy(bitmap)) {
return DIRTY_BITMAP_STATUS_LOCKED;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
return DIRTY_BITMAP_STATUS_DISABLED;
@@ -242,7 +237,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
uint64_t granularity;
BdrvDirtyBitmap *child;
-if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+if (bdrv_dirty_bitmap_busy(bitmap)) {
error_setg(errp, "Cannot create a successor for a bitmap that is
in-use "
"by an operation");
return -1;
@@ -266,7 +261,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Install the successor and lock the parent */
bitmap->successor = child;
-bitmap->qmp_locked = true;
+bitmap->busy = true;
return 0;
}
@@ -288,7 +283,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap
*bitmap)
static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
{
assert(!bitmap->active_iterators);
-assert(!bdrv_dirty_bitmap_user_locked(bitmap));
+assert(!bdrv_dirty_bitmap_busy(bitmap));
assert(!bitmap->meta);
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
@@ -320,7 +315,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
bitmap->successor = NULL;
successor->persistent = bitmap->persistent;
bitmap->persistent = false;
-bitmap->qmp_locked = false;
+bitmap->busy = false;
bdrv_release_dirty_bitmap(bs, bitmap);
return successor;
@@ -329,7 +324,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
/**
* In cases of failure where we can no longer safely delete the parent,
* we may wish to re-join the parent and child/successor.
- * The merged parent will not be user_locked, nor explicitly re-enabled.
+ * The merged parent will not be busy, nor explicitly re-enabled.
* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
*/
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -349,7 +344,7 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
}
parent->disabled = successor->disabled;
-parent->qmp_locked = false;
+parent->busy = false;
bdrv_release_dirty_bitmap_locked(successor);
parent->successor = NULL;
@@ -380,7 +375,7 @@ void
[libvirt] [PATCH v2 4/6] block/dirty-bitmap: explicitly lock bitmaps with successors
Instead of implying a locked status, make it explicit.
Now, bitmaps in use by migration, NBD or backup operations
are all treated the same way with the same code paths.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
block/dirty-bitmap.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bb2e19e0d8..2042c62602 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
*bitmap)
return bitmap->successor;
}
-/* Both conditions disallow user-modification via QMP. */
bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_has_successor(bitmap) ||
- bdrv_dirty_bitmap_qmp_locked(bitmap);
+return bdrv_dirty_bitmap_qmp_locked(bitmap);
}
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
@@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
child->disabled = bitmap->disabled;
bitmap->disabled = true;
-/* Install the successor and freeze the parent */
+/* Install the successor and lock the parent */
bitmap->successor = child;
+bitmap->qmp_locked = true;
return 0;
}
@@ -321,6 +320,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
bitmap->successor = NULL;
successor->persistent = bitmap->persistent;
bitmap->persistent = false;
+bitmap->qmp_locked = false;
bdrv_release_dirty_bitmap(bs, bitmap);
return successor;
@@ -349,6 +349,7 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
}
parent->disabled = successor->disabled;
+parent->qmp_locked = false;
bdrv_release_dirty_bitmap_locked(successor);
parent->successor = NULL;
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/6] block/dirty-bitmap: change semantics of enabled predicate
Currently, enabled means something like "the status of the bitmap
is ACTIVE." After this patch, it should mean exclusively: "This
bitmap is recording guest writes, and is allowed to do so."
In many places, this is how this predicate was already used.
We'll allow users to call user_locked if they're really curious about
finding out if the bitmap is in use by an operation.
To accommodate this, modify the create_successor routine to now
explicitly disable the parent bitmap at creation time.
Justifications:
1. bdrv_dirty_bitmap_status suffers no change from the lack of
1:1 parity with the new predicates because of the order in which
the predicates are checked. This is now only for compatibility.
2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
disabled bitmaps -- all of these writes are internal usages.
Therefore, we should allow writes even in the disabled state.
The condition is removed.
3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
mirror and migration. In these contexts it is always enabled anyway,
but our API does not need to enforce this.
4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were
disabled or had a successor, while post-patch it is only skipping bitmaps
that are disabled. To accommodate this, create_successor now ensures that
any bitmap with a successor is explicitly disabled.
5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if the
bitmap was enabled or not. Theoretically if we save during an operation,
this now gets set as enabled instead of disabled, but this cannot happen
in practice because jobs must be finished before we close the disk.
6. block_dirty_bitmap_enable_prepare only ever cared about the
literal bit, and already checked for user_locked beforehand.
7. block_dirty_bitmap_disable_prepare ditto as above.
8. init_dirty_bitmap_migration also already checks user_locked,
so this call can be a simple enabled/disabled check.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
block/dirty-bitmap.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 639ebc0645..bb2e19e0d8 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
-return !(bitmap->disabled || bitmap->successor);
+return !bitmap->disabled;
}
/* Called with BQL taken. */
@@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Successor will be on or off based on our current state. */
child->disabled = bitmap->disabled;
+bitmap->disabled = true;
/* Install the successor and freeze the parent */
bitmap->successor = child;
@@ -346,6 +347,8 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
error_setg(errp, "Merging of parent and successor bitmap failed");
return NULL;
}
+
+parent->disabled = successor->disabled;
bdrv_release_dirty_bitmap_locked(successor);
parent->successor = NULL;
@@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
-assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
hbitmap_set(bitmap->bitmap, offset, bytes);
}
@@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
-assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
hbitmap_reset(bitmap->bitmap, offset, bytes);
}
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper
"Frozen" was a good description a long time ago, but it isn't adequate now.
Rename the frozen predicate to has_successor to make the semantics of the
predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically
make sense. For enabled and disabled in particular, it's actually okay for
the internals to do this but only forbidden for users to invoke them, and
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use
by another operation -- use the qmp_locked function for this instead, which
presently also checks for has_successor.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 32 +---
include/block/dirty-bitmap.h | 2 +-
migration/block-dirty-bitmap.c | 2 +-
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 101383b3af..639ebc0645 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
HBitmap *meta; /* Meta dirty bitmap */
bool qmp_locked;/* Bitmap is locked, it can't be modified
through QMP */
-BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state
*/
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
bool disabled; /* Bitmap is disabled. It ignores all writes to
@@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap
*bitmap)
}
/* Called with BQL taken. */
-bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
{
return bitmap->successor;
}
/* Both conditions disallow user-modification via QMP. */
bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_frozen(bitmap) ||
+return bdrv_dirty_bitmap_has_successor(bitmap) ||
bdrv_dirty_bitmap_qmp_locked(bitmap);
}
@@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
/* Called with BQL taken. */
DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
{
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_has_successor(bitmap)) {
return DIRTY_BITMAP_STATUS_FROZEN;
} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
return DIRTY_BITMAP_STATUS_LOCKED;
@@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap
*bitmap)
/**
* Create a successor bitmap destined to replace this bitmap after an
operation.
- * Requires that the bitmap is not frozen and has no successor.
+ * Requires that the bitmap is not locked and has no successor.
* Called with BQL taken.
*/
int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
@@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState
*bs,
uint64_t granularity;
BdrvDirtyBitmap *child;
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
-error_setg(errp, "Cannot create a successor for a bitmap that is "
- "currently frozen");
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+error_setg(errp, "Cannot create a successor for a bitmap that is
in-use "
+ "by an operation");
+return -1;
+}
+if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+error_setg(errp, "Cannot create a successor for a bitmap that already "
+ "has one");
return -1;
}
-assert(!bitmap->successor);
/* Create an anonymous successor */
granularity = bdrv_dirty_bitmap_granularity(bitmap);
@@ -268,7 +272,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
{
-assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = false;
}
@@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap
*bitmap)
static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
{
assert(!bitmap->active_iterators);
-assert(!bdrv_dirty_bitmap_frozen(bitmap));
+assert(!bdrv_dirty_bitmap_user_locked(bitmap));
assert(!bitmap->meta);
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
@@ -325,7 +328,7 @@ BdrvDirtyBitmap
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
/**
* In cases of failure where we can no longer safely delete the parent,
* we may wish to re-join the parent and child/successor.
- * The merged parent will be un-frozen, but not explicitly re-enabled.
+ * The merged parent will not be user_locked, nor explicitly re-enabled.
* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
*/
BdrvDirtyBitmap
[libvirt] [PATCH v2 1/6] block/dirty-bitmap: add recording and busy properties
The current API allows us to report a single status, which we've defined as:
Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
Locked: no successor, qmp_locked. may or may not be enabled.
Disabled: Not frozen or locked, disabled.
Active: Not frozen, locked, or disabled.
The problem is that both "Frozen" and "Locked" mean nearly the same thing,
and that both of them do not intuit whether they are recording guest writes
or not.
This patch deprecates that status field and introduces two orthogonal
properties instead to replace it.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 9 +
qapi/block-core.json | 10 +-
qemu-deprecated.texi | 6 ++
tests/qemu-iotests/236.out | 28
4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c6d4acebfa..101383b3af 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -226,6 +226,13 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap
*bitmap)
}
}
+/* Called with BQL taken. */
+static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
+{
+return !bitmap->disabled || (bitmap->successor &&
+ !bitmap->successor->disabled);
+}
+
/**
* Create a successor bitmap destined to replace this bitmap after an
operation.
* Requires that the bitmap is not frozen and has no successor.
@@ -448,6 +455,8 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
info->status = bdrv_dirty_bitmap_status(bm);
+info->recording = bdrv_dirty_bitmap_recording(bm);
+info->busy = bdrv_dirty_bitmap_user_locked(bm);
info->persistent = bm->persistent;
entry->value = info;
*plist = entry;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f23f2ebb8..5d1d182447 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -458,7 +458,14 @@
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
#
-# @status: current status of the dirty bitmap (since 2.4)
+# @status: Deprecated in favor of @recording and @locked. (since 2.4)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+# Replaces `active` and `disabled` statuses. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#and cannot be modified via QMP or used by another operation.
+#Replaces `locked` and `frozen` statuses. (since 4.0)
#
# @persistent: true if the bitmap will eventually be flushed to persistent
# storage (since 4.0)
@@ -467,6 +474,7 @@
##
{ 'struct': 'BlockDirtyInfo',
'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+ 'recording': 'bool', 'busy': 'bool',
'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
##
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 80b0702ad5..f7c9f4c101 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -84,6 +84,12 @@ topologies described with -smp include all possible cpus,
i.e.
"autoload" parameter is now ignored. All bitmaps are automatically loaded
from qcow2 images.
+@subsection query-block dirty-bitmaps.status parameter (since 4.0)
+
+The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
+the query-block command is deprecated. Two new boolean fields,
+``recording'' and ``busy'' effectively replace it.
+
@subsection query-cpus (since 2.12.0)
The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index 5006f7bca1..815cd053f0 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -22,17 +22,21 @@ write -P0xcd 0x3ff 64k
"bitmaps": {
"drive0": [
{
+"busy": false,
"count": 262144,
"granularity": 65536,
"name": "bitmapB",
"persistent": false,
+"recording": true,
"status": "active"
},
{
+"busy": false,
"count": 262144,
"granularity": 65536,
"name": "bitmapA",
"persistent": false,
+"recording": true,
"status": "active"
}
]
@@ -84,17 +88,21 @@ write -P0xcd 0x3ff 64k
"bitmaps": {
"drive0": [
{
+"busy": false,
"count": 262144,
"granularity": 65536,
"name": "bitmapB",
"persistent": false,
+"recording": true,
"status": "active"
},
{
+"busy": false,
"count": 262144,
"granularity": 65536,
"name": "bitmapA",
"persistent": false,
+"recording": true,
"status": "active"
}
]
@@ -184,24 +192,30 @@ write -P0xea 0x3fe 64k
"bitmaps": {
"drive0": [
{
+"busy":
[libvirt] [PATCH v2 0/6] dirty-bitmaps: deprecate @status field
The current internal meanings of "locked", "user_locked", "qmp_locked", "frozen", "enabled", and "disabled" are all a little muddled. Deprecate the @status field in favor of two new booleans that carry very specific meanings. Then, rename and rework some of the internal semantics to help make the API a bit more clear and easier to read. Well, in my opinion. Based on my current bitmaps branch (includes Eric's patch and my documentation update patch.) V2: - All of Eric's suggestions, I hope. - Vladimir's phrasing suggestion on patch 1 - Added a sixth patch that's mostly just motion. I'm on PTO the next two days, so I didn't get to writing a test for the busy bit. I think test 223 is a good candidate, because it uses the NBD functionality. To test with push mode, I need to come up with a blockdebug configuration that will let me pause an incremental backup so I can test race-free. Maybe for V3, sorry. John Snow (6): block/dirty-bitmap: add recording and busy properties block/dirty-bitmaps: rename frozen predicate helper block/dirty-bitmap: change semantics of enabled predicate block/dirty-bitmap: explicitly lock bitmaps with successors block/dirty-bitmaps: unify qmp_locked and user_locked calls block/dirty-bitmaps: move comment block block/dirty-bitmap.c | 110 ++--- blockdev.c | 18 +++--- include/block/dirty-bitmap.h | 7 +-- migration/block-dirty-bitmap.c | 8 +-- nbd/server.c | 6 +- qapi/block-core.json | 10 ++- qemu-deprecated.texi | 6 ++ tests/qemu-iotests/236.out | 28 + 8 files changed, 122 insertions(+), 71 deletions(-) -- 2.17.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] domain: Define explicit flags for saved image xml
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the save image documentation to declare it as invalid.
Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text
into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; and qemu is the only supporting
driver with support for just VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during saved image creation
needs to be migration-friendly (the saved image is loaded by the
current libvirt version, and not migrated to a potentially older
version), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from live domain API, and risking confusion if even more
domain flags are added later (in fact, I have an upcoming patch that
plans to add a new flag to virDomainGetXMLDesc that makes no sense for
saved images). On the other hand, it DOES make sense to reuse the
same flags for SaveImage and for ManagedSave (since ManagedSave is
really just sugar for creating a normal SaveImage in a location
controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake
---
include/libvirt/libvirt-domain.h | 5 +
src/libvirt-domain.c | 14 ++
src/qemu/qemu_driver.c | 4 ++--
src/remote/remote_protocol.x | 4 ++--
4 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index d82c75a9d9..1d5bdb545d 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1219,6 +1219,7 @@ int virDomainRestoreFlags
(virConnectPtr conn,
const char *dxml,
unsigned int flags);
+/* See below for virDomainSaveImageXMLFlags */
char * virDomainSaveImageGetXMLDesc(virConnectPtr conn,
const char *file,
unsigned int flags);
@@ -1571,6 +1572,10 @@ typedef enum {
VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration
*/
} virDomainXMLFlags;
+typedef enum {
+VIR_DOMAIN_SAVE_IMAGE_XML_SECURE = VIR_DOMAIN_XML_SECURE, /* dump
security sensitive information too */
+} virDomainSaveImageXMLFlags;
+
char * virDomainGetXMLDesc (virDomainPtr domain,
unsigned int flags);
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 6158382d07..4528cab0e2 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -1066,16 +1066,15 @@ virDomainRestoreFlags(virConnectPtr conn, const char
*from, const char *dxml,
* virDomainSaveImageGetXMLDesc:
* @conn: pointer to the hypervisor connection
* @file: path to saved state file
- * @flags: bitwise-OR of subset of virDomainXMLFlags
+ * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags
*
* This method will extract the XML describing the domain at the time
* a saved state file was created. @file must be a file created
* previously by virDomainSave() or virDomainSaveFlags().
*
* No security-sensitive data will be included unless @flags contains
- * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
- * connections. For this API, @flags should not contain either
- * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
+ * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only
+ * connections.
*
* Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of
* error. The caller must free() the returned value.
@@ -9483,15 +9482,14 @@ virDomainManagedSaveRemove(virDomainPtr dom, unsigned
int flags)
/**
* virDomainManagedSaveGetXMLDesc:
* @domain: a domain object
- * @flags: bitwise-OR of subset of virDomainXMLFlags
+ * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags
*
* This method will extract the XML description of the managed save
* state file of a domain.
*
* No security-sensitive data will be included unless @flags contains
- * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
- * connections. For this API, @flags should not contain either
- * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
+ * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only
+ * connections.
*
* Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of
[libvirt] [PATCH 3/3] snapshot: Define explicit flags for snapshot xml
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the snapshot documentation to declare it as invalid.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; esx and vbox don't support flags;
qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during a snapshot creation
needs to be migration-friendly (as the snapshot is not the source of
a migration), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from domain API, and risking confusion if even more domain
flags are added later (in fact, I have an upcoming patch that plans to
add a new flag to virDomainGetXMLDesc that makes no sense for
snapshots).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake
---
include/libvirt/libvirt-domain-snapshot.h | 4
src/libvirt-domain-snapshot.c | 9 -
src/qemu/qemu_driver.c| 2 +-
src/remote/remote_protocol.x | 2 +-
src/test/test_driver.c| 2 +-
src/vz/vz_driver.c| 2 +-
6 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index 0c9985f7f4..2532b99c58 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -78,6 +78,10 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr
domain,
const char *xmlDesc,
unsigned int flags);
+typedef enum {
+VIR_DOMAIN_SNAPSHOT_XML_SECURE = VIR_DOMAIN_XML_SECURE, /* dump
security sensitive information too */
+} virDomainSnapshotXMLFlags;
+
/* Dump the XML of a snapshot */
char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
unsigned int flags);
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 100326a5e7..a724c66421 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -244,14 +244,13 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
/**
* virDomainSnapshotGetXMLDesc:
* @snapshot: a domain snapshot object
- * @flags: bitwise-OR of subset of virDomainXMLFlags
+ * @flags: bitwise-OR of supported virDomainSnapshotXMLFlags
*
* Provide an XML description of the domain snapshot.
*
* No security-sensitive data will be included unless @flags contains
- * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
- * connections. For this API, @flags should not contain either
- * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
+ * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only
+ * connections.
*
* Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error.
* the caller must free() the returned value.
@@ -268,7 +267,7 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
virCheckDomainSnapshotReturn(snapshot, NULL);
conn = snapshot->domain->conn;
-if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) {
+if ((conn->flags & VIR_CONNECT_RO) && (flags &
VIR_DOMAIN_SNAPSHOT_XML_SECURE)) {
virReportError(VIR_ERR_OPERATION_DENIED, "%s",
_("virDomainSnapshotGetXMLDesc with secure flag"));
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 40bd24cdf2..a4c169f71c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16251,7 +16251,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr
snapshot,
virDomainSnapshotObjPtr snap = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
-virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+virCheckFlags(VIR_DOMAIN_SNAPSHOT_XML_SECURE, NULL);
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
return NULL;
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 42a87d418b..60cc40e04a 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -4902,7 +4902,7 @@ enum remote_procedure {
* @generate: both
* @priority: high
* @acl: domain:read
- * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE
+ * @acl: domain:read_secure:VIR_DOMAIN_SNAPSHOT_XML_SECURE
*/
REMOTE_PROC_DOMAIN_SNAPSHOT_GET_XML_DESC = 186,
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index c1faff46ff..155ebc00d6 100644
--- a/src/test/test_driver.c
+++
[libvirt] [PATCH 0/3] virDomainXMLFlags cleanups
Nir has asked that my upcoming checkpoint APIs have a way to do bulk operations: grab the XML for ALL checkpoints in one call, and in turn redefine checkpoints on a new host using the dumped XML from an old host in one call. But since checkpoints borrow heavily from the APIs used for snapshots, it makes sense to do the same thing for checkpoints. And in the process of implementing that code, I discovered some oddities in the use of flags when dumping domain-related XML, which this series aims to fix. Eric Blake (3): domain: Document VIR_DOMAIN_XML_MIGRATABLE domain: Define explicit flags for saved image xml snapshot: Define explicit flags for snapshot xml include/libvirt/libvirt-domain-snapshot.h | 4 include/libvirt/libvirt-domain.h | 5 + src/libvirt-domain-snapshot.c | 9 - src/libvirt-domain.c | 22 +- src/qemu/qemu_driver.c| 6 +++--- src/remote/remote_protocol.x | 6 +++--- src/test/test_driver.c| 2 +- src/vz/vz_driver.c| 2 +- 8 files changed, 34 insertions(+), 22 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] domain: Document VIR_DOMAIN_XML_MIGRATABLE
Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but failed to document its effects. Signed-off-by: Eric Blake --- src/libvirt-domain.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 54ca18f249..6158382d07 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2559,7 +2559,13 @@ virDomainGetControlInfo(virDomainPtr domain, * currently running domain. If @flags contains * VIR_DOMAIN_XML_UPDATE_CPU, then the portion of the domain XML * describing CPU capabilities is modified to match actual - * capabilities of the host. + * capabilities of the host. If @flags contains VIR_DOMAIN_XML_MIGRATABLE, + * the XML is altered to trim redundant information that might interfere + * with migration to an older version of libvirt, as well as expose additional + * information internal to libvirt; this flag is rejected on read-only + * connections, and the resulting XML might not validate against the schema, + * but it can serve as a starting point for custom XML in calls such as + * virDomainMigrate2(). * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Escape external snapshot names containing comma
On 2/13/19 3:58 PM, John Ferlan wrote: > > > On 2/12/19 11:33 PM, Eric Blake wrote: >> The code for creating external snapshots for an offline domain >> called out to qemu-img without escaping commas in the manner >> that qemu-img expects. This also fixes a typo in the comment. >> >> Signed-off-by: Eric Blake >> --- >> >> Noticed by code inspection; I did not try very hard to see how >> easy or hard it would be to convince libvirt to actually try >> and create an external snapshot to /path/file,with,comma to >> see how things break. >> >> src/qemu/qemu_driver.c | 15 ++- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> > > Oh joy another place that uses qemu-img... > > Looks like qemuDomainSnapshotForEachQcow2Raw uses virDomainDiskGetSource > or disk def->src->path as well for qemuimgarg[4]. Urgh, so it does. And it uses virRun() instead of the nicer virCommand... API :( I'll save that for a separate patch, > > Reviewed-by: John Ferlan and have pushed this one. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Escape external snapshot names containing comma
On 2/12/19 11:33 PM, Eric Blake wrote: > The code for creating external snapshots for an offline domain > called out to qemu-img without escaping commas in the manner > that qemu-img expects. This also fixes a typo in the comment. > > Signed-off-by: Eric Blake > --- > > Noticed by code inspection; I did not try very hard to see how > easy or hard it would be to convince libvirt to actually try > and create an external snapshot to /path/file,with,comma to > see how things break. > > src/qemu/qemu_driver.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > Oh joy another place that uses qemu-img... Looks like qemuDomainSnapshotForEachQcow2Raw uses virDomainDiskGetSource or disk def->src->path as well for qemuimgarg[4]. Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] virsh: Add options for parallel migration
On 2/8/19 10:08 AM, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark
> ---
> tools/virsh-domain.c | 19 +++
> tools/virsh.pod | 7 +++
> 2 files changed, 26 insertions(+)
>
Should supplying parallel-connections imply parallel since the previous
patch would fail:
+if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set &&
+!(flags & VIR_MIGRATE_PARALLEL)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Turn parallel migration on to tune it"));
+goto error;
+}
+
either that or make parallel-connections be an optional parameter to
--parallel using VSH_OFLAG_EMPTY_OK and converting the optionally read
string into an int.
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 8b20059335..c704faf7e1 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10561,6 +10561,14 @@ static const vshCmdOptDef opts_migrate[] = {
> .type = VSH_OT_INT,
> .help = N_("post-copy migration bandwidth limit in MiB/s")
> },
> +{.name = "parallel",
> + .type = VSH_OT_BOOL,
> + .help = N_("enable parallel migration")
> +},
> +{.name = "parallel-connections",
> + .type = VSH_OT_INT,
> + .help = N_("number of connections for parallel migration")
> +},
> {.name = NULL}
> };
>
> @@ -10766,6 +10774,14 @@ doMigrate(void *opaque)
> goto save_error;
> }
>
> +if (vshCommandOptInt(ctl, cmd, "parallel-connections", ) < 0)
> +goto out;
> +if (intOpt &&
> +virTypedParamsAddInt(, , ,
> + VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS,
> + intOpt) < 0)
> +goto save_error;
> +
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_MIGRATE_LIVE;
> if (vshCommandOptBool(cmd, "p2p"))
> @@ -10814,6 +10830,9 @@ doMigrate(void *opaque)
> if (vshCommandOptBool(cmd, "tls"))
> flags |= VIR_MIGRATE_TLS;
>
> +if (vshCommandOptBool(cmd, "parallel"))
> +flags |= VIR_MIGRATE_PARALLEL;
> +
> if (flags & VIR_MIGRATE_PEER2PEER || vshCommandOptBool(cmd, "direct")) {
> if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) ==
> 0)
> ret = '0';
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 67edb57b14..7604c7536e 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1904,6 +1904,7 @@ I I [I] [I]
> [I] [I [I<--comp-xbzrle-cache>] [I<--auto-converge>] [I]
> [I] [I<--persistent-xml> B] [I<--tls>]
> [I<--postcopy-bandwidth> B]
> +[I<--parallel> [I B]]
--parallel-connections
>
> Migrate domain to another host. Add I<--live> for live migration; <--p2p>
> for peer-2-peer migration; I<--direct> for direct migration; or
> I<--tunnelled>
> @@ -1997,6 +1998,12 @@ Providing I<--tls> causes the migration to use the
> host configured TLS setup
> the migration of the domain. Usage requires proper TLS setup for both source
> and target.
>
> +I<--parallel> option will cause migration data to be sent over multiple
> +parallel connections. The number of such connections can be set using
> +I. Parallel connections may help with saturating the
--parallel-connections
> +network link between the source and the target and thus speeding up the
> +migration.
> +
[although too many could really be a problem ;-)]
What's here is fine If you want to alter to have one option with an
optional parameter, then just repost this patch.
Reviewed-by: John Ferlan
John
> Running migration can be canceled by interrupting virsh (usually using
> C) or by B command sent from another virsh instance.
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/3] qemu: Add support for parallel migration
On 2/8/19 10:08 AM, Jiri Denemark wrote: > The VIR_MIGRATE_PARALLEL flag is implemented using QEMU's multifd > migration capability and the corresponding multifd-channels migration > parameter. > > Signed-off-by: Jiri Denemark > --- > > Notes: > QEMU still uses the x- prefix for multifd capability and multifd-channels, > but Juan already sent a series of patches to make multifd migration fully > supported by dropping the experimental prefix. > > src/qemu/qemu_migration.c| 9 ++--- > src/qemu/qemu_migration.h| 5 - > src/qemu/qemu_migration_params.c | 18 ++ > src/qemu/qemu_migration_params.h | 2 ++ > 4 files changed, 30 insertions(+), 4 deletions(-) > Standard practice is to wait for the x-* to hit the qemu.git tree before we push this... Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] Public API for parallel migration
On 2/8/19 10:08 AM, Jiri Denemark wrote: > This patch adds a new VIR_MIGRATE_PARALLEL flag for migration APIs which > will ask the hypervisor to use multiple parallel connections for > migrating a domain. The number of parallel connections can be set using > VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS typed parameter. > > Signed-off-by: Jiri Denemark > --- > include/libvirt/libvirt-domain.h | 14 ++ > 1 file changed, 14 insertions(+) > Shall I assume the multifd-page-count is not going to be implemented? or something for the future? Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 07/10] lcitool: avoid installing recommended packages
We know exactly which packages we need and don't want apt
picking extra "recommended" ones for us.
Reviewed-by: Andrea Bolognani
Signed-off-by: Daniel P. Berrangé
---
guests/lcitool | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/guests/lcitool b/guests/lcitool
index a7bcae3..1271954 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -544,7 +544,7 @@ class Application:
RUN export DEBIAN_FRONTEND=noninteractive && \\
apt-get update && \\
apt-get dist-upgrade -y && \\
-apt-get install -y ${PACKAGES} && \\
+apt-get install --no-install-recommends -y ${PACKAGES} &&
\\
apt-get autoremove -y && \\
apt-get autoclean -y
"""))
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 08/10] lcitool: refactor logic for building package list
Signed-off-by: Daniel P. Berrangé
---
guests/lcitool | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 1271954..0978c40 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -529,15 +529,18 @@ class Application:
if os_full in mappings[package]:
temp[package] = mappings[package][os_full]
-flattened = []
+pkgs = []
for item in temp:
-if temp[item] is not None and temp[item] not in flattened:
-flattened += [temp[item]]
+pkgname = temp[item]
+if pkgname is None:
+continue
+if pkgname not in pkgs:
+pkgs.append(pkgname)
print("FROM {}".format(facts["docker_base"]))
sys.stdout.write("ENV PACKAGES ")
-sys.stdout.write(" \\\n ".join(sorted(flattened)))
+sys.stdout.write(" \\\n ".join(sorted(pkgs)))
if package_format == "deb":
sys.stdout.write(textwrap.dedent("""
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 09/10] lcitool: avoid using an env var to store package list
Every statement in a dockerfile results in a new layer in the
image. There is no need for an env var to store the package list
when it can be included inline. This avoids the env variable being
later exposed to the container at runtime.
Signed-off-by: Daniel P. Berrangé
---
guests/lcitool | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 0978c40..8252dc2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -539,34 +539,34 @@ class Application:
print("FROM {}".format(facts["docker_base"]))
-sys.stdout.write("ENV PACKAGES ")
-sys.stdout.write(" \\\n ".join(sorted(pkgs)))
-
+varmap = {}
+varmap["pkgs"] = " \\\n".join(sorted(pkgs))
if package_format == "deb":
sys.stdout.write(textwrap.dedent("""
RUN export DEBIAN_FRONTEND=noninteractive && \\
apt-get update && \\
apt-get dist-upgrade -y && \\
-apt-get install --no-install-recommends -y ${PACKAGES} &&
\\
+apt-get install --no-install-recommends -y \\
+{pkgs} && \\
apt-get autoremove -y && \\
apt-get autoclean -y
-"""))
+""").format(**varmap))
elif package_format == "rpm":
if os_name == "Fedora" and os_version == "Rawhide":
sys.stdout.write(textwrap.dedent("""
RUN yum update -y --nogpgcheck fedora-gpg-keys && \\
yum update -y && \\
-yum install -y ${PACKAGES} && \\
+yum install -y %(pkgs)s && \\
yum autoremove -y && \\
yum clean all -y
-"""))
+""").format(**varmap))
else:
sys.stdout.write(textwrap.dedent("""
RUN yum update -y && \\
-yum install -y ${PACKAGES} && \\
+yum install -y %(pkgs)s && \\
yum autoremove -y && \\
yum clean all -y
-"""))
+""").format(**varmap))
def run(self):
cmdline = self._parser.parse_args()
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 05/10] lcitool: include root cause when reporting errors
The root cause exception contains the useful information about what
really failed during loading of some resource, or running of a
command.
Signed-off-by: Daniel P. Berrangé
---
guests/lcitool | 54 +-
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 759eff6..bd32d1f 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -95,10 +95,10 @@ class Config:
if not os.path.exists(config_dir):
try:
os.mkdir(config_dir)
-except Exception:
+except Exception as ex:
raise Error(
-"Can't create configuration directory ({})".format(
-config_dir,
+"Can't create configuration directory ({}): {}".format(
+config_dir, ex,
)
)
@@ -117,10 +117,10 @@ class Config:
try:
with open(flavor_file, "w") as infile:
infile.write("{}\n".format(flavor))
-except Exception:
+except Exception as ex:
raise Error(
-"Can't write flavor file ({})".format(
-flavor_file,
+"Can't write flavor file ({}): {}".format(
+flavor_file, ex
)
)
@@ -141,10 +141,10 @@ class Config:
with open(vault_pass_file, "r") as infile:
if not infile.readline().strip():
raise ValueError
-except Exception:
+except Exception as ex:
raise Error(
-"Missing or invalid vault password file ({})".format(
-vault_pass_file,
+"Missing or invalid vault password file ({}): {}".format(
+vault_pass_file, ex
)
)
@@ -157,10 +157,10 @@ class Config:
with open(root_pass_file, "r") as infile:
if not infile.readline().strip():
raise ValueError
-except Exception:
+except Exception as ex:
raise Error(
-"Missing or invalid root password file ({})".format(
-root_pass_file,
+"Missing or invalid root password file ({}): {}".format(
+root_pass_file, ex
)
)
@@ -177,8 +177,8 @@ class Inventory:
parser = configparser.SafeConfigParser()
parser.read(ansible_cfg_path)
inventory_path = parser.get("defaults", "inventory")
-except Exception:
-raise Error("Can't find inventory location in ansible.cfg")
+except Exception as ex:
+raise Error("Can't read inventory location in ansible.cfg:
{}".format(ex))
inventory_path = os.path.join(base, inventory_path)
@@ -191,10 +191,10 @@ class Inventory:
for line in infile:
host = line.strip()
self._facts[host] = {}
-except Exception:
+except Exception as ex:
raise Error(
-"Missing or invalid inventory ({})".format(
-inventory_path,
+"Missing or invalid inventory ({}): {}".format(
+inventory_path, ex
)
)
@@ -202,8 +202,8 @@ class Inventory:
try:
self._facts[host] = self._read_all_facts(host)
self._facts[host]["inventory_hostname"] = host
-except Exception:
-raise Error("Can't load facts for '{}'".format(host))
+except Exception as ex:
+raise Error("Can't load facts for '{}': {}".format(host, ex))
@staticmethod
def _add_facts_from_file(facts, yaml_path):
@@ -254,8 +254,8 @@ class Projects:
with open(mappings_path, "r") as infile:
mappings = yaml.load(infile)
self._mappings = mappings["mappings"]
-except Exception:
-raise Error("Can't load mappings")
+except Exception as ex:
+raise Error("Can't load mappings: {}".format(ex))
source = os.path.join(base, "vars", "projects")
@@ -273,8 +273,8 @@ class Projects:
with open(yaml_path, "r") as infile:
packages = yaml.load(infile)
self._packages[project] = packages["packages"]
-except Exception:
-raise Error("Can't load packages for '{}'".format(project))
+except Exception as ex:
+raise Error("Can't load packages for '{}': {}".format(project,
ex))
def expand_pattern(self, pattern):
projects = Util.expand_pattern(pattern, self._packages, "project")
@@ -398,8 +398,8 @@ class Application:
[libvirt] [jenkins-ci PATCH v3 10/10] lcitool: support generating cross compiler dockerfiles
Debian's filesystem layout has a nice advantage over Fedora which is that it can install non-native RPMs in the main root filesystem. It is thus possible to prepare an x86_64 filesystem containing -dev packages for a foreign architecture, along with a GCC cross compiler. QEMU has used this technique to facilitate developer build testing of non-x86 architectures, since few people have access to physical hardware for most of these architectures. For the same reason it would be helpful to libvirt developers. This patch extends the 'dockerfile' command to 'lcitool' so that it accepts a '-x $ARCH' argument. $ lcitool -a dockerfile -h libvirt-debian-9 -p libvirt -x s390x This is only valid when using a 'deb' based distro. With the Debian 9 distro, this supports arm64, armel, armhf, mips, mipsel, mips64el, ppc64el, s390x, which are all the official archs that Debian maintainers currently build libvirt for. With Debian Sid, this is extended to include i386. Debian also builds libvirt on hppa, powerpcspe, sparc64 and x32 which are unofficial ports. Unfortunately it is not possible to reliably add foreign arch packages from the unofficial ports in parallel with those from the native arch, without hitting dependency problems from apt. When an architecture is given, any package name ending in '-dev' will be installed using that architecture variant, while all remaining packages will have their native variant installed. For various reasons (commented inline) a few blacklists are required on a per-arch basis. Signed-off-by: Daniel P. Berrangé --- .../libvirt-debian-9/cross-build.yml | 50 ++ .../libvirt-debian-sid/cross-build.yml| 52 +++ guests/lcitool| 66 --- 3 files changed, 159 insertions(+), 9 deletions(-) create mode 100644 guests/host_vars/libvirt-debian-9/cross-build.yml create mode 100644 guests/host_vars/libvirt-debian-sid/cross-build.yml diff --git a/guests/host_vars/libvirt-debian-9/cross-build.yml b/guests/host_vars/libvirt-debian-9/cross-build.yml new file mode 100644 index 000..9c6e511 --- /dev/null +++ b/guests/host_vars/libvirt-debian-9/cross-build.yml @@ -0,0 +1,50 @@ +--- +cross_build: + blacklist: +# apt doesn't properly resolve from foreign arch package +# to the native package of augeas-lenses +- libnetcf-dev +# apt fails to resolve deps from foreign arch +- wireshark-dev +# dtrace tool doesn't know how to cross-compile +- systemtap-sdt-dev + arches: +arm64: + target: aarch64-linux-gnu +armel: + target: arm-linux-gnueabi + blacklist: +# Not built on this arch +- libxen-dev +# Arch does not support NUMA concept +- libnuma-dev +armhf: + target: arm-linux-gnueabihf + blacklist: +# Arch does not support NUMA concept +- libnuma-dev +mips64el: + target: mips64el-linux-gnuabi64 + blacklist: +# Not built on this arch +- libxen-dev +mips: + target: mips-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +mipsel: + target: mipsel-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +ppc64el: + target: powerpc64le-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +s390x: + target: s390x-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev diff --git a/guests/host_vars/libvirt-debian-sid/cross-build.yml b/guests/host_vars/libvirt-debian-sid/cross-build.yml new file mode 100644 index 000..7184083 --- /dev/null +++ b/guests/host_vars/libvirt-debian-sid/cross-build.yml @@ -0,0 +1,52 @@ +--- +cross_build: + blacklist: +# apt doesn't properly resolve from foreign arch package +# to the native package of augeas-lenses +- libnetcf-dev +# apt fails to resolve deps from foreign arch +- wireshark-dev +# dtrace tool doesn't know how to cross-compile +- systemtap-sdt-dev + arches: +arm64: + target: aarch64-linux-gnu +armel: + target: arm-linux-gnueabi + blacklist: +# Not built on this arch +- libxen-dev +# Arch does not support NUMA concept +- libnuma-dev +armhf: + target: arm-linux-gnueabihf + blacklist: +# Arch does not support NUMA concept +- libnuma-dev +mips64el: + target: mips64el-linux-gnuabi64 + blacklist: +# Not built on this arch +- libxen-dev +mips: + target: mips-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +mipsel: + target: mipsel-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +ppc64el: + target: powerpc64le-linux-gnu + blacklist: +# Not built on this arch +- libxen-dev +s390x: + target: s390x-linux-gnu +
[libvirt] [jenkins-ci PATCH v3 00/10] Add support for cross compiling libvirt via Debian
Changed in v3: - Remove sheepdog more generally - Use .format() style printf - Split config to cross-build.yml - Make glusterfs name per-distro customized - Misc code style changes - Rename fields in cross-build.yml - Don't use crossbuild-essential packages Changed in v2: - Fix multiple package name mistakes - Modify lcitool to generate cross-arch docker files - Add --no-install-recommended flag to apt-get - Add DEBIAN_FRONTEND=noninteractive env to apt-get - Improve error reporting in lcitool - Add make rule for generating dockerfiles locally Daniel P. Berrangé (10): guests: use libpcap0.8-dev package on Debian guests: add xfsprogs development package for libvirt guests: fix glusterfs package name on Debian guests: Debian SID has dropped the sheepdog package lcitool: include root cause when reporting errors lcitool: force non-interactive apt-get frontend lcitool: avoid installing recommended packages lcitool: refactor logic for building package list lcitool: avoid using an env var to store package list lcitool: support generating cross compiler dockerfiles .../libvirt-debian-9/cross-build.yml | 50 ++ .../libvirt-debian-sid/cross-build.yml| 52 +++ guests/lcitool| 146 -- guests/vars/mappings.yml | 13 +- guests/vars/projects/libvirt.yml | 1 + 5 files changed, 213 insertions(+), 49 deletions(-) create mode 100644 guests/host_vars/libvirt-debian-9/cross-build.yml create mode 100644 guests/host_vars/libvirt-debian-sid/cross-build.yml -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 01/10] guests: use libpcap0.8-dev package on Debian
The libpcap-dev package is a temporary backcompat package until everything switches to the new libpcap0.8-dev pacakge name. Reviewed-by: Andrea Bolognani Signed-off-by: Daniel P. Berrangé --- guests/vars/mappings.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index 84543c4..f211169 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -281,7 +281,7 @@ mappings: rpm: parted-devel libpcap: -deb: libpcap-dev +deb: libpcap0.8-dev pkg: libpcap rpm: libpcap-devel -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 03/10] guests: fix glusterfs package name on Debian
We want the development headers not the client binary Reviewed-by: Andrea Bolognani Signed-off-by: Daniel P. Berrangé --- guests/vars/mappings.yml | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index e10e3da..0945e36 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -141,8 +141,12 @@ mappings: rpm: glibc-static glusterfs: -deb: glusterfs-client +deb: libglusterfs-dev rpm: glusterfs-api-devel +Debian8: glusterfs-common +Debian9: glusterfs-common +Ubuntu16: glusterfs-common +Ubuntu18: glusterfs-common gnome-common: default: gnome-common -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 06/10] lcitool: force non-interactive apt-get frontend
Signed-off-by: Daniel P. Berrangé
---
guests/lcitool | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/guests/lcitool b/guests/lcitool
index bd32d1f..a7bcae3 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -541,7 +541,8 @@ class Application:
if package_format == "deb":
sys.stdout.write(textwrap.dedent("""
-RUN apt-get update && \\
+RUN export DEBIAN_FRONTEND=noninteractive && \\
+apt-get update && \\
apt-get dist-upgrade -y && \\
apt-get install -y ${PACKAGES} && \\
apt-get autoremove -y && \\
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 04/10] guests: Debian SID has dropped the sheepdog package
Signed-off-by: Daniel P. Berrangé --- guests/vars/mappings.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index 0945e36..f31b460 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -774,6 +774,7 @@ mappings: default: sheepdog CentOS: FreeBSD: +DebianSid: showmount: deb: nfs-common -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v3 02/10] guests: add xfsprogs development package for libvirt
Reviewed-by: Andrea Bolognani Signed-off-by: Daniel P. Berrangé --- guests/vars/mappings.yml | 4 guests/vars/projects/libvirt.yml | 1 + 2 files changed, 5 insertions(+) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index f211169..e10e3da 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -815,6 +815,10 @@ mappings: deb: libxen-dev Fedora: xen-devel + xfsprogs: +deb: xfslibs-dev +rpm: xfsprogs-devel + xmllint: default: libxml2 deb: libxml2-utils diff --git a/guests/vars/projects/libvirt.yml b/guests/vars/projects/libvirt.yml index 605d201..b19d91f 100644 --- a/guests/vars/projects/libvirt.yml +++ b/guests/vars/projects/libvirt.yml @@ -53,6 +53,7 @@ packages: - tc - wireshark - xen + - xfsprogs - xmllint - xsltproc - yajl -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v2 8/9] lcitool: support generating cross compiler dockerfiles
On Thu, Feb 07, 2019 at 04:21:34PM +0100, Andrea Bolognani wrote: > On Tue, 2019-02-05 at 17:53 +, Daniel P. Berrangé wrote: > [...] > > With the Debian 9 distro, this supports arm64, armel, armhf, mips, > > mipsel, mips64el, ppc64el, s390x, which are all the official archs > > that Debian maintainers currently build libvirt for. With Debian Sid, > > this is extended to include i386. > > Is i386 really not supported as a cross compilation target on Debian > 9? It seems like it would be such a low hanging fruit... Yes, the i386 gcc simply doesn't exist in 9. No idea why > [...] > > +++ b/guests/host_vars/libvirt-debian-9/docker.yml > > @@ -1,2 +1,59 @@ > > --- > > docker_base: debian:9 > > +cross_build: > > I don't think this belongs in docker.yml, since there's nothing > specific to Docker here: we could just as well use this information > to build a virtual machine in which to perform cross builds. > > I would create a new fact file, eg. cross-build.yml, for this. Ok. In practice this split doesn't seem to actually do anything since we load all files & take the union of their contents. > > + blacklist: > > +# Doesn't properly resolve from foreign arch package > > +# to the native package of augeas-lenses > > I cannot really understand what you're trying to say with this > comment, can you try to reword it please? dpkg fails to resolve the deps. It wants augeas-lenses:s390x when it should be satisfied with augeas-lenses:$NATIVE. You can't install both augeas-lenses as they conflict on files. > > +- libnetcf-dev > > +# Fails to resolve deps from foreign arch > > +- wireshark-dev > > +# dtrace tool doesn't know how to cross-compile > > +- systemtap-sdt-dev > > For these and all other blacklisted packages, you should list the > name of the mapping (eg. netcf) rather than the concrete Debian > package name (eg. libnetcf-dev). Then of course you'll have to act > on the blacklist earlier, before mapping is performed. I don't find that desirable. This is a debian specific config file, and it is clearer to list the actual debian packages we have trouble with, as that's what apt-get is complaining about when it fails. Using the generic mapping obscures what is going on. > > + arches: > > +arm64: > > + gcc-pkg-prefix: crossbuild-essential-arm64 > > This is not a prefix, and the package is not GCC :) > > I would use 'cross_gcc', which incidentally is also the name of the > variable you use in the Python script to store this value. (More on > this later, though.) Actually this entire variable can be eliminated, as we can derive it from the target name. > > > + target-prefix: aarch64-linux-gnu > > Same here, you later use 'cross_prefix' in the script but also store > it as 'TARGET' in the environment... I'd suggest 'cross_target'. Using "cross_" is redundant as this already inside a parent "cross-build" block. > [...] > > +mipsel: > > + gcc-pkg-prefix: gcc-mipsel-linux-gnu > > crossbuild-essential-mipsel is available in Debian 9, so you should > use that instead. Actually, I've eliminated all usage of the crossbuild-essential-* packages. These needlessly pull in the C++ compiler too. > > docker_base: debian:sid > > +cross_build: > > + blacklist: > > +# Doesn't properly resolve from foreign arch package > > +# to the native package of augeas-lenses > > +- libnetcf-dev > > +# Fails to resolve deps from foreign arch > > +- wireshark-dev > > +# dtrace tool doesn't know how to cross-compile > > +- systemtap-sdt-dev > > +# appears to be dropped from the 'sid' tree > > +- sheepdog > > So it has! And it's apparently not going to be in the next Ubuntu > release, either. What's missing that we've not seen this problem already ? Is it just because we don't regularlyl rebuild the CI hosts / images ? > > +if cross_arch and pkgname[-4:] == "-dev": > > +if pkgname not in cross_pkgs: > > +cross_pkgs.append(pkgname + ":" + cross_arch) > > I can see how sticking with the Debian-style architecture names > makes this bit trivial, but I'm still entirely convinced we should > use the same architecture names as libvirt does. > Especially once we'll have integrated this into libvirt's own build > system, the difference in names would introduce needless friction, > as the target audience (libvirt developers) is certainly more > familiar with the architecture names used in that project than > Debian's. Using the libvirt arch names adds no value. There's no friction as there is no context in which we're using the libvirt arch names & need a mapping to the cross build names. These image configs are distro specific and using the distro's native arch is directly relevant to the build results. > > +ENV TARGET "%(cross_prefix)s" > > +ENV CONFIGURE_OPTS "--host=%(cross_prefix)s > > --target=%(cross_prefix)s" > > +
Re: [libvirt] [jenkins-ci PATCH v2 5/9] lcitool: force non-interactive apt-get frontend
On Wed, Feb 06, 2019 at 05:21:36PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-02-05 at 17:53 +, Daniel P. Berrangé wrote:
> [...]
> > if package_format == "deb":
> > sys.stdout.write(textwrap.dedent("""
> > -RUN apt-get update && \\
> > -apt-get dist-upgrade -y && \\
> > -apt-get install -y ${PACKAGES} && \\
> > -apt-get autoremove -y && \\
> > -apt-get autoclean -y
> > +RUN DEBIAN_FRONTEND=noninteractive && \\
> > +( \\
> > +apt-get update && \\
> > +apt-get dist-upgrade -y && \\
> > +apt-get install -y ${PACKAGES} && \\
> > +apt-get autoremove -y && \\
> > +apt-get autoclean -y \\
> > +)
>
> Wouldn't
>
> RUN export DEBIAN_FRONTEND=noninteractive && \\
> apt-get update && \\
> ...
>
> work as well, without requiring the extra indentation (and shell)?
Oh, true, I could have done that
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 RESEND] openvswitch: Add new port VLAN mode "dot1q-tunnel"(802.1ad double-tagged)
On 12/11/18 7:26 AM, luzhip...@uniudc.com wrote: > From: ZhiPeng Lu > > This patch adds functionality to allow libvirt to configure the 'dot1q-tunnel' > modes(802.1ad double-tagged) on openvswitch networks. > For example: > > > > > > > > > > > > > > > Signed-off-by: ZhiPeng Lu > --- > v1->v2: >1. Fix "make syntax-check" failure > v2->v3: >1. remove other_config when updating vlan > v3->v4: >1. add commit message that has a brief description of the new > feature >2. add tests for 'dot1q-tunnel' vlan mode > Seems this was lost/dropped over the holidays. In any case it doesn't apply to current top of tree, can you update and resend please. Hopefully this time it won't get lost and apologies in advance for not addressing in a more timely manner. Tks, John > docs/formatdomain.html.in| 20 > ++-- > docs/formatnetwork.html.in | 20 > +++- > docs/schemas/networkcommon.rng | 1 + > src/conf/netdev_vlan_conf.c | 2 +- > src/util/virnetdevopenvswitch.c | 7 +++ > src/util/virnetdevvlan.h | 1 + > tests/networkxml2xmlin/openvswitch-net.xml | 9 + > tests/networkxml2xmlout/openvswitch-net.xml | 9 + > .../openvswitch-net-modified.xml | 9 + > .../openvswitch-net-more-portgroups.xml | 9 + > .../openvswitch-net-without-alice.xml| 9 + > 11 files changed, 80 insertions(+), 16 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 84259c4..ced21c0 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -6085,8 +6085,14 @@ qemu-kvm -net nic,model=? /dev/null >tag id='42'/ >tag id='123' nativeMode='untagged'/ > /vlan > -... >/interface > + interface type='bridge' > +vlan trunk='yes' > + tag id='42'/ > + tag id='123' nativeMode='dot1q-tunnel'/ > +/vlan > + /interface > +... > /devices > ... > > @@ -6122,11 +6128,13 @@ qemu-kvm -net nic,model=? /dev/null > > >For network connections using Open vSwitch it is also possible > - to configure 'native-tagged' and 'native-untagged' VLAN modes > - Since 1.1.0. This is done with the > - optional nativeMode attribute on > - the tag subelement: nativeMode > - may be set to 'tagged' or 'untagged'. The id > + to configure 'native-tagged' and 'native-untagged' > + Since 1.1.0. and 'dot1q-tunnel' > + (802.1ad double-tagged) Since 5.0.0. > + VLAN modes This is done with the optional nativeMode > + attribute on the tag subelement: > + nativeMode may be set to 'tagged' or 'untagged' or > + 'dot1q-tunnel'. The id >attribute of the tag subelement >containing nativeMode sets which VLAN is considered >to be the "native" VLAN for this interface, and > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 363a72b..d9ac9f7 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -688,16 +688,18 @@ > > >For network connections using Open vSwitch it is also possible > - to configure 'native-tagged' and 'native-untagged' VLAN modes > - Since 1.1.0. This is done with the > - optional nativeMode attribute on > - the tag subelement: nativeMode > - may be set to 'tagged' or 'untagged'. The id > - attribute of the tag subelement > - containing nativeMode sets which VLAN is considered > - to be the "native" VLAN for this interface, and > + to configure 'native-tagged' and 'native-untagged' > + Since 1.1.0. > + and 'dot1q-tunnel'(802.1ad double-tagged) VLAN modes. > + Since 5.0.0. This is done with the > + optional nativeMode attribute on the > + tag subelement: nativeMode > + may be set to 'tagged' or 'untagged' or 'dot1q-tunnel'. > + The id attribute of the tag > + subelement containing nativeMode sets which VLAN is > + considered to be the "native" VLAN for this interface, and >the nativeMode attribute determines whether or not > - traffic for that VLAN will be tagged. > + traffic for that VLAN will be tagged or QinQ. > > >vlan elements can also be specified in > diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng > index 2699555..11c48ff 100644 > --- a/docs/schemas/networkcommon.rng > +++ b/docs/schemas/networkcommon.rng > @@ -223,6 +223,7 @@ > > tagged > untagged > +dot1q-tunnel > > > > diff --git a/src/conf/netdev_vlan_conf.c
Re: [libvirt] [PATCH v2 0/3] qemu: don't duplicate suspended events and state changes
On 2/8/19 2:52 AM, Nikolay Shirokovskiy wrote: > Patches 1 and 2 are already Reviewed-by: John. Patch 3 needs Peter comments. > Right - feel free to add my : Reviewed-by: John Ferlan to the first 2 patches for sure. To help push this along, Peter is again CC'd and of importance is the v1 review of patch3: https://www.redhat.com/archives/libvir-list/2018-October/msg00831.html where I noted a specific commit and case to be considered. John > Diff from v1: > > - minor rebase changes > - minor changes according to review > > [1] PATCH v1 : > https://www.redhat.com/archives/libvir-list/2018-October/msg00591.html > > Nikolay Shirokovskiy (3): > qemu: Pass stop reason from qemuProcessStopCPUs to stop handler > qemu: Map suspended state reason to suspended event detail > qemu: Don't duplicate suspend events and state changes > > src/qemu/qemu_domain.c| 34 ++ > src/qemu/qemu_domain.h| 7 +++ > src/qemu/qemu_driver.c| 26 +++--- > src/qemu/qemu_migration.c | 42 ++ > src/qemu/qemu_migration.h | 4 > src/qemu/qemu_process.c | 38 +- > 6 files changed, 75 insertions(+), 76 deletions(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del
[...]
>>
>> In testing with your patches, I did find I could only do at most one
>> attach/detach cycle - a second attach for a running guest results in:
>>
>> error: internal error: unable to execute QEMU command 'chardev-add':
>> attempt to add duplicate property 'charchannel0' to object (type
>> 'container')
>
> Interesting, this test scenario works for me just fine. What's your qemu
> version? Mine's v3.1.0-1709-g0b5e750bea; Looks like 3.1.0 is broken - it
> does not removes the chardev. And running bisect shows this was changed
> in e47f81b617684c4546af286d307b69014a83538a (merged Feb 7). If I use
> unfixed qemu, the chardev is not removed on netdev_del, but trying to
> remove it manually fails too:
>
> {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' is
> busy"}}
>
>
> So if users are using any qemu but the one from git, no matter what
> libvirt does it won't help them. Might as well go with 2/5 and require
> freshly start domain.
>
Well now... Nice detective work. So I had v3.0.0 in my (new) f29 laptop
- I hadn't recently done a build from qemu source and run (explains an
error on another guest I had where I was using a built qemu tree based
on the 2.12 branch and the guest wasn't starting).
So using a top of tree QEMU allows things to succeed now for me.
Probably want to be sure to update the bz with what you found so that
the history isn't lost and to note the problem for QE so that they don't
file a bug against libvirt on it ;-)...
So let's think about this series again...
Patch1 - I think perhaps it could be replaced by removing @devstr since
it's really not even used. It could also be a follow-up, IDC. If you go
with the former, taken an implied R-by since @devstr doesn't need to be
formulated.
Then for Patch2 and Patch4 - you have my Reviewed-by: John Ferlan
for the patches as is.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: fix memory leak in virFirewallDInterfaceSetZone()
On Wed, 2019-02-13 at 11:01 -0500, Laine Stump wrote: > commit 3bba4825 added the new function virFirewallDInterfaceSetZone() > which calledsends virDBUSCallMethod a DBusMessage** for the reply > message, but doesn't use the reply, and also doesn't free it. Since > this arg is allowed to be NULL, this patch simply sets it to NULL so > we don't have to deal with it. > > Signed-off-by: Laine Stump > --- > src/util/virfirewalld.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
On Wed, 2019-02-13 at 13:29 +0100, Ján Tomko wrote: > On Wed, Feb 13, 2019 at 10:59:28AM +0100, Andrea Bolognani wrote: > > On Wed, 2019-02-13 at 10:03 +0100, Ján Tomko wrote: > > > Also, the device will be given an iobase by QEMU, we should represent > > > that in the XML and fill in that default. > > > > If we can manage to implement it in a way that's both reliable and > > backwards compatible, then I'm absolutely in favor of doing that! The > > current behavior is clearly not great. > > We cannot if any proposal for improvement is met with "it's already > ugly so we might keep doing it that way" I enthusiastically backed a proposal for improvement literally in the paragraph you're replying to :) > > > > > > > > While having a model attribute and a model element is ugly, despite > exaclty matching QEMU's device, this is just a different way of saying > a generic device (e.g. isa-serial in my example above) There are no "generic" devices, only hypervisor-specific devices that present a similar interface to the guest but are completely separate and not interchangeable from the hypervisor's point of view. For controllers, we could have decided to use "intel-pcie-root-port" instead of "ioh3420" and then translate back and forth, but I guess at the time it was considered to be not worth doing, or perhaps the lack of redundancy resulted in the issue not being raised at all. > > Either way, my point is that while the current serial device XML is a > > bit redundant, at least it's mostly consistent > > consistent meaning it matches the QEMU devices? Yes, thus matching how controllers (and possibly other devices?) work. When I introduced the element for serial devices, that's what I based the idea on, so it makes sense to me that we'll keep following the same semantics. > BTW if you look at the title of this thread, it says 'debugcon-isa', while > the QEMU device is named 'isa-debugcon'. Clearly an oversight: if you look through the patches, you'll see that there is no 'debucon-isa' anywhere in the code. > > and its implementation > > is very simple; > > Ah, the beauty of using virDomainChrSerialTargetModelTypeToString > instead of qemuDomainChrSerialTargetModelTypeToString What about the beauty of not having to implement qemuDomainChrSerialTargetModelTypeToString() in the first place? :) > > what you suggest doing would compromise both of those > > positive facts without allowing us to remove any redundancy from the > > existing scenarios, so I think it would overall represent a net > > negative. > > It allows us not to introduce more redundancy. I don't believe avoiding those four extra bytes is worth the extra code complexity and deviating from well-established semantics. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del
On 2/13/19 2:39 PM, John Ferlan wrote:
On 2/13/19 5:58 AM, Michal Privoznik wrote:
On 2/12/19 11:19 PM, John Ferlan wrote:
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as
such. Also, DEVICE_DELETED event is not triggered (surprisingly,
since we're not issuing device_del rather than netdev_del) and
associated chardev is removed automagically too. This means that
we need to do qemuDomainRemoveChrDevice() minus monitor call to
remove the chardev.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_hotplug.c | 48 -
1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going
to be able to remove any guestfwd that is already assigned to a guest
because it'll have that "user-" prefix... It will work once the guest
is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a0ccc3b82c..107d0fb7a9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr
driver,
static int
qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- virDomainChrDefPtr chr)
+ virDomainChrDefPtr chr,
+ bool monitor)
{
virObjectEventPtr event;
char *charAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int ret = -1;
- int rc;
+ int rc = 0;
VIR_DEBUG("Removing character device %s from domain %p %s",
chr->info.alias, vm, vm->def->name);
- if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
- goto cleanup;
+ if (monitor) {
+ if (!(charAlias =
qemuAliasChardevFromDevAlias(chr->info.alias)))
+ goto cleanup;
- qemuDomainObjEnterMonitor(driver, vm);
- rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
+ }
if (rc == 0 &&
qemuDomainDelChardevTLSObjects(driver, vm, chr->source,
charAlias) < 0)
@@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
break;
case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
+ ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr,
true);
break;
case VIR_DOMAIN_DEVICE_RNG:
ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng);
@@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr
driver,
virDomainDefPtr vmdef = vm->def;
virDomainChrDefPtr tmpChr;
char *devstr = NULL;
+ bool guestfwd = false;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
virReportError(VIR_ERR_DEVICE_MISSING,
@@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr
driver,
goto cleanup;
}
+ /* guestfwd channels are not really -device rather than
+ * -netdev. We need to treat them slightly differently. */
+ guestfwd = tmpChr->deviceType ==
VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
+ tmpChr->targetType ==
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD;
+
if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef,
tmpChr, -1) < 0)
goto cleanup;
@@ -6144,22 +6153,31 @@ int
qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
if (qemuBuildChrDeviceStr(, vmdef, tmpChr,
priv->qemuCaps) < 0)
goto cleanup;
Bright idea...
The returned @devstr contains the "id=" value used to generate the
command line, thus why not extract out the id= value to be the alias
used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
Problem with this approach is that string processing in C is pain in the
#@$. Cutting out the alias from @devstr would be not trivial. But okay,
there's another approach - have yet another local variable that would be
initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might
override it for those types of chardev that need it. But this is rather
ugly.
It's not that bad...
size_t i;
const char *alias;
VIR_AUTOPTR(elems) = NULL;
then after is generated
if (!(elems = virStringSplit(devstr, "," 0)))
goto cleanup;
for (i = 0; elems[i]; i++) {
if ((alias = STRSKIP(elems[i], "id=")))
break;
if (!alias) {
alias = tmpChr->info.alias
VIR_WARN("Cannot find 'id=' in devstr='%s' using '%'s",
devstr, alias);
}
BTW: w/r/t whether tmpChr->info.alias checking should be done consider
commits c86735e2d and 2bd9db96 which I think removed
[libvirt] [PATCH] util: fix memory leak in virFirewallDInterfaceSetZone()
commit 3bba4825 added the new function virFirewallDInterfaceSetZone()
which calledsends virDBUSCallMethod a DBusMessage** for the reply
message, but doesn't use the reply, and also doesn't free it. Since
this arg is allowed to be NULL, this patch simply sets it to NULL so
we don't have to deal with it.
Signed-off-by: Laine Stump
---
src/util/virfirewalld.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
index 47bacdcf4a..f8965eea09 100644
--- a/src/util/virfirewalld.c
+++ b/src/util/virfirewalld.c
@@ -351,13 +351,12 @@ virFirewallDInterfaceSetZone(const char *iface,
const char *zone)
{
DBusConnection *sysbus = virDBusGetSystemBus();
-DBusMessage *reply = NULL;
if (!sysbus)
return -1;
return virDBusCallMethod(sysbus,
- ,
+ NULL,
NULL,
VIR_FIREWALL_FIREWALLD_SERVICE,
"/org/fedoraproject/FirewallD1",
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virjsontest: introduce DO_TEST_PARSE_FILE
On Wed, 2019-02-13 at 15:07 +0100, Ján Tomko wrote:
> On Wed, Feb 13, 2019 at 02:18:32PM +0100, Andrea Bolognani wrote:
> > Are the backslashes at the end of lines necessary?
>
> In this patch? Yes. The aim is to preserve the test coverage done before
> and after.
>
> > I've tried
> > removing a bunch of them and the test didn't break. Are the files
> > even valid JSON with the backslashes included?
>
> No, they get stripped by virTestLoadFile
Oh, I see, they're there to keep the input JSON as a single line
as it was originally. Makes sense.
> > Additional question: can't we pretty-print at least the input
> > files now? Unless of course the point of these specific test cases
> > is to prove we can successfully parse certain unusual constructs.
>
> The whole point of separating these was to allow easier changes
> and make it more-readable, so introducing more whitespace by removing
> the backslashes and prettifying it is possible.
Alright, it can be done in separate commits then.
[...]
> All the error messages match the original test. Guess it would make more
> sense to alter them before copying them.
Yeah, that way they get improved for FromString tests as well.
> > I think it would also look more legible if this entire if block was
> > inside the else branch of the previous if block, but if you feel
> > strongly about this version then just leave it as is.
>
> Like this?
>
> if (!injson) {
> if (info->pass) {
> return 0;
> } else {
> return -1;
> }
> } else {
> if (!info->pass)
> return -1;
> }
That's exactly what I had in mind.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails
On 2/13/19 7:38 AM, Marc Hartmayer wrote:
> Even if an error is reported by `udev_enumerate_scan_devices`,
> e.g. because a driver of a device has an bug, we can still enumerate
> all other devices. Additionally the documentation of
> udev_enumerate_scan_devices says that on success an integer >= 0 is
> returned (see man udev_enumerate_scan_devices(3)).
>
> Reviewed-by: Bjoern Walk
> Signed-off-by: Marc Hartmayer
> ---
> src/node_device/node_device_udev.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
Interesting - looking at many examples of udev_enumerate_scan_devices
usage shows a lack of testing the return value and as is done here just
using @udev_enumerate to add devices after the call.
Eventually found some source code for enumerator_scan_devices_tags which
I believe is what device_enumerator_scan_devices would call due to what
our AddMatches does. It seems that code works until it finds an error,
but still would return a partially enumerated list.
Long way of saying I think this is fine... However, now @ret = -1
doesn't ever get changed, so the caller would still fail. So it's a nice
way to test your other patch ;-)
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index 299f55260129..90168eb8a969 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
> if (udevEnumerateAddMatches(udev_enumerate) < 0)
> goto cleanup;
>
> -ret = udev_enumerate_scan_devices(udev_enumerate);
> -if (ret != 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("udev scan devices returned %d"),
> - ret);
> -goto cleanup;
> -}
> +if (udev_enumerate_scan_devices(udev_enumerate) < 0)
> +VIR_WARN("udev scan devices failed");
Either before or after this, set ret = 0... or change the default from
-1 to 0 and only change if the AddMatches fails.
I think the other patch would still be necessary since if
udevEnumerateAddMatches fails, then wouldn't the issue of setting
threadQuit still exist?
>
> udev_list_entry_foreach(list_entry,
> udev_enumerate_get_list_entry(udev_enumerate)) {
>
BTW: Using udevProcessDeviceListEntry as the 'example' of not failing if
an element of udev_enumerate is problematic, I think logically if we
don't get a full list we'd be OK to continue as well.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Wed, Feb 13, 2019 at 03:03 PM +0100, John Ferlan wrote:
> On 2/13/19 4:34 AM, Marc Hartmayer wrote:
>> On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan
>> wrote:
>>> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
(9f0ae0b18e3e620) has moved the heavy task of device enumeration into
a separate thread. The problem with this commit is that there is a
functionality change in the cleanup when udevEnumerateDevices
fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
by calling nodeStateCleanup (defined in node_device_udev.c) which
resulted in libvirtd stopping with the error message
'daemonRunStateInit:800 : Driver state initialization failed'. With
the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
that it must stop. This means that for example the watch handle isn't
removed from the main loop and this can result in the main loop
consuming 100% CPU time as soon as a new udev event occurs.
This patch proposes a simple solution to the described problem. In
case the udev thread stops the watch handle is removed from the main
loop.
Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
nodeStateInitialize")
Signed-off-by: Marc Hartmayer
---
Note: I'm not sure whether we should stop libvirtd (as it would have
been done before) or if this patch is already sufficient.
---
src/node_device/node_device_udev.c | 7 +++
1 file changed, 7 insertions(+)
>>>
>>> What you have seems reasonable - although I wonder if it would be better
>>> to remove the event handle in the error of
>>> nodeStateInitializeEnumerate.
>>
>> Might be better, yes - I’ve no strong opinion about that. I’ve added the
>> removal here because it doesn’t make sense to still have the watch
>> handle registered in the main loop if the udev thread stops - at least I
>> think so (just to be bulletproof).
>>
>> The more important point is before your change, the behavior was that
>> libvirtd stops after this error. Now (with this patch) it only removes
>> the watch handle and stops the udev thread. Is this change of behavior
>> okay?
>>
>
> This was mentioned during review of the patch - final response here:
>
> https://www.redhat.com/archives/libvir-list/2017-December/msg00531.html
>
> with the feeling that at least allowing other aspects of libvirt to
> function even though udev processing is crippled wasn't a bad thing.
Okay.
>
> Without the patch something would need to be done before starting
> libvirtd anyway. The "missing piece" was actually having what you had
> happen occur. Part of me wonders what fails such that enumeration fails
> and is that something we should be chasing instead. IOW: Is there
> something in udevEnumerateDevices failure that could just be ignored and
> we continue to operate logging the failure (like you've done in the
> patch you recently posted). Does that fix what caused this patch?
Yep, it does - if you’re referring to my mail
id:20190213123838.2826-1-mhart...@linux.ibm.com.
>
[…snip]
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virjsontest: introduce DO_TEST_PARSE_FILE
On Wed, Feb 13, 2019 at 02:18:32PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-12 at 16:57 +0100, Ján Tomko wrote:
[...]
+{"return": [{"filename": \
+"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server",\
+"label": "charmonitor"}, {"filename": "pty:/dev/pts/158",\
+"label": "charserial0"}], "id": "libvirt-3"}
Are the backslashes at the end of lines necessary?
In this patch? Yes. The aim is to preserve the test coverage done before
and after.
I've tried
removing a bunch of them and the test didn't break. Are the files
even valid JSON with the backslashes included?
No, they get stripped by virTestLoadFile
Additional question: can't we pretty-print at least the input
files now? Unless of course the point of these specific test cases
is to prove we can successfully parse certain unusual constructs.
The whole point of separating these was to allow easier changes
and make it more-readable, so introducing more whitespace by removing
the backslashes and prettifying it is possible.
[...]
+if (!injson) {
+if (info->pass) {
+VIR_TEST_VERBOSE("Fail to parse %s\n", info->name);
+return -1;
+} else {
+VIR_TEST_DEBUG("Fail to parse %s\n", info->name);
+return 0;
+}
+}
The second message should read something like
Expected failure while parsing %s
or
Failed to parse %s (expected)
+
+if (!info->pass) {
+VIR_TEST_VERBOSE("Should not have parsed %s\n", info->name);
+return -1;
+}
Maybe
Expected failure while parsing %s, got success instead
or something along those lines.
All the error messages match the original test. Guess it would make more
sense to alter them before copying them.
I think it would also look more legible if this entire if block was
inside the else branch of the previous if block, but if you feel
strongly about this version then just leave it as is.
Like this?
if (!injson) {
if (info->pass) {
return 0;
} else {
return -1;
}
} else {
if (!info->pass)
return -1;
}
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On 2/13/19 4:34 AM, Marc Hartmayer wrote:
> On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan
> wrote:
>> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>>> a separate thread. The problem with this commit is that there is a
>>> functionality change in the cleanup when udevEnumerateDevices
>>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>>> resulted in libvirtd stopping with the error message
>>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>>> that it must stop. This means that for example the watch handle isn't
>>> removed from the main loop and this can result in the main loop
>>> consuming 100% CPU time as soon as a new udev event occurs.
>>>
>>> This patch proposes a simple solution to the described problem. In
>>> case the udev thread stops the watch handle is removed from the main
>>> loop.
>>>
>>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>>> nodeStateInitialize")
>>> Signed-off-by: Marc Hartmayer
>>> ---
>>>
>>> Note: I'm not sure whether we should stop libvirtd (as it would have
>>> been done before) or if this patch is already sufficient.
>>>
>>> ---
>>> src/node_device/node_device_udev.c | 7 +++
>>> 1 file changed, 7 insertions(+)
>>>
>>
>> What you have seems reasonable - although I wonder if it would be better
>> to remove the event handle in the error of
>> nodeStateInitializeEnumerate.
>
> Might be better, yes - I’ve no strong opinion about that. I’ve added the
> removal here because it doesn’t make sense to still have the watch
> handle registered in the main loop if the udev thread stops - at least I
> think so (just to be bulletproof).
>
> The more important point is before your change, the behavior was that
> libvirtd stops after this error. Now (with this patch) it only removes
> the watch handle and stops the udev thread. Is this change of behavior
> okay?
>
This was mentioned during review of the patch - final response here:
https://www.redhat.com/archives/libvir-list/2017-December/msg00531.html
with the feeling that at least allowing other aspects of libvirt to
function even though udev processing is crippled wasn't a bad thing.
Without the patch something would need to be done before starting
libvirtd anyway. The "missing piece" was actually having what you had
happen occur. Part of me wonders what fails such that enumeration fails
and is that something we should be chasing instead. IOW: Is there
something in udevEnumerateDevices failure that could just be ignored and
we continue to operate logging the failure (like you've done in the
patch you recently posted). Does that fix what caused this patch?
John
>>
>> I think the assumption made was that by setting threadQuit that the next
>> event have some sort of failure through udevEventMonitorSanityCheck
>> which removes the EventHandle too. Although I cannot be sure it's been
>> too long to remember for sure ;-)
>>
>> Furthermore, should nodeStateCleanup be altered to check for priv->watch
>> == -1 and thus not worry about the code to set threadQuit, signal, and
>> joining the thread.
>>
>> John
>>
>> BTW: I'm subscribed to the list, no need to CC...
>
> Yep, I know :) But adding you to CC increases the chance that you’re
> looking for this patch since it was your commit.
>
> Thanks for the feedback!
>
>>
>>> diff --git a/src/node_device/node_device_udev.c
>>> b/src/node_device/node_device_udev.c
>>> index b1e5f00067e8..299f55260129 100644
>>> --- a/src/node_device/node_device_udev.c
>>> +++ b/src/node_device/node_device_udev.c
>>> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>>> }
>>>
>>> if (priv->threadQuit) {
>>> +if (priv->watch != -1) {
>>> +/* Since the udev thread getting stopped remove the
>>> + * watch handle from the main loop */
>>> +virEventRemoveHandle(priv->watch);
>>> +priv->watch = -1;
>>> +}
>>> +
>>> virObjectUnlock(priv);
>>> return;
>>> }
>>>
>>
> --
> Kind regards / Beste Grüße
>Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Matthias Hartmann
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virjsontest: switch AddAndRemove tests to work with files
On Tue, 2019-02-12 at 16:57 +0100, Ján Tomko wrote:
[...]
> +++ b/tests/virjsontest.c
> @@ -114,12 +114,24 @@ static int
> testJSONAddRemove(const void *data)
> {
> const struct testInfo *info = data;
> -virJSONValuePtr json;
> +virJSONValuePtr json = NULL;
> virJSONValuePtr name = NULL;
> -char *result = NULL;
> +char *infile = NULL;
> +char *indata = NULL;
> +char *outfile = NULL;
> +char *actual = NULL;
Feel free to convert this function and the rest of the file to
VIR_AUTOFREE() in a follow-up series O:-)
> @@ -159,20 +171,22 @@ testJSONAddRemove(const void *data)
> VIR_TEST_VERBOSE("%s", "unexpected failure adding new key\n");
> goto cleanup;
> }
> -if (!(result = virJSONValueToString(json, false))) {
> +if (!(actual = virJSONValueToString(json, false))) {
The amount of data is much smaller in this case, so whether or not
it's pretty printed doesn't make a lot of difference. I'd still
pretty print everything for consistency and because it just looks
much better, but it's okay to leave it as is too.
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del
On 2/13/19 5:58 AM, Michal Privoznik wrote:
> On 2/12/19 11:19 PM, John Ferlan wrote:
>>
>>
>> On 2/11/19 10:40 AM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1624204
>>>
>>> The guestfwd channels are -netdevs really. Hotunplug them as
>>> such. Also, DEVICE_DELETED event is not triggered (surprisingly,
>>> since we're not issuing device_del rather than netdev_del) and
>>> associated chardev is removed automagically too. This means that
>>> we need to do qemuDomainRemoveChrDevice() minus monitor call to
>>> remove the chardev.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> src/qemu/qemu_hotplug.c | 48 -
>>> 1 file changed, 33 insertions(+), 15 deletions(-)
>>>
>>
>> So while, yes this does work and fix the issue... It's still not going
>> to be able to remove any guestfwd that is already assigned to a guest
>> because it'll have that "user-" prefix... It will work once the guest
>> is restarted of course... so...
>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index a0ccc3b82c..107d0fb7a9 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr
>>> driver,
>>> static int
>>> qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>> virDomainObjPtr vm,
>>> - virDomainChrDefPtr chr)
>>> + virDomainChrDefPtr chr,
>>> + bool monitor)
>>> {
>>> virObjectEventPtr event;
>>> char *charAlias = NULL;
>>> qemuDomainObjPrivatePtr priv = vm->privateData;
>>> int ret = -1;
>>> - int rc;
>>> + int rc = 0;
>>> VIR_DEBUG("Removing character device %s from domain %p %s",
>>> chr->info.alias, vm, vm->def->name);
>>> - if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
>>> - goto cleanup;
>>> + if (monitor) {
>>> + if (!(charAlias =
>>> qemuAliasChardevFromDevAlias(chr->info.alias)))
>>> + goto cleanup;
>>> - qemuDomainObjEnterMonitor(driver, vm);
>>> - rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>>> + qemuDomainObjEnterMonitor(driver, vm);
>>> + rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>>> - if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>> - goto cleanup;
>>> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>> + goto cleanup;
>>> + }
>>> if (rc == 0 &&
>>> qemuDomainDelChardevTLSObjects(driver, vm, chr->source,
>>> charAlias) < 0)
>>> @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>> break;
>>> case VIR_DOMAIN_DEVICE_CHR:
>>> - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
>>> + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr,
>>> true);
>>> break;
>>> case VIR_DOMAIN_DEVICE_RNG:
>>> ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng);
>>> @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr
>>> driver,
>>> virDomainDefPtr vmdef = vm->def;
>>> virDomainChrDefPtr tmpChr;
>>> char *devstr = NULL;
>>> + bool guestfwd = false;
>>> if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>>> virReportError(VIR_ERR_DEVICE_MISSING,
>>> @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr
>>> driver,
>>> goto cleanup;
>>> }
>>> + /* guestfwd channels are not really -device rather than
>>> + * -netdev. We need to treat them slightly differently. */
>>> + guestfwd = tmpChr->deviceType ==
>>> VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>>> + tmpChr->targetType ==
>>> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD;
>>> +
>>> if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef,
>>> tmpChr, -1) < 0)
>>> goto cleanup;
>>> @@ -6144,22 +6153,31 @@ int
>>> qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>> if (qemuBuildChrDeviceStr(, vmdef, tmpChr,
>>> priv->qemuCaps) < 0)
>>> goto cleanup;
>>
>> Bright idea...
>>
>> The returned @devstr contains the "id=" value used to generate the
>> command line, thus why not extract out the id= value to be the alias
>> used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
>
> Problem with this approach is that string processing in C is pain in the
> #@$. Cutting out the alias from @devstr would be not trivial. But okay,
> there's another approach - have yet another local variable that would be
> initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might
> override it for those types of chardev that need it. But this is rather
> ugly.
>
It's not that bad...
size_t i;
const char *alias;
VIR_AUTOPTR(elems) = NULL;
then after is generated
if (!(elems = virStringSplit(devstr, "," 0)))
goto cleanup;
for (i = 0; elems[i]; i++) {
Re: [libvirt] [PATCH 2/3] virjsontest: switch DO_TEST_PARSE_FILE to use output files
On Tue, 2019-02-12 at 16:57 +0100, Ján Tomko wrote:
[...]
> +{"return":[{"filename":\
> +"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server",\
> +"label":"charmonitor"},{"filename":"pty:/dev/pts/158",\
> +"label":"charserial0"}],"id":"libvirt-3"}
Same questions as the previous patch when it comes to pretty
printing: it looks like...
[...]
> @@ -52,10 +55,8 @@ testJSONFromFile(const void *data)
> if (!(actual = virJSONValueToString(injson, false)))
> return -1;
... changing the second argument of virJSONValueToString() to true
is all that's needed to obtain much more legible output files. Any
reason why we shouldn't do that?
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virjsontest: introduce DO_TEST_PARSE_FILE
On Tue, 2019-02-12 at 16:57 +0100, Ján Tomko wrote:
[...]
> +{"return": [{"filename": \
> +"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server",\
> +"label": "charmonitor"}, {"filename": "pty:/dev/pts/158",\
> +"label": "charserial0"}], "id": "libvirt-3"}
Are the backslashes at the end of lines necessary? I've tried
removing a bunch of them and the test didn't break. Are the files
even valid JSON with the backslashes included?
Additional question: can't we pretty-print at least the input
files now? Unless of course the point of these specific test cases
is to prove we can successfully parse certain unusual constructs.
[...]
> +if (!injson) {
> +if (info->pass) {
> +VIR_TEST_VERBOSE("Fail to parse %s\n", info->name);
> +return -1;
> +} else {
> +VIR_TEST_DEBUG("Fail to parse %s\n", info->name);
> +return 0;
> +}
> +}
The second message should read something like
Expected failure while parsing %s
or
Failed to parse %s (expected)
> +
> +if (!info->pass) {
> +VIR_TEST_VERBOSE("Should not have parsed %s\n", info->name);
> +return -1;
> +}
Maybe
Expected failure while parsing %s, got success instead
or something along those lines.
I think it would also look more legible if this entire if block was
inside the else branch of the previous if block, but if you feel
strongly about this version then just leave it as is.
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails
Even if an error is reported by `udev_enumerate_scan_devices`,
e.g. because a driver of a device has an bug, we can still enumerate
all other devices. Additionally the documentation of
udev_enumerate_scan_devices says that on success an integer >= 0 is
returned (see man udev_enumerate_scan_devices(3)).
Reviewed-by: Bjoern Walk
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 299f55260129..90168eb8a969 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
if (udevEnumerateAddMatches(udev_enumerate) < 0)
goto cleanup;
-ret = udev_enumerate_scan_devices(udev_enumerate);
-if (ret != 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("udev scan devices returned %d"),
- ret);
-goto cleanup;
-}
+if (udev_enumerate_scan_devices(udev_enumerate) < 0)
+VIR_WARN("udev scan devices failed");
udev_list_entry_foreach(list_entry,
udev_enumerate_get_list_entry(udev_enumerate)) {
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
On Wed, Feb 13, 2019 at 10:59:28AM +0100, Andrea Bolognani wrote: On Wed, 2019-02-13 at 10:03 +0100, Ján Tomko wrote: On Tue, Feb 12, 2019 at 05:21:56PM +0100, Andrea Bolognani wrote: > On Tue, 2019-02-12 at 16:07 +0100, Ján Tomko wrote: > > > > There should be no pressure to maintain the 1:1 mapping. > > > > For QEMU, the devices need to be represented in single namespace, so > > > > they have to include the bus. In libvirt, we already have the serial > > > > type and the element. It does not have to be duplicated in the > > > > model name as well. > > Note that the element is not automatically added for > ISA devices, so that specific duplication is not present. The other duplication in serial type is more worrying. Back when I last touched that code, part of the duplication was already in place and neither me nor the reviewer were able to come up with a less redundant representation that still maintained some amount of logic and consistency between serial console types. So yeah, it's far from being an optimal solution, but that's kinda what happens when your XML design grows organically over 10+ years :) Also, the device will be given an iobase by QEMU, we should represent that in the XML and fill in that default. If we can manage to implement it in a way that's both reliable and backwards compatible, then I'm absolutely in favor of doing that! The current behavior is clearly not great. We cannot if any proposal for improvement is met with "it's already ugly so we might keep doing it that way" > > My point is that the internal implementation is not relevant here > > (we do map XML attributes to QEMU devices elsewhere, see > > qemuDeviceVideo), it's the XML that matters. > > > > The 'usb-serial', 'pci-serial', 'isa-serial' models are all a generic > > repetition of the target type, all of those are IMO better than > > or > > > > However > > > > > > > > looks better to me than > > > > > > > > We are consistently using the QEMU device name as the model > attribute for devices, Consistently mapping QEMU device names to libvirt attributes is explicitly a non-goal of libvirt. The reason for the XML should be: "it represents the device well", not "we won't need to add another enum". See "virtio-scsi" There is precedent for using the model attribute as a way to store the hypervisor-specific device name, eg. Ah, the copy & paste argument. While having a model attribute and a model element is ugly, despite exaclty matching QEMU's device, this is just a different way of saying a generic device (e.g. isa-serial in my example above) vs Either way, my point is that while the current serial device XML is a bit redundant, at least it's mostly consistent consistent meaning it matches the QEMU devices? BTW if you look at the title of this thread, it says 'debugcon-isa', while the QEMU device is named 'isa-debugcon'. and its implementation is very simple; Ah, the beauty of using virDomainChrSerialTargetModelTypeToString instead of qemuDomainChrSerialTargetModelTypeToString what you suggest doing would compromise both of those positive facts without allowing us to remove any redundancy from the existing scenarios, so I think it would overall represent a net negative. It allows us not to introduce more redundancy. Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] iohelper: Don't include newlines in error messages
The newline was pretty arbitrary, and we're better off
without it.
Signed-off-by: Andrea Bolognani
---
src/util/iohelper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 1ff4a7b314..aed7ef3184 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -236,7 +236,7 @@ main(int argc, char **argv)
return 0;
error:
-fprintf(stderr, _("%s: failure with %s\n: %s"),
+fprintf(stderr, _("%s: failure with %s: %s"),
program_name, path, virGetLastErrorMessage());
exit(EXIT_FAILURE);
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] virfile: Report error in virFileWrapperFdFree()
Logging the error is fine and all, but getting the information
to the user directly is even better.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani
---
src/util/virfile.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 271bf5e796..30cad87df9 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -351,8 +351,12 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
if (!wfd)
return;
+/* If the command used to process IO has produced errors, it's fair
+ * to assume those will be more relevant to the user than whatever
+ * eg. QEMU can figure out on its own, so it's okay if we end up
+ * discarding an existing error */
if (wfd->err_msg && *wfd->err_msg)
-VIR_WARN("iohelper reports: %s", wfd->err_msg);
+virReportError(VIR_ERR_OPERATION_FAILED, "%s", wfd->err_msg);
virCommandAbort(wfd->cmd);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/3] vircommand: Ensure buffers are NULL-terminated
The memory allocated by VIR_REALLOC_N() is uninitialized,
which means it's not possible to figure out whether any
output was produced at all after the fact.
Since we don't care about the previous contents of buffers,
if any, use VIR_FREE() followed by VIR_ALLOC_N() instead.
Signed-off-by: Andrea Bolognani
---
src/util/vircommand.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index d965068369..3d533c68a6 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -2055,12 +2055,14 @@ virCommandProcessIO(virCommandPtr cmd)
* results accumulated over a prior run of the same command. */
if (cmd->outbuf) {
outfd = cmd->outfd;
-if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0)
+VIR_FREE(*cmd->outbuf);
+if (VIR_ALLOC_N(*cmd->outbuf, 1) < 0)
ret = -1;
}
if (cmd->errbuf) {
errfd = cmd->errfd;
-if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0)
+VIR_FREE(*cmd->errbuf);
+if (VIR_ALLOC_N(*cmd->errbuf, 1) < 0)
ret = -1;
}
if (ret == -1)
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/3] qemu: Report better error on dump/migrate failure
Changes from [v1]: * Use VIR_FREE() followed by VIR_ALLOC_N() instead of manually setting the last (and only) byte of the array returned by VIR_REALLOC_N() to zero. [v1] https://www.redhat.com/archives/libvir-list/2019-February/msg00156.html Andrea Bolognani (3): vircommand: Ensure buffers are NULL-terminated virfile: Report error in virFileWrapperFdFree() iohelper: Don't include newlines in error messages src/util/iohelper.c | 2 +- src/util/vircommand.c | 6 -- src/util/virfile.c| 6 +- 3 files changed, 10 insertions(+), 4 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemuL: Drop "user-" prefix for guestfwd netdev
On 2/12/19 11:08 PM, John Ferlan wrote: $SUBJ: s/qemuL:/qemu:/ On 2/11/19 10:40 AM, Michal Privoznik wrote: Introduced by d86c876a66e3. There is no real need to have "user-" prefix for chardev. Signed-off-by: Michal Privoznik --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/channel-guestfwd.args | 2 +- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args | 4 ++-- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args | 4 ++-- tests/qemuxml2argvdata/name-escape.args | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) At least it wasn't "ua-" ;-) Although I don't believe this patch is necessary. It's only made necessary because two patches from now qemuDomainDetachChrDevice wants to use tmpChr->info.alias instead of grabbing the alias that was used to generated the command line from qemuBuildChrDeviceStr out of the @devstr. Let me hold off on an R-by for this until you give feedback on my patch4 comments... Given that guestfwd has a very limitted use case and that as you pointed out we switched it multiple times, there shouldn't be much problems with this. I rather be unable to detach guestfwd than come up with some complicated changes to our code just to allow it for domains started with older libvirt. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del
On 2/12/19 11:19 PM, John Ferlan wrote:
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as
such. Also, DEVICE_DELETED event is not triggered (surprisingly,
since we're not issuing device_del rather than netdev_del) and
associated chardev is removed automagically too. This means that
we need to do qemuDomainRemoveChrDevice() minus monitor call to
remove the chardev.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_hotplug.c | 48 -
1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going
to be able to remove any guestfwd that is already assigned to a guest
because it'll have that "user-" prefix... It will work once the guest
is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a0ccc3b82c..107d0fb7a9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
static int
qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- virDomainChrDefPtr chr)
+ virDomainChrDefPtr chr,
+ bool monitor)
{
virObjectEventPtr event;
char *charAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int ret = -1;
-int rc;
+int rc = 0;
VIR_DEBUG("Removing character device %s from domain %p %s",
chr->info.alias, vm, vm->def->name);
-if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
-goto cleanup;
+if (monitor) {
+if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
+goto cleanup;
-qemuDomainObjEnterMonitor(driver, vm);
-rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto cleanup;
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;
+}
if (rc == 0 &&
qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) <
0)
@@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
break;
case VIR_DOMAIN_DEVICE_CHR:
-ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr);
+ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true);
break;
case VIR_DOMAIN_DEVICE_RNG:
ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng);
@@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
virDomainDefPtr vmdef = vm->def;
virDomainChrDefPtr tmpChr;
char *devstr = NULL;
+bool guestfwd = false;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
virReportError(VIR_ERR_DEVICE_MISSING,
@@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
goto cleanup;
}
+/* guestfwd channels are not really -device rather than
+ * -netdev. We need to treat them slightly differently. */
+guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
+ tmpChr->targetType ==
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD;
+
if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) <
0)
goto cleanup;
@@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
if (qemuBuildChrDeviceStr(, vmdef, tmpChr, priv->qemuCaps) < 0)
goto cleanup;
Bright idea...
The returned @devstr contains the "id=" value used to generate the
command line, thus why not extract out the id= value to be the alias
used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
Problem with this approach is that string processing in C is pain in the
#@$. Cutting out the alias from @devstr would be not trivial. But okay,
there's another approach - have yet another local variable that would be
initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might
override it for those types of chardev that need it. But this is rather
ugly.
That way patch2 becomes unnecessary (I tried it). Of course fear always
strikes my soul when "assuming" anything, so I figured I'd run it past
you first for your thoughts. I am OK with the patches as they are
except for the tiny gotcha of never being able to remove that
user-channel# device.
Well, one way to look at it might be: if you have a domain started via
older libvirt nothing changes for you. You're unable to detach guestfwd
just like you always was :-)
Then again, guestfwd really makes sense only with slirp (that's also why
qemu rejects any other IP than 10.0.2.1) and since we're not setting
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
On Wed, 2019-02-13 at 10:03 +0100, Ján Tomko wrote: > On Tue, Feb 12, 2019 at 05:21:56PM +0100, Andrea Bolognani wrote: > > On Tue, 2019-02-12 at 16:07 +0100, Ján Tomko wrote: > > > > > There should be no pressure to maintain the 1:1 mapping. > > > > > For QEMU, the devices need to be represented in single namespace, so > > > > > they have to include the bus. In libvirt, we already have the serial > > > > > type and the element. It does not have to be duplicated in > > > > > the > > > > > model name as well. > > > > Note that the element is not automatically added for > > ISA devices, so that specific duplication is not present. > > The other duplication in serial type is more worrying. Back when I last touched that code, part of the duplication was already in place and neither me nor the reviewer were able to come up with a less redundant representation that still maintained some amount of logic and consistency between serial console types. So yeah, it's far from being an optimal solution, but that's kinda what happens when your XML design grows organically over 10+ years :) > Also, the device will be given an iobase by QEMU, we should represent > that in the XML and fill in that default. If we can manage to implement it in a way that's both reliable and backwards compatible, then I'm absolutely in favor of doing that! The current behavior is clearly not great. > > > My point is that the internal implementation is not relevant here > > > (we do map XML attributes to QEMU devices elsewhere, see > > > qemuDeviceVideo), it's the XML that matters. > > > > > > The 'usb-serial', 'pci-serial', 'isa-serial' models are all a generic > > > repetition of the target type, all of those are IMO better than > > > or > > > > > > However > > > > > > > > > > > > looks better to me than > > > > > > > > > > > > > We are consistently using the QEMU device name as the model > > attribute for devices, > > Consistently mapping QEMU device names to libvirt attributes is > explicitly a non-goal of libvirt. The reason for the XML should > be: "it represents the device well", not "we won't need to add another > enum". See "virtio-scsi" There is precedent for using the model attribute as a way to store the hypervisor-specific device name, eg. vs Either way, my point is that while the current serial device XML is a bit redundant, at least it's mostly consistent and its implementation is very simple; what you suggest doing would compromise both of those positive facts without allowing us to remove any redundancy from the existing scenarios, so I think it would overall represent a net negative. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan wrote:
> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>> a separate thread. The problem with this commit is that there is a
>> functionality change in the cleanup when udevEnumerateDevices
>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>> resulted in libvirtd stopping with the error message
>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>> that it must stop. This means that for example the watch handle isn't
>> removed from the main loop and this can result in the main loop
>> consuming 100% CPU time as soon as a new udev event occurs.
>>
>> This patch proposes a simple solution to the described problem. In
>> case the udev thread stops the watch handle is removed from the main
>> loop.
>>
>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>> nodeStateInitialize")
>> Signed-off-by: Marc Hartmayer
>> ---
>>
>> Note: I'm not sure whether we should stop libvirtd (as it would have
>> been done before) or if this patch is already sufficient.
>>
>> ---
>> src/node_device/node_device_udev.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>
> What you have seems reasonable - although I wonder if it would be better
> to remove the event handle in the error of
> nodeStateInitializeEnumerate.
Might be better, yes - I’ve no strong opinion about that. I’ve added the
removal here because it doesn’t make sense to still have the watch
handle registered in the main loop if the udev thread stops - at least I
think so (just to be bulletproof).
The more important point is before your change, the behavior was that
libvirtd stops after this error. Now (with this patch) it only removes
the watch handle and stops the udev thread. Is this change of behavior
okay?
>
> I think the assumption made was that by setting threadQuit that the next
> event have some sort of failure through udevEventMonitorSanityCheck
> which removes the EventHandle too. Although I cannot be sure it's been
> too long to remember for sure ;-)
>
> Furthermore, should nodeStateCleanup be altered to check for priv->watch
> == -1 and thus not worry about the code to set threadQuit, signal, and
> joining the thread.
>
> John
>
> BTW: I'm subscribed to the list, no need to CC...
Yep, I know :) But adding you to CC increases the chance that you’re
looking for this patch since it was your commit.
Thanks for the feedback!
>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index b1e5f00067e8..299f55260129 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>> }
>>
>> if (priv->threadQuit) {
>> +if (priv->watch != -1) {
>> +/* Since the udev thread getting stopped remove the
>> + * watch handle from the main loop */
>> +virEventRemoveHandle(priv->watch);
>> +priv->watch = -1;
>> +}
>> +
>> virObjectUnlock(priv);
>> return;
>> }
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
On Tue, Feb 12, 2019 at 05:21:56PM +0100, Andrea Bolognani wrote: On Tue, 2019-02-12 at 16:07 +0100, Ján Tomko wrote: On Tue, Feb 12, 2019 at 02:44:05PM +, Nikolay Shirokovskiy wrote: > On 12.02.2019 17:37, Ján Tomko wrote: > > On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote: > > > Diff from v1 [1]: > > > = > > > - expose the device as serial device instead of channel in config > > > > > > I use isa-debugcon name becase libvirt passes these names to qemu as-is > > > so I don't want to make exception for this device. > > > > There should be no pressure to maintain the 1:1 mapping. > > For QEMU, the devices need to be represented in single namespace, so > > they have to include the bus. In libvirt, we already have the serial > > type and the element. It does not have to be duplicated in the > > model name as well. Note that the element is not automatically added for ISA devices, so that specific duplication is not present. The other duplication in serial type is more worrying. Also, the device will be given an iobase by QEMU, we should represent that in the XML and fill in that default. > Yeah. But we already have models like isa-serial, usb-serial etc. And thus > we don't need map libvirt models to qemu models i.e. internally > we use virDomainChrSerialTargetModelTypeToString to generates names for > qemu. It would be odd if I start to use map just for debugcon now. My point is that the internal implementation is not relevant here (we do map XML attributes to QEMU devices elsewhere, see qemuDeviceVideo), it's the XML that matters. The 'usb-serial', 'pci-serial', 'isa-serial' models are all a generic repetition of the target type, all of those are IMO better than or However looks better to me than We are consistently using the QEMU device name as the model attribute for devices, Consistently mapping QEMU device names to libvirt attributes is explicitly a non-goal of libvirt. The reason for the XML should be: "it represents the device well", not "we won't need to add another enum". See "virtio-scsi" so I don't really see a compelling reason to start adding inconsistencies now... Do you remember when you lost your passion for this work? -- Lisa Simpson Jano -- Andrea Bolognani / Red Hat / Virtualization signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/6] Introduce more NULLSTR macros
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > Instead of using EMPTY_?STR with various meanings, > use a family of NULLSTR_.+ macros I didn't check particularly well, but you seem to have missed a few: src/util/viriptables.c:portRangeStr ? portRangeStr : ""); src/util/viriptables.c:portRangeStr ? portRangeStr : ""); src/util/virnetdevip.c: peerStr ? " peer " : "", peerStr ? peerStr : "", src/util/virnetdevip.c: bcastStr ? " bcast " : "", bcastStr ? bcastStr : "", src/util/virnetdevip.c: peerStr ? " peer " : "", peerStr ? peerStr : "", src/util/virnetdevip.c: bcastStr ? " bcast " : "", bcastStr ? bcastStr : "", tests/domaincapstest.c:Machine ? "-" : "", Machine ? Machine : "", \ tools/virsh-domain-monitor.c: target ? target : "-", tools/virsh-domain-monitor.c: source ? source : "-", tools/virsh-domain-monitor.c: model ? model : "-", tools/virsh-domain-monitor.c: mac ? mac : "-", tools/virsh-domain.c:if (vshTableRowAppend(table, iothreadIdStr, pinInfo ? pinInfo : "", NULL) < 0) tools/virsh-domain.c: targets ? targets : "", You can just post a couple of follow-up patches to take care of those instead of respinning, it's gonna be more convenient for both of us ;) Please also consider adding a syntax-check rule to avoid more instances of the open coded version creeping in over time. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] Use NULLSTR_EMPTY even more
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > This time even in places that would possibly better be served by > a more complex macro. > > Can be squashed into the previous patch if requested. I agree that the level of repetition might call for a local macro, but you can do that in a follow-up patch; for the time being, just squashing it into the previous one will do. Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] Use NULLSTR_EMPTY
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > Instead of repetitive: > s ? s : "" > use NULLSTR_EMPTY. > > Signed-off-by: Ján Tomko > --- > src/datatypes.c | 2 +- > src/libvirt-admin.c | 2 +- > src/libvirt.c| 2 +- > src/lxc/lxc_controller.c | 2 +- > src/qemu/qemu_command.c | 8 +++- > src/qemu/qemu_interface.c| 2 +- > src/qemu/qemu_migration_params.c | 2 +- > src/qemu/qemu_monitor_json.c | 2 +- > src/qemu/qemu_process.c | 4 ++-- > src/util/vircgroupv1.c | 2 +- > src/util/vircgroupv2.c | 2 +- > src/util/virnetdevvportprofile.c | 4 ++-- > src/util/virnuma.c | 4 ++-- > src/util/virstoragefile.c| 2 +- > src/util/virsystemd.c| 4 ++-- > src/util/viruri.c| 2 +- > src/xenconfig/xen_sxpr.c | 18 ++ > tools/virsh-domain-monitor.c | 2 +- > tools/virsh-domain.c | 2 +- > tools/virt-admin.c | 4 ++-- > 20 files changed, 32 insertions(+), 40 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] Remove EMPTY_STR macro
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > Another misleadingly named macro. > Deprecate in favor of NULLSTR_STAR. > > Signed-off-by: Ján Tomko > --- > src/util/virlease.c | 15 --- > 1 file changed, 4 insertions(+), 11 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] Remove EMPTYSTR macro
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > This macro neither takes nor produces an empty string. > Remove it in favor of NULLSTR_MINUS. > > Signed-off-by: Ján Tomko > --- > src/internal.h| 5 - > tools/virsh-network.c | 10 +- > 2 files changed, 5 insertions(+), 10 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] tools: use NULLSTR_MINUS
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > Use the newly introduced macro in the few places that open-code it. > > Signed-off-by: Ján Tomko > --- > tools/virsh-domain-monitor.c | 4 ++-- > tools/virsh-snapshot.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] internal: introduce a family of NULLSTR macros
On Tue, 2019-02-12 at 17:40 +0100, Ján Tomko wrote: > NULLSTR_EMPTY, the quiet child, > NULLSTR_STAR, the famous one and > NULLSTR_MINUS, the grumpy one. > > Signed-off-by: Ján Tomko > --- > src/internal.h | 15 +++ > 1 file changed, 15 insertions(+) 10/10 commit message, would Reviewed-by: Andrea Bolognani again. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/17] qemu: virtio-{non-}transitional support
On Fri, 2019-02-08 at 17:11 -0500, Cole Robinson wrote:
> v2 libvirt patches:
> https://www.redhat.com/archives/libvir-list/2019-January/msg00877.html
> v1 libvirt patches:
> https://www.redhat.com/archives/libvir-list/2019-January/msg00593.html
> Previous incomplete RFC here:
> https://www.redhat.com/archives/libvir-list/2019-January/msg00346.html
> qemu patches, queued for qemu 4.0.0:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00923.html
> Previous libvirt discussion around this:
> https://www.redhat.com/archives/libvir-list/2018-August/msg01073.html
>
> Changes since v2:
> * Some prep patches merged
> * filesystem model dropped the -9p naming, now uses virtio-{non-}transitional
> like other devices
> * Now uses a single capability QEMU_CAPS_VIRTIO_PCI_NON_TRANSITIONAL
> which is set whenever any of the -transitional or -non-transitional
> devices are present.
> * Add a formatdomain section 'Virtio transitional devices' and reference
> it from each relevant device section
> * if virtio-transitional specified, and qemu is too old but has the
> disable_X options, convert it to explicit
> disable-legacy=off,disable-modern=off
> * Misc small changes pointed out in review
Oh, and this definitely deserves to be mentioned in the release
notes! Can you please post a 18/17 or a separate patch that adds
the corresponding entry?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
