[libvirt] [PATCH 3/5] test: Support topological visits
snapshot_conf does all the hard work, the test driver just has to accept the new flag. Signed-off-by: Eric Blake --- src/test/test_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ef754658f3..02cd4f4d07 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5960,6 +5960,7 @@ testDomainSnapshotNum(virDomainPtr domain, unsigned int flags) int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) @@ -5981,6 +5982,7 @@ testDomainSnapshotListNames(virDomainPtr domain, int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) @@ -6002,6 +6004,7 @@ testDomainListAllSnapshots(virDomainPtr domain, int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) @@ -6024,6 +6027,7 @@ testDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) @@ -6049,6 +6053,7 @@ testDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) @@ -6074,6 +6079,7 @@ testDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] qemu: Support topological visits
snapshot_conf does all the hard work, the qemu driver just has to accept the new flag. Signed-off-by: Eric Blake --- src/qemu/qemu_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 205e544d92..e461fb51b0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15972,6 +15972,7 @@ qemuDomainSnapshotListNames(virDomainPtr domain, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) @@ -15997,6 +15998,7 @@ qemuDomainSnapshotNum(virDomainPtr domain, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) @@ -16022,6 +16024,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) @@ -16049,6 +16052,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) @@ -16078,6 +16082,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) @@ -16107,6 +16112,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] snapshots: Support topological visits
Wire up support for VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in the
domain-agnostic support code.
Clients of snapshot_conf were previously getting a depth-first search
on anything that used virDomainSnapshotForEachDescendant(); but a
switch to a breadth-first search will give a topological search.
With that change, we now always have a topological sort for
virDomainSnapshotListAllChildren(); then with one more tweak, we
can get a topological rather than a faster random hash visit
for virDomainListAllSnapshots().
Note that virDomainSnapshotForEach() still uses a random hash
visit; we could change that signature to take a tri-state for
random, depth-first, or breadth-first visit if we ever had clients
that cared about the distinctions, but for now, none of the
drivers seem to care.
Signed-off-by: Eric Blake
---
src/conf/snapshot_conf.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 224f3e6ca1..e2c91a5072 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1213,9 +1213,10 @@
virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots,
from = >metaroot;
}
-/* We handle LIST_ROOT/LIST_DESCENDANTS directly, mask that bit
- * out to determine when we must use the filter callback. */
-data.flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
+/* We handle LIST_ROOT/LIST_DESCENDANTS and LIST_TOPOLOGICAL directly,
+ * mask those bits out to determine when we must use the filter callback.
*/
+data.flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
+VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL);
/* If this common code is being used, we assume that all snapshots
* have metadata, and thus can handle METADATA up front as an
@@ -1240,7 +1241,11 @@
virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots,
data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION;
if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) {
-if (from->def)
+/* We could just always do a topological visit; but it is
+ * possible to optimize for less stack usage and time when a
+ * simpler full hashtable visit or counter will do. */
+if (from->def || (names &&
+ (flags & VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL)))
virDomainSnapshotForEachDescendant(from,
virDomainSnapshotObjListCopyNames,
);
@@ -1327,10 +1332,10 @@ virDomainSnapshotActOnDescendant(void *payload,
virDomainSnapshotObjPtr obj = payload;
struct snapshot_act_on_descendant *curr = data;
+(curr->iter)(payload, name, curr->data);
curr->number += 1 + virDomainSnapshotForEachDescendant(obj,
curr->iter,
curr->data);
-(curr->iter)(payload, name, curr->data);
return 0;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] snapshots: Add flag to guarantee topological sort
When doing REDEFINE on multiple snapshot metadata XML descriptions, we
require that a child cannot be redefined before its parent. Since
libvirt already tracks a DAG, it is more convenient if we can ensure
that virDomainListAllSnapshots() and friends have a way to return data
in an order that we can directly reuse, rather than having to
post-process the data ourselves to reconstruct the DAG.
Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a
guarantee at the time of the API call conclusion; there's always a
possible TOCTTOU race where someone redefining snapshots in between
the API results and the client actually using the list might render
the list out-of-date). Four listing APIs are directly benefitted by
the new flag; additionally, since we document that the older racy
ListNames interfaces should be sized by using the same flags on their
Num counterparts, the Num interfaces must document when they accept
(and ignore) the flag.
Signed-off-by: Eric Blake
---
include/libvirt/libvirt-domain-snapshot.h | 4 +++
src/libvirt-domain-snapshot.c | 42 ++-
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index d9b689abbd..602e5def59 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -135,6 +135,10 @@ typedef enum {
VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL= (1 << 9), /* Filter by snapshots
that use files external
to disk images */
+
+VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur
+ before children in
+ the resulting list */
} virDomainSnapshotListFlags;
/* Return the number of snapshots for this domain */
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 11d84289f8..d133c84933 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -1,7 +1,7 @@
/*
* libvirt-domain-snapshot.c: entry points for virDomainSnapshotPtr APIs
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -298,6 +298,10 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
*
* Provides the number of domain snapshots for this domain.
*
+ * This function will accept VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in
+ * @flags only if virDomainSnapshotListNames() can honor it, although
+ * the flag has no other effect here.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -369,6 +373,14 @@ virDomainSnapshotNum(virDomainPtr domain, unsigned int
flags)
* their names in @names. The value to use for @nameslen can be determined
* by virDomainSnapshotNum() with the same @flags.
*
+ * If @flags contains VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL, and no
+ * other connection is modifying snapshots, then it is guaranteed that
+ * for any snapshot in the resulting list, then no snapshots later in
+ * the list can be reached by a sequence of
+ * virDomainSnapshotGetParent() starting from that snapshot;
+ * otherwise, the order of snapshots in the resulting list is
+ * unspecified.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -457,6 +469,14 @@ virDomainSnapshotListNames(virDomainPtr domain, char
**names, int nameslen,
* an array to store those objects. This API solves the race inherent in
* virDomainSnapshotListNames().
*
+ * If @flags contains VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL and @snaps
+ * is non-NULL, and no other connection is modifying snapshots, then
+ * it is guaranteed that for any snapshot in the resulting list, no
+ * snapshots later in the list can be reached by a sequence of
+ * virDomainSnapshotGetParent() starting from that snapshot;
+ * otherwise, the order of snapshots in the resulting list is
+ * unspecified.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -533,6 +553,10 @@ virDomainListAllSnapshots(virDomainPtr domain,
virDomainSnapshotPtr **snaps,
*
* Provides the number of child snapshots for this domain snapshot.
*
+ * This function will accept VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in
+ * @flags only
[libvirt] [PATCH 0/5] snapshots: topological sorting (incremental backup saga)
While working to add new API for bulk dumpxml/redefine, and turn virDomainSnapshotObjList into a base class for sharing with checkpoints, I realized how trivial it would be to take advantage of information already present on the server, instead of using a lot of CPU time with 'virsh snapshot-list --tree' to reconstruct a DAG on the client side. Eric Blake (5): snapshots: Add flag to guarantee topological sort snapshots: Support topological visits test: Support topological visits qemu: Support topological visits virsh: Add snapshot-list --topological include/libvirt/libvirt-domain-snapshot.h | 4 +++ src/conf/snapshot_conf.c | 15 +--- src/libvirt-domain-snapshot.c | 42 ++- src/qemu/qemu_driver.c| 6 src/test/test_driver.c| 6 tools/virsh-snapshot.c| 16 +++-- tools/virsh.pod | 7 +++- 7 files changed, 86 insertions(+), 10 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] virsh: Add snapshot-list --topological
To some extent, virsh already has a (shockingly naive [1])
client-side topological sorter with the --tree option. But
as a series of REDEFINE calls must be presented in topological
order, it's worth letting the server do the work for us.
[1] The XXX comment about O(n^3) in virshSnapshotListCollect() is
telling; https://en.wikipedia.org/wiki/Topological_sorting is an
interesting resource for anyone motivated to use a more elegant
algorithm than brute force.
For now, I am purposefully NOT implementing virsh fallback code to
provide a topological sort when the flag was rejected as unsupported;
we can worry about that down the road if users actually demonstrate
that they use new virsh but old libvirt to even need the fallback.
The test driver makes it easy to test:
$ virsh -c test:///default '
snapshot-create-as test a
snapshot-create-as test c
snapshot-create-as test b
snapshot-list test
snapshot-list test --topological
snapshot-list test --descendants a
snapshot-list test --descendants a --topological
snapshot-list test --tree
snapshot-list test --tree --topological
'
Without --topological, virsh does client-side sorting alphabetically,
and lists 'b' before 'c' (even though 'c' is the parent of 'b'); with
the flag, virsh skips sorting, and you can now see that the server
handed back data in a correct ordering. As shown here with a simple
linear chain, there isn't any other possible ordering, and --tree mode
doesn't seem to care whether --topological is used. But it is
possible to compose more complicated DAGs with multiple children to a
parent (representing reverting back to a snapshot then creating more
snapshots along those divergent execution timelines), where it may
become possible to observe non-deterministic behavior when
--topological is in use, but even so, the result will still be
topologically correct.
Signed-off-by: Eric Blake
---
tools/virsh-snapshot.c | 16 +---
tools/virsh.pod| 7 ++-
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 025321c58e..31153f5b10 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1272,7 +1272,9 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr
dom,
* still in list. We mark known descendants by clearing
* snaps[i].parents. Sorry, this is O(n^3) - hope your
* hierarchy isn't huge. XXX Is it worth making O(n^2 log n)
- * by using qsort and bsearch? */
+ * by using qsort and bsearch? Or even a linear topological
+ * sort such as Kahn's algorithm? Should we emulate
+ * --topological for older libvirt that lacked the flag? */
if (start_index < 0) {
vshError(ctl, _("snapshot %s disappeared from list"), fromname);
goto cleanup;
@@ -1351,8 +1353,9 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr
dom,
}
}
}
-qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps),
- virshSnapSorter);
+if (!(orig_flags & VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL))
+qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps),
+ virshSnapSorter);
snaplist->nsnaps -= deleted;
VIR_STEAL_PTR(ret, snaplist);
@@ -1451,6 +1454,10 @@ static const vshCmdOptDef opts_snapshot_list[] = {
.type = VSH_OT_BOOL,
.help = N_("list snapshot names only")
},
+{.name = "topological",
+ .type = VSH_OT_BOOL,
+ .help = N_("sort list topologically rather than by name"),
+},
{.name = NULL}
};
@@ -1512,6 +1519,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
FILTER("external", EXTERNAL);
#undef FILTER
+if (vshCommandOptBool(cmd, "topological"))
+flags |= VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL;
+
if (roots)
flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 5759a396d4..66e2bf24ec 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4726,7 +4726,7 @@ Output basic information about a named , or the
current snapshot
with I<--current>.
=item B I [I<--metadata>] [I<--no-metadata>]
-[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}]
+[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] [I<--topological>]
[{[I<--from>] B | I<--current>} [I<--descendants>]]
[I<--leaves>] [I<--no-leaves>] [I<--inactive>] [I<--active>]
[I<--disk-only>] [I<--internal>] [I<--external>]
@@ -4734,6 +4734,11 @@ with I<--current>.
List all of the available snapshots for the given domain, defaulting
to show columns for the snapshot name, creation time, and domain state.
+Normally, table form output is sorted by snapshot name; using
+I<--topological> instead sorts so that no child is listed before its
+ancestors (although there may be more than one possible ordering with
+this property).
+
If I<--parent> is specified, add a column to the output table giving
the name of the parent of each
[libvirt] [PATCH] snapshots: Trivial doc improvements
Fix an incorrect @xmlDesc comment, as well as adding more details about which XML element should be root. Signed-off-by: Eric Blake --- Pushing this one as trivial; noticed it while working on Jan's suggestion of a new API for bulk dumpxml/redefine. src/libvirt-domain-snapshot.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index a647a500d6..11d84289f8 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -98,11 +98,11 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) /** * virDomainSnapshotCreateXML: * @domain: a domain object - * @xmlDesc: string containing an XML description of the domain + * @xmlDesc: string containing an XML description of the domain snapshot * @flags: bitwise-OR of virDomainSnapshotCreateFlags * * Creates a new snapshot of a domain based on the snapshot xml - * contained in xmlDesc. + * contained in xmlDesc, with a top-level element . * * If @flags is 0, the domain can be active, in which case the * snapshot will be a full system snapshot (capturing both disk state, @@ -247,7 +247,8 @@ virDomainSnapshotCreateXML(virDomainPtr domain, * @snapshot: a domain snapshot object * @flags: bitwise-OR of supported virDomainSnapshotXMLFlags * - * Provide an XML description of the domain snapshot. + * Provide an XML description of the domain snapshot, with a top-level + * element of . * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/4] qemu_domain: NVLink2 device tree functions for PPC64
On 08/03/2019 04:51, Piotr Jaroszynski wrote:
> On 3/7/19 7:39 AM, Daniel Henrique Barboza wrote:
>>
>>
>> On 3/7/19 12:15 PM, Erik Skultety wrote:
>>> On Tue, Mar 05, 2019 at 09:46:08AM -0300, Daniel Henrique Barboza wrote:
The NVLink2 support in QEMU implements the detection of NVLink2
capable devices by verfying the attributes of the VFIO mem region
QEMU allocates for the NVIDIA GPUs. To properly allocate an
adequate amount of memLock, Libvirt needs this information before
a QEMU instance is even created.
An alternative is presented in this patch. Given a PCI device,
we'll traverse the device tree at /proc/device-tree to check if
the device has a NPU bridge, retrieve the node of the NVLink2 bus,
find the memory-node that is related to the bus and see if it's a
NVLink2 bus by inspecting its 'reg' value. This logic is contained
inside the 'device_is_nvlink2_capable' function, which uses other
new helper functions to navigate and fetch values from the device
tree nodes.
Signed-off-by: Daniel Henrique Barboza
---
>>> This fails with a bunch of compilation errors, please make sure you run make
>>> check and make syntax-check on each patch of the series.
>>
>> Ooops, forgot to follow up make syntax-check with 'make' before
>> submitting ... my bad.
>>
>>>
src/qemu/qemu_domain.c | 194 +
1 file changed, 194 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 77548c224c..97de5793e2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10343,6 +10343,200 @@
qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm)
}
+/**
+ * Reads a phandle file and returns the phandle value.
+ */
+static int
+read_dt_phandle(const char* file)
>>> ..we prefer camelCase naming of functions...all functions should have a
>>> prefix,
>>> e.g. vir,, etc.
>>>
+{
+unsigned int buf[1];
+size_t read;
+FILE *f;
+
+f = fopen(file, "r");
+if (!f)
+return -1;
+
+read = fread(buf, sizeof(unsigned int), 1, f);
>>> We already have safe helpers that take care of such operations.
>>>
+
+if (!read) {
+VIR_CLOSE(f);
>>> You could use VIR_AUTOCLOSE for this
>>>
+return 0;
+}
+
+VIR_CLOSE(f);
+return be32toh(buf[0]);
>>> Is this part of gnulib? We need to make sure it's portable otherwise we'd
>>> need
>>> to make the conversion ourselves, just like for le64toh
>>>
+}
+
+
+/**
+ * Reads a memory reg file and returns the first 4 int values.
+ *
+ * The caller is responsible for freeing the returned array.
+ */
+static unsigned int *
+read_dt_memory_reg(const char *file)
+{
+unsigned int *buf;
+size_t read, i;
+FILE *f;
+
+f = fopen(file, "r");
+if (!f)
+return NULL;
+
+if (VIR_ALLOC_N(buf, 4) < 0)
+return NULL;
+
+read = fread(buf, sizeof(unsigned int), 4, f);
+
+if (!read && read < 4)
+/* shouldn't happen */
+VIR_FREE(buf);
+else for (i = 0; i < 4; i++)
+buf[i] = be32toh(buf[i]);
+
+VIR_CLOSE(f);
+return buf;
>>> Same comments as above...
>>>
+}
+
+
+/**
+ * This wrapper function receives arguments to be used in a
+ * 'find' call to retrieve the file names that matches
+ * the criteria inside the /proc/device-tree dir.
+ *
+ * A 'find' call with '-iname phandle' inside /proc/device-tree
+ * provides more than a thousand matches. Adding '-path' to
+ * narrow it down further is necessary to keep the file
+ * listing sane.
+ *
+ * The caller is responsible to free the buffer returned by
+ * this function.
+ */
+static char *
+retrieve_dt_files_pattern(const char *path_pattern, const char
*file_pattern)
+{
+virCommandPtr cmd = NULL;
+char *output = NULL;
+
+cmd = virCommandNew("find");
+virCommandAddArgList(cmd, "/proc/device-tree/", "-path", path_pattern,
+ "-iname", file_pattern, NULL);
+virCommandSetOutputBuffer(cmd, );
+
+if (virCommandRun(cmd, NULL) < 0)
+VIR_FREE(output);
+
+virCommandFree(cmd);
+return output;
+}
+
+
+/**
+ * Helper function that receives a listing of file names and
+ * calls read_dt_phandle() on each one finding for a match
+ * with the given phandle argument. Returns the file name if a
+ * match is found, NULL otherwise.
+ */
+static char *
+find_dt_file_with_phandle(char *files, int phandle)
+{
Re: [libvirt] [PATCH v5 05/20] wip: backup: Document new XML for backups
On 3/6/19 11:47 PM, Eric Blake wrote: > wip: Still need to incorporate v4 review comments... and let 'make check' finish prior to sending late at night. > +++ b/docs/docs.html.in > @@ -80,7 +80,9 @@ >storage pool capabilities, >node devices, >secrets, > - snapshots > + snapshots, > + checkpoints, > + backup jobs > As this is obviously botched duplication. At least it's an obvious fix. -- 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] tools/virt-host-validate: Fix IOMMU check on s390x
On 07/03/2019 18.15, Michal Privoznik wrote:
> On 3/1/19 12:10 PM, Thomas Huth wrote:
>> When running virt-host-validate on an s390x host, the tool currently
>> warns
>> that it is "Unknown if this platform has IOMMU support". We can use the
>> common check for entries in /sys/kernel/iommu_groups here, too, but it
>> only
>> makes sense to check it if there are also PCI devices available. It's
>> also
>> common on s390x that there are no PCI devices assigned to the LPAR,
>> and in
>> that case there is no need for the PCI-related IOMMU, so without PCI
>> devices
>> we should simply skip this test.
>>
>> Signed-off-by: Thomas Huth
>> ---
>> v2:
>> - Use virDirOpen() + virDirRead() instead of stat() - this should
>> hopefully
>> work now as expected.
>>
>> tools/virt-host-validate-common.c | 22 +-
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/virt-host-validate-common.c
>> b/tools/virt-host-validate-common.c
>> index 73e3bdb..e27b558 100644
>> --- a/tools/virt-host-validate-common.c
>> +++ b/tools/virt-host-validate-common.c
>> @@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
>> virBitmapPtr flags;
>> struct stat sb;
>> const char *bootarg = NULL;
>> - bool isAMD = false, isIntel = false, isPPC = false;
>> + bool isAMD = false, isIntel = false;
>> + virArch arch = virArchFromHost();
>> + struct dirent *dent;
>> + DIR *dir;
>> + int rc;
>> +
>> flags = virHostValidateGetCPUFlags();
>> if (flags && virBitmapIsBitSet(flags,
>> VIR_HOST_VALIDATE_CPU_FLAG_VMX))
>> @@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
>> virBitmapFree(flags);
>> - isPPC = ARCH_IS_PPC64(virArchFromHost());
>> -
>> if (isIntel) {
>> virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU
>> support"));
>> if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
>> @@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
>> "hardware platform");
>> return -1;
>> }
>> - } else if (isPPC) {
>> + } else if (ARCH_IS_PPC64(arch)) {
>> /* Empty Block */
>> + } else if (ARCH_IS_S390(arch)) {
>> + /* On s390x, we skip the IOMMU check if there are no PCI devices
>> + * (which is quite usual on s390x) */
>> + if (!virDirOpen(, "/sys/bus/pci/devices"))
>> + return 0;
>> + rc = virDirRead(dir, , NULL);
>> + VIR_DIR_CLOSE(dir);
>
> Is this diropen and dirread really necessary?
Yes.
> I mean, would something
> like plain test of the path presence be okay, e.g.
> virFileExists("/sys/bus/pci/devices").
No, that's unfortunately not enough. The "/sys/bus/pci/devices"
directory is always present, even if there are no PCI devices available,
it's just empty in that case. So as far as I can see, the dirread is
necessary here.
Thomas
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test: storage: Fill in default vol types for every pool
Fill in a default volume type for every pool type, as reported
by the VolGetInfo API.
Signed-off-by: Cole Robinson
---
A more complete fix would take into account VolDef->type, so test
driver users could set this value via volume XML. That would require
adding a PostParse callback to the storage driver infrastructure.
This is still an improvement in the interim and would be part of
a complete fix anyways
src/test/test_driver.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ce0df1f8e3..3f37b2ede7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol,
static int
testStorageVolumeTypeForPool(int pooltype)
{
-switch (pooltype) {
-case VIR_STORAGE_POOL_DIR:
-case VIR_STORAGE_POOL_FS:
-case VIR_STORAGE_POOL_NETFS:
-return VIR_STORAGE_VOL_FILE;
-default:
-return VIR_STORAGE_VOL_BLOCK;
+switch ((virStoragePoolType) pooltype) {
+case VIR_STORAGE_POOL_DIR:
+case VIR_STORAGE_POOL_FS:
+case VIR_STORAGE_POOL_NETFS:
+case VIR_STORAGE_POOL_VSTORAGE:
+return VIR_STORAGE_VOL_FILE;
+case VIR_STORAGE_POOL_SHEEPDOG:
+case VIR_STORAGE_POOL_ISCSI_DIRECT:
+case VIR_STORAGE_POOL_GLUSTER:
+case VIR_STORAGE_POOL_RBD:
+return VIR_STORAGE_VOL_NETWORK;
+case VIR_STORAGE_POOL_LOGICAL:
+case VIR_STORAGE_POOL_DISK:
+case VIR_STORAGE_POOL_MPATH:
+case VIR_STORAGE_POOL_ISCSI:
+case VIR_STORAGE_POOL_SCSI:
+case VIR_STORAGE_POOL_ZFS:
+case VIR_STORAGE_POOL_LAST:
+default:
+return VIR_STORAGE_VOL_BLOCK;
}
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] news: document virtio-{non-}transitional feature
Signed-off-by: Cole Robinson
---
docs/news.xml | 19 +++
1 file changed, 19 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index f58c30ee22..81466c3d55 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -47,6 +47,25 @@
virConnectGetStoragePoolCapabilites API.
+
+
+ qemu: Support virtio-{non-}transitional device models
+
+
+ virtio-transitional and
+ virtio-non-transitional model values
+ were added to the QEMU driver for the following devices:
+ disk, interface, filesystem,
+ rng, vsock, memballoon,
+ controller type scsi,
+ controller type virtio-serial,
+ input bus virtio
+ type passthrough,
+ hostdev type scsi_host. These new
+ models can be used to give fine grained control over what
+ virtio device version is presented to the guest.
+
+
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 13/18] snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag
On 3/7/19 10:13 AM, Ján Tomko wrote:
> On Mon, Mar 04, 2019 at 09:34:40PM -0600, Eric Blake wrote:
>> Continue the work of the previous patch in making it possible
>> to copy the state of a transient domain with snapshots from one
>> host to another, by allowing the destination to perform bulk
>> redefines. Note that the destination still has to do separate
>> calls for creating/defining the domain first, and then redefining
>> the snapshots (that is, there is intentional asymmetry between
>> dumping the list in virDomainGetXMLDesc() but redefining it via
>> virDomainSnapshotCreateXML()), but this is better than the
>> previous state of having to make multiple REDEFINE calls.
>
> What is the intention behind the assymetry?
virDomainSnapshotGetXMLDesc won't work - you can't pass in NULL because
that function requires a snapshot (in order to get at the virDomain and
virConnection) to even make the call.
On the flip side, I did NOT want virDomainDefine/virDomainCreate to take
the argument, even with the presence of a flag, because
there are scenarios where you want the domain defined before you add in
the snapshots; virDomainSnapshotCreateXML with new flag fit that purpose
well.
I could, however, add a new API instead of a new flag overloading to the
existing API. Naming is hard, maybe:
virDomainGetSnapshotsXMLDesc
since it would be a new virDomain function, but returns the new
XML element.
>
> It feels odd to request the list by a flag to virDomainGetXMLDesc
> (because we're not going to reuse the whole , just the
> sub-element here). Having a counterpart to the API doing the
> redefine would only mean two calls (GetXMLDesc, GetSnapshots) instead of
> getting every snapshot separately.
One call (virDomainGetXMLDesc with flag) is even better than two
(virDomainGetXMLDesc, virDomainGetSnapshotsXMLDesc), but a new API has
the benefit of not suffering from the recently-fixed bug about unknown
new flags being rejected by buggy old servers.
>
> Also, virDomainSnapshotCreateXML is designed for a single snapshot,
> using a flag to turn it into a different API
> ('virDomainSnapshotsCreateXML'?
> 'virDomainSnapshotsRedefine'?) leads to strangeness like returning
> a single snapshot while making no guarantees on which one it is
> or a repetition of this pattern:
> if (flags & REDEFINE_LIST) {
> /* ... */
> goto cleanup; /* <- no fallthrough here */
> }
If I add a new API for getting the XML, then it is not a stretch to
require a new API for redefining all snapshots at once. And now that
I've typed this up, the suggestion for a separate API is starting to be
more appealing.
Looks like I'll be posting a v4 shortly.
--
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 v3 07/18] domain: Add struct for future domain format parameters
On 3/7/19 1:14 PM, Eric Blake wrote:
> On 3/7/19 8:01 AM, John Ferlan wrote:
>
>>> Upcoming patches will add new flags for increasing the amount of
>>> information present in dumpxml, but where the source
>>> of that information is tied to somewhere other than the active
>>> or offline domain sub-definition. Make those extensions easier
>>> by updating internal callers to pass in a struct, rather than
>>> adding new parameters for each extension, so that later patches
>>> only have to patch callers that care about a new member of the
>>> struct rather than all callers.
>>>
>
>>
>> The structure formatting of _virDomainDefFormatData and friends is a bit
>> non-standard from what is typically done, but not incorrect.
>
> Do you have a pointer to a struct I should be copying from? I don't mind
> making that sort of cosmetic cleanup for code-base consistency.
>
Usually it's like...
typedef struct _virDomainDef virDomainDef;
typedef virDomainDef *virDomainDefPtr;
struct _virDomainDef {
...
};
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots
On 3/7/19 8:02 AM, John Ferlan wrote: > > > On 3/4/19 10:34 PM, Eric Blake wrote: >> Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() >> are right before freeing the virDomainSnapshotObjList, so it did not >> matter if the list's metaroot (which points to all the defined root >> snapshots) is left inconsistent. But an upcoming patch will want to >> clear all snapshots if a bulk redefine fails partway through, in >> which case things must be reset. Make this work by teaching the >> existing virDomainSnapshotUpdateRelations() to be safe regardless of >> the incoming state of the metaroot (since we don't want to leak that >> internal detail into qemu code), then fixing the qemu code to use >> it after deleting all snapshots. >> >> Signed-off-by: Eric Blake >> --- >> src/conf/snapshot_conf.c | 7 +-- >> src/qemu/qemu_domain.c | 4 >> 2 files changed, 9 insertions(+), 2 deletions(-) >> > > I have some random personal grumbling about the lack of comments for > existing code. What's described in this patch as being done would seem > correct, but my knowledge of snapshot's is 'cursory'. Yeah, and you already tasked me with refactoring virDomainSnapshotObj and virDomainCheckpointObj to share a common, better-commented, parent class. So hopefully there are patches in the next few days to improve the situation (or at least isolate it into one file instead of the current leaky abstraction where src/qemu is poking at .first_child contents in the first place). > >> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c >> index 206b05c172..386ec82d15 100644 >> --- a/src/conf/snapshot_conf.c >> +++ b/src/conf/snapshot_conf.c >> @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, >> } >> >> /* Populate parent link and child count of all snapshots, with all >> - * relations starting as 0/NULL. Return 0 on success, -1 if a parent >> - * is missing or if a circular relationship was requested. */ >> + * assigned defs having relations starting as 0/NULL. Return 0 on >> + * success, -1 if a parent is missing or if a circular relationship >> + * was requested. */ > ^^ > nit: there's an extra space here Pre-existing, and habitual, but I'll clean up the ones you or I notice. >> +++ b/src/qemu/qemu_domain.c >> @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr >> driver, >> rem.err = 0; >> virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, >> ); >> +if (rem.current) >> +vm->current_snapshot = NULL; > > If rem.err != 0, is this still something that should be done Yes. rem.current is set only if we removed the current snapshot's metadata; even if we later failed halfway through before removing ALL snapshots, keeping the current_snapshot pointer around would be pointing into free'd memory. So the pointer must be cleared whether we succeed or fail to match that we did remove that metadata (and since we don't have any good way to undo a partial failure). -- 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 v3 07/18] domain: Add struct for future domain format parameters
On 3/7/19 8:01 AM, John Ferlan wrote: >> Upcoming patches will add new flags for increasing the amount of >> information present in dumpxml, but where the source >> of that information is tied to somewhere other than the active >> or offline domain sub-definition. Make those extensions easier >> by updating internal callers to pass in a struct, rather than >> adding new parameters for each extension, so that later patches >> only have to patch callers that care about a new member of the >> struct rather than all callers. >> > > The structure formatting of _virDomainDefFormatData and friends is a bit > non-standard from what is typically done, but not incorrect. Do you have a pointer to a struct I should be copying from? I don't mind making that sort of cosmetic cleanup for code-base consistency. -- 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 v3 02/18] snapshot: Rework virDomainSnapshotState enum
On 3/7/19 9:26 AM, John Ferlan wrote: >>> + VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED, >>> + /* Additional enum values local to qemu */ >> >> As Eric Blake pointed out in an earlier iteration: >> s/qemu/snapshots/ D'oh - it's a bad sign when I miss incorporating my own review comments. >>> >>> /* virDomainSnapshotState is really virDomainState plus one extra >>> state */ >>> -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST, >>> +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST, >> >> VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix >> Indeed, but it is also longer, and runs into line-wrap issues (I'm not opposed to it, though). > > , true, but we could go with virSnapState to ensure we match the > typedef nomenclature with the enum symbol names or change the _SNAP_ to > _SNAPSHOT_ w/ virSnapshotState for the typedef - whatever is easiest. I concur with having consistency one way or another. Unless I hear a strong preference, I'll go ahead and spell the values out in full (and wrap lines) rather than trying to abbreviate the values but not the enum name. -- 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 v3 3/4] qemu_domain: NVLink2 device tree functions for PPC64
On 3/7/19 7:39 AM, Daniel Henrique Barboza wrote:
>
>
> On 3/7/19 12:15 PM, Erik Skultety wrote:
>> On Tue, Mar 05, 2019 at 09:46:08AM -0300, Daniel Henrique Barboza wrote:
>>> The NVLink2 support in QEMU implements the detection of NVLink2
>>> capable devices by verfying the attributes of the VFIO mem region
>>> QEMU allocates for the NVIDIA GPUs. To properly allocate an
>>> adequate amount of memLock, Libvirt needs this information before
>>> a QEMU instance is even created.
>>>
>>> An alternative is presented in this patch. Given a PCI device,
>>> we'll traverse the device tree at /proc/device-tree to check if
>>> the device has a NPU bridge, retrieve the node of the NVLink2 bus,
>>> find the memory-node that is related to the bus and see if it's a
>>> NVLink2 bus by inspecting its 'reg' value. This logic is contained
>>> inside the 'device_is_nvlink2_capable' function, which uses other
>>> new helper functions to navigate and fetch values from the device
>>> tree nodes.
>>>
>>> Signed-off-by: Daniel Henrique Barboza
>>> ---
>> This fails with a bunch of compilation errors, please make sure you run make
>> check and make syntax-check on each patch of the series.
>
> Ooops, forgot to follow up make syntax-check with 'make' before
> submitting ... my bad.
>
>>
>>> src/qemu/qemu_domain.c | 194 +
>>> 1 file changed, 194 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 77548c224c..97de5793e2 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -10343,6 +10343,200 @@ qemuDomainUpdateCurrentMemorySize(virDomainObjPtr
>>> vm)
>>> }
>>>
>>>
>>> +/**
>>> + * Reads a phandle file and returns the phandle value.
>>> + */
>>> +static int
>>> +read_dt_phandle(const char* file)
>> ..we prefer camelCase naming of functions...all functions should have a
>> prefix,
>> e.g. vir,, etc.
>>
>>> +{
>>> +unsigned int buf[1];
>>> +size_t read;
>>> +FILE *f;
>>> +
>>> +f = fopen(file, "r");
>>> +if (!f)
>>> +return -1;
>>> +
>>> +read = fread(buf, sizeof(unsigned int), 1, f);
>> We already have safe helpers that take care of such operations.
>>
>>> +
>>> +if (!read) {
>>> +VIR_CLOSE(f);
>> You could use VIR_AUTOCLOSE for this
>>
>>> +return 0;
>>> +}
>>> +
>>> +VIR_CLOSE(f);
>>> +return be32toh(buf[0]);
>> Is this part of gnulib? We need to make sure it's portable otherwise we'd
>> need
>> to make the conversion ourselves, just like for le64toh
>>
>>> +}
>>> +
>>> +
>>> +/**
>>> + * Reads a memory reg file and returns the first 4 int values.
>>> + *
>>> + * The caller is responsible for freeing the returned array.
>>> + */
>>> +static unsigned int *
>>> +read_dt_memory_reg(const char *file)
>>> +{
>>> +unsigned int *buf;
>>> +size_t read, i;
>>> +FILE *f;
>>> +
>>> +f = fopen(file, "r");
>>> +if (!f)
>>> +return NULL;
>>> +
>>> +if (VIR_ALLOC_N(buf, 4) < 0)
>>> +return NULL;
>>> +
>>> +read = fread(buf, sizeof(unsigned int), 4, f);
>>> +
>>> +if (!read && read < 4)
>>> +/* shouldn't happen */
>>> +VIR_FREE(buf);
>>> +else for (i = 0; i < 4; i++)
>>> +buf[i] = be32toh(buf[i]);
>>> +
>>> +VIR_CLOSE(f);
>>> +return buf;
>> Same comments as above...
>>
>>> +}
>>> +
>>> +
>>> +/**
>>> + * This wrapper function receives arguments to be used in a
>>> + * 'find' call to retrieve the file names that matches
>>> + * the criteria inside the /proc/device-tree dir.
>>> + *
>>> + * A 'find' call with '-iname phandle' inside /proc/device-tree
>>> + * provides more than a thousand matches. Adding '-path' to
>>> + * narrow it down further is necessary to keep the file
>>> + * listing sane.
>>> + *
>>> + * The caller is responsible to free the buffer returned by
>>> + * this function.
>>> + */
>>> +static char *
>>> +retrieve_dt_files_pattern(const char *path_pattern, const char
>>> *file_pattern)
>>> +{
>>> +virCommandPtr cmd = NULL;
>>> +char *output = NULL;
>>> +
>>> +cmd = virCommandNew("find");
>>> +virCommandAddArgList(cmd, "/proc/device-tree/", "-path", path_pattern,
>>> + "-iname", file_pattern, NULL);
>>> +virCommandSetOutputBuffer(cmd, );
>>> +
>>> +if (virCommandRun(cmd, NULL) < 0)
>>> +VIR_FREE(output);
>>> +
>>> +virCommandFree(cmd);
>>> +return output;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * Helper function that receives a listing of file names and
>>> + * calls read_dt_phandle() on each one finding for a match
>>> + * with the given phandle argument. Returns the file name if a
>>> + * match is found, NULL otherwise.
>>> + */
>>> +static char *
>>> +find_dt_file_with_phandle(char *files, int phandle)
>>> +{
>>> +char *line, *tmp;
>>> +int ret;
>>> +
>>> +line = strtok_r(files, "\n", );
>>> +do {
>>> + ret = read_dt_phandle(line);
>>> + if (ret == phandle)
>>> + break;
[libvirt] [PATCH] util: skip RDMA detection for non-PCI network devices
Only PCI devices have '/sys/class/net//device/resource' so we
need to skip this check for all other network devices.
Without this patch and RDMA enabled libvirt will not detect any network
device that doesn't have the path above which includes 'lo', 'virbr',
'tun', etc.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1639258
Signed-off-by: Pavel Hrdina
---
src/util/virnetdev.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index aeb9caab2a..699f2a0acb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2914,8 +2914,14 @@ virNetDevRDMAFeature(const char *ifname,
if (virAsprintf(_devpath, SYSFS_NET_DIR "%s/device/resource", ifname)
< 0)
goto cleanup;
-if (!virFileExists(eth_devpath))
+
+/* If /sys/class/net//device/resource doesn't exist it is not a PCI
+ * device and therefore it will not have RDMA. */
+if (!virFileExists(eth_devpath)) {
+ret = 0;
goto cleanup;
+}
+
if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, _res_buf) < 0)
goto cleanup;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
On 3/1/19 12:10 PM, Thomas Huth wrote:
When running virt-host-validate on an s390x host, the tool currently warns
that it is "Unknown if this platform has IOMMU support". We can use the
common check for entries in /sys/kernel/iommu_groups here, too, but it only
makes sense to check it if there are also PCI devices available. It's also
common on s390x that there are no PCI devices assigned to the LPAR, and in
that case there is no need for the PCI-related IOMMU, so without PCI devices
we should simply skip this test.
Signed-off-by: Thomas Huth
---
v2:
- Use virDirOpen() + virDirRead() instead of stat() - this should hopefully
work now as expected.
tools/virt-host-validate-common.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/tools/virt-host-validate-common.c
b/tools/virt-host-validate-common.c
index 73e3bdb..e27b558 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
virBitmapPtr flags;
struct stat sb;
const char *bootarg = NULL;
-bool isAMD = false, isIntel = false, isPPC = false;
+bool isAMD = false, isIntel = false;
+virArch arch = virArchFromHost();
+struct dirent *dent;
+DIR *dir;
+int rc;
+
flags = virHostValidateGetCPUFlags();
if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))
@@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
virBitmapFree(flags);
-isPPC = ARCH_IS_PPC64(virArchFromHost());
-
if (isIntel) {
virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU
support"));
if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
@@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
"hardware platform");
return -1;
}
-} else if (isPPC) {
+} else if (ARCH_IS_PPC64(arch)) {
/* Empty Block */
+} else if (ARCH_IS_S390(arch)) {
+/* On s390x, we skip the IOMMU check if there are no PCI devices
+ * (which is quite usual on s390x) */
+if (!virDirOpen(, "/sys/bus/pci/devices"))
+return 0;
+rc = virDirRead(dir, , NULL);
+VIR_DIR_CLOSE(dir);
Is this diropen and dirread really necessary? I mean, would something
like plain test of the path presence be okay, e.g.
virFileExists("/sys/bus/pci/devices").
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] LXC containers don't stopped under some conditions
On 3/6/19 7:39 PM, Maxim Kozin wrote:
LXC containers can be started, but time to time can't be stopped.
1) virsh shutdown with option "--mode initctl" always stop container, but
virsh report error:
"Container does not provide an initctl pipe"
In container present /dev/initctl
2) virsh shutdown with option "--mode signal" never stop lxc container, at
least for centos 7.5/7.6
In container log:
systemd: Received SIGTERM.
systemd: Reexecuting.
systemd: systemd 219 running in system mode. (+PAM +AUDIT +SELINUX +IMA
-APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS
+ACL +XZ +LZ4 -SECCOMP +BLKID +ELFUTILS +KMOD +IDN)
systemd: Detected virtualization lxc-libvirt.
systemd: Detected architecture x86-64.
3) virsh shutdown without option "--mode" can stop lxc container, but in 2
cases:
- cointainer not under heavy load
- in gdb when perform step-by-setp debug
But most often not, with simptoms as with --mode signal.
Patch tested only with host centos 7.6 and guests centos 7.5/7.6
Short comments to patch.
lxcDomainShutdownFlags() return rc=0 and container begin stopped.
We not go to endjob label, but later we pass check:
if (rc == 0 &&
(flags == 0 ||
(flags & VIR_DOMAIN_SHUTDOWN_SIGNAL))) {
And trying next shutdown method with sigterm to PID 1.
If container heavy loaded, it not stopped. IF not or if you wait in gdb, then
first method succefully perform shutdown.
Signed-off-by: Maxim Kozin
---
src/lxc/lxc_driver.c | 40 ++--
1 file changed, 18 insertions(+), 22 deletions(-)
I've reworeded the commit message, ACKed and pushed.
Congratulations on your first libvirt contribution!
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Remove unnecessary @ctxt check in virSecurityLabelDefsParseXML
s/@ctxt check/checks/ On Thu, Mar 07, 2019 at 11:25:18AM -0500, John Ferlan wrote: Failure would have occurred before in callers other virXPath calls. Found by Coverity due to commit 66a508d2 using VIR_XPATH_NODE_AUTORESTORE to access @ctxt before the if condition. Signed-off-by: John Ferlan --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eb660f5764..dc57b23084 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8516,8 +8516,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, virCapsHostPtr host = >host; VIR_AUTOFREE(xmlNodePtr *) list = NULL; -/* Check args and save context */ -if (def == NULL || ctxt == NULL) +if (def == NULL) return 0; Both now and at the point of its introduction in: commit e9377dda367b847b5a15dac1403bcdf19f05438a Multiple security drivers in XML data def is dereferenced right before its only caller so you can delete the whole condition. Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] util: Tweak virStringMatchesNameSuffix() some more
On Thu, Mar 07, 2019 at 11:14:46 +0100, Andrea Bolognani wrote: > We can use STRNEQ() instead of STRNEQLEN() since we're only > interested in the trailing part of the string. > > Signed-off-by: Andrea Bolognani > --- > src/util/virstring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/util/virstring.c b/src/util/virstring.c > index ba36562f85..f23daca0bd 100644 > --- a/src/util/virstring.c > +++ b/src/util/virstring.c > @@ -1300,7 +1300,7 @@ virStringMatchesNameSuffix(const char *file, > if (STRNEQLEN(file, name, namelen)) > return false; > > -if (STRNEQLEN(file + namelen, suffix, suffixlen)) > +if (STRNEQ(file + namelen, suffix)) > return false; ACK to this if you rebase it without the previous patches. The change is okay as we verify that filelen == (namelen + suffixlen) prior to attempting this. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Remove unnecessary @ctxt check in virSecurityLabelDefsParseXML
Failure would have occurred before in callers other virXPath calls. Found by Coverity due to commit 66a508d2 using VIR_XPATH_NODE_AUTORESTORE to access @ctxt before the if condition. Signed-off-by: John Ferlan --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eb660f5764..dc57b23084 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8516,8 +8516,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, virCapsHostPtr host = >host; VIR_AUTOFREE(xmlNodePtr *) list = NULL; -/* Check args and save context */ -if (def == NULL || ctxt == NULL) +if (def == NULL) return 0; /* Allocate a security labels based on XML */ -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 13/18] snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag
On Mon, Mar 04, 2019 at 09:34:40PM -0600, Eric Blake wrote:
Continue the work of the previous patch in making it possible
to copy the state of a transient domain with snapshots from one
host to another, by allowing the destination to perform bulk
redefines. Note that the destination still has to do separate
calls for creating/defining the domain first, and then redefining
the snapshots (that is, there is intentional asymmetry between
dumping the list in virDomainGetXMLDesc() but redefining it via
virDomainSnapshotCreateXML()), but this is better than the
previous state of having to make multiple REDEFINE calls.
What is the intention behind the assymetry?
It feels odd to request the list by a flag to virDomainGetXMLDesc
(because we're not going to reuse the whole , just the
sub-element here). Having a counterpart to the API doing the
redefine would only mean two calls (GetXMLDesc, GetSnapshots) instead of
getting every snapshot separately.
Also, virDomainSnapshotCreateXML is designed for a single snapshot,
using a flag to turn it into a different API ('virDomainSnapshotsCreateXML'?
'virDomainSnapshotsRedefine'?) leads to strangeness like returning
a single snapshot while making no guarantees on which one it is
or a repetition of this pattern:
if (flags & REDEFINE_LIST) {
/* ... */
goto cleanup; /* <- no fallthrough here */
}
Jano
The
bulk flag requires no pre-existing snapshot metadata (as that
makes life much easier if there is a failure encountered partway
through the list processing - simply remove all other snapshot
metadatas), and makes no guarantees on which snapshot (when there
are multiple) will actually be returned.
Actual driver implementations will be in later patches.
Signed-off-by: Eric Blake
Reviewed-by: John Ferlan
---
include/libvirt/libvirt-domain-snapshot.h | 3 +++
src/libvirt-domain-snapshot.c | 23 +++
2 files changed, 22 insertions(+), 4 deletions(-)
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/4] PPC64 support for NVIDIA V100 GPU with NVLink2 passthrough
Erik, It appears that patches 1 and 2 are good to go (and probably can be pushed separately from the other 2). Should I resend only patches 3 and 4 when I get more info from NVIDIA about the detection code in patch 3? Thanks, Daniel On 3/5/19 9:46 AM, Daniel Henrique Barboza wrote: This series includes Libvirt support for a new QEMU feature for the spapr (PPC64) machine, NVIDIA V100 + P9 passthrough. Refer to [1] for the version 3 of this feature (same version used as a reference for this series). Changes in v3: - added a new patch (patch 2) that isolates the PPC64 exclusive code to calculate the memLockLimit (suggested by Erik Skultety) - fixed 'make syntax-check' errors across all patches - v2 can be found at [2] [1] https://patchwork.kernel.org/cover/10831413/ [2] https://www.redhat.com/archives/libvir-list/2019-March/msg00059.html Daniel Henrique Barboza (4): qemu_domain: simplify non-VFIO memLockLimit calc for PPC64 qemu_domain: add a PPC64 memLockLimit helper qemu_domain: NVLink2 device tree functions for PPC64 PPC64 support for NVIDIA V100 GPU with NVLink2 passthrough src/qemu/qemu_domain.c | 402 - 1 file changed, 321 insertions(+), 81 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix query-cpus-fast target architecture detection
On 3/1/19 11:29 AM, Boris Fiuczynski wrote: From: Viktor Mihajlovski Since qemu 2.13 reports the target architecture in a property called 'target' additionally to the property 'arch', that has been used in qemu 2.12 in the response data of 'query-cpus-fast'. Libvirts monitor code prefers the 'target' property over 'arch'. At least for s390(x), target is reported as 's390x' while arch is 's390'. In a later step a comparison is performed against 's390' which fails for qemu 2.13 and later. In consequence the architecture specific data for s390 won't be extracted from the returned data, leading to incorrect values being reported by virsh domstats --vcpu. Changing to check explicitly for 's390' and 's390x'. Signed-off-by: Viktor Mihajlovski Reviewed-by: Bjoern Walk Reviewed-by: Boris Fiuczynski --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cf474eb6c8..0236323a51 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1772,7 +1772,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, goto cleanup; /* process optional architecture-specific data */ -if (STREQ_NULLABLE(arch, "s390")) +if (STREQ_NULLABLE(arch, "s390") || STREQ_NULLABLE(arch, "s390x")) qemuMonitorJSONExtractCPUS390Info(entry, cpus + i); } ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/18] qemu: Implement bulk snapshot operations
On 3/4/19 10:34 PM, Eric Blake wrote:
> Implement the new flags for bulk snapshot dump and redefine. This
> borrows from ideas in the test driver, but is further complicated
> by the fact that qemu must write snapshot XML to disk, and thus must
> do additional validation after the initial parse to ensure the user
> didn't attempt to rename a snapshot with "../" or similar.
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_domain.c | 26 +
> src/qemu/qemu_driver.c | 66 --
> 2 files changed, 83 insertions(+), 9 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 39cc45537d..6a8f8e2bbe 100644
[...]
> static virDomainSnapshotPtr
> qemuDomainSnapshotCreateXML(virDomainPtr domain,
> const char *xmlDesc,
> @@ -15765,7 +15792,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
>VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
>VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
> - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
> + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
> + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);
>
> VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
> VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
> @@ -15797,6 +15825,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> goto cleanup;
> }
>
> +if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
> +struct qemuDomainSnapshotBulk bulk = {
> +.vm = vm,
> +.driver = driver,
> +.snapshotDir = cfg->snapshotDir,
> +.flags = flags,
> +};
> +
> +if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid,
> + vm->snapshots,
> >current_snapshot,
> + caps, driver->xmlopt,
> + parse_flags) < 0)
> +goto cleanup;
> +/* Validate and save the snapshots to disk. Since we don't get
> + * here unless there were no snapshots beforehand, just delete
> + * everything if anything failed, ignoring further errors. */
> +if (virDomainSnapshotForEach(vm->snapshots,
> + qemuDomainSnapshotBulkRedefine,
> + ) < 0) {
> +virErrorPtr orig_err = virSaveLastError();
I've seen newer code using virErrorPreserveLast
> +
> +qemuDomainSnapshotDiscardAllMetadata(driver, vm);
> +virSetError(orig_err);
> +virFreeError(orig_err);
Similar newer code using virErrorRestore instead of both
> +goto cleanup;
> +}
> +/* Return is arbitrary, so use the first root */
> +snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
> +snapshot = virGetDomainSnapshot(domain,
> snap->first_child->def->name);
> +goto cleanup;
> +}
> +
> if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot halt after transient domain snapshot"));
>
Reviewed-by: John Ferlan
John
FYI: I ran the series through Coverity too with no new errors
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] numa: warn if numa 'mem' option or default RAM splitting between nodes is used.
On Thu, 7 Mar 2019 10:04:56 +
Daniel P. Berrangé wrote:
> On Wed, Mar 06, 2019 at 07:48:22PM +0100, Igor Mammedov wrote:
> > On Wed, 6 Mar 2019 17:10:37 +
> > Daniel P. Berrangé wrote:
> >
> > > On Wed, Mar 06, 2019 at 05:58:35PM +0100, Igor Mammedov wrote:
> > > > On Wed, 6 Mar 2019 16:39:38 +
> > > > Daniel P. Berrangé wrote:
> > > >
> > > > > On Wed, Mar 06, 2019 at 05:30:25PM +0100, Igor Mammedov wrote:
> > > > > > Ammend -numa option docs and print warnings if 'mem' option or
> > > > > > default RAM
> > > > > > splitting between nodes is used. It's intended to discourage users
> > > > > > from using
> > > > > > configuration that allows only to fake NUMA on guest side while
> > > > > > leading
> > > > > > to reduced performance of the guest due to inability to properly
> > > > > > configure
> > > > > > VM's RAM on the host.
> > > > > >
> > > > > > In NUMA case, it's recommended to always explicitly configure guest
> > > > > > RAM
> > > > > > using -numa node,memdev={backend-id} option.
> > > > > >
> > > > > > Signed-off-by: Igor Mammedov
> > > > > > ---
> > > > > > numa.c | 5 +
> > > > > > qemu-options.hx | 12
> > > > > > 2 files changed, 13 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/numa.c b/numa.c
> > > > > > index 3875e1e..c6c2a6f 100644
> > > > > > --- a/numa.c
> > > > > > +++ b/numa.c
> > > > > > @@ -121,6 +121,8 @@ static void parse_numa_node(MachineState *ms,
> > > > > > NumaNodeOptions *node,
> > > > > >
> > > > > > if (node->has_mem) {
> > > > > > numa_info[nodenr].node_mem = node->mem;
> > > > > > +warn_report("Parameter -numa node,mem is obsolete,"
> > > > > > +" use -numa node,memdev instead");
> > > > >
> > > > > I don't think we should do this. Libvirt isn't going to stop using
> > > > > this
> > > > > option in the near term. When users see warnings like this in logs
> > > > well when it was the only option available libvirt had no other choice,
> > > > but since memdev became available libvirt should try to use it whenever
> > > > possible.
> > >
> > > As we previously discussed, it is not possible for libvirt to use it
> > > in all cases.
> > >
> > > >
> > > > > they'll often file bugs reports thinking something is broken which is
> > > > > not the case here.
> > > > It's the exact purpose of the warning, to force user asking questions
> > > > and fix configuration, since he/she obviously not getting NUMA benefits
> > > > and/or performance-wise
> > >
> > > That's only useful if it is possible to do something about the problem.
> > > Libvirt wants to use the new option but it can't due to the live migration
> > > problems. So this simply leads to bug reports that will end up marked
> > > as CANTFIX.
> > The problem could be solved by user though, by reconfiguring and restarting
> > domain since it's impossible to (at least as it stands now wrt migration).
> >
> > > I don't believe libvirt actually suffers from the performance problem
> > > you describe wrt lack of pinning. When we attempt to pin guest NUMA
> > > nodes to host NUMA nodes, libvirt *will* use "memdev". IIUC, we
> > > use "mem" in the case where there /no/ requested pinning of guest
> > > NUMA nodes, and so we're not suffering from the limitations of "mem"
> > > in that case.
> > What would be the use-case for not pinning numa nodes?
> > If user isn't asking for pinning, VM would run with degraded performance and
> > it would be better of being non-numa.
>
> The guest could have been originally booted on a host which has 2 NUMA
> nodes and have been migrated to a host with 1 NUMA node, in which case
> pinnning is not relevant.
>
> For CI purposes too it is reasonable to create guests with NUMA configurations
> that bear no resemblance to the host NUMA configuration. This allows for
> testing
> the operation of guest applications. This is something that is relevant to
> OpenStack for testnig Nova's handling of NUMA placement logic, since almost
> all their testing is in VMs not bare metal.
>
> Not pinning isn't common, but it is reasonable to do it.
> > Even if user doesn't ask for pinning, non-pinned memdev (for new VMs where
> > available) could be used as well. Users of 'mem' with new QEMU will see the
> > warning
> > and probably think about if they are configured VM correctly.
>
> We can't change to memdev due to migration problems. Since there is no
> functional problem with using 'mem' in this scenario there's no pressing
> reason to stop using 'mem'.
The problem with it is that it blocks fixing bug in QEMU for the multi node case
'-numa node,mem + -mem-path + -mem-prealloc + -object "memdev",policy=bind'
and it also gets in the way of unifying RAM handling using device-memory
for initial RAM due to the same migration issue since RAM layout in migration
stream is different.
> > Desire to deprecate 'mem' at least for new machines is to be
[libvirt] [PATCH 8/8] tests: Use testQemuCapsIterate()
With only a couple minor tweaks, we can make the existing
doCapsTest() functions with testQemuCapsIterate() and finally
remove the need to manually adjust the test programs every time
a new input file is introduced; moreover, this means that the
two lists can't possibly get out of sync anymore.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 48 +++-
tests/qemucaps2xmltest.c | 48 +++-
2 files changed, 8 insertions(+), 88 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index b4ed081d3e..16c2832ffb 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -179,8 +179,9 @@ testQemuCapsCopy(const void *opaque)
static int
doCapsTest(const char *base,
const char *archName,
- testQemuDataPtr data)
+ void *opaque)
{
+testQemuDataPtr data = (testQemuDataPtr) opaque;
VIR_AUTOFREE(char *) title = NULL;
VIR_AUTOFREE(char *) copyTitle = NULL;
@@ -220,49 +221,8 @@ mymain(void)
if (testQemuDataInit() < 0)
return EXIT_FAILURE;
-#define DO_TEST(arch, name) \
-do { \
-if (doCapsTest(name, arch, ) < 0) \
-return EXIT_FAILURE; \
-} while (0)
-
-/* Keep this in sync with qemucaps2xmltest */
-DO_TEST("x86_64", "caps_1.5.3");
-DO_TEST("x86_64", "caps_1.6.0");
-DO_TEST("x86_64", "caps_1.7.0");
-DO_TEST("x86_64", "caps_2.1.1");
-DO_TEST("x86_64", "caps_2.4.0");
-DO_TEST("x86_64", "caps_2.5.0");
-DO_TEST("x86_64", "caps_2.6.0");
-DO_TEST("x86_64", "caps_2.7.0");
-DO_TEST("x86_64", "caps_2.8.0");
-DO_TEST("x86_64", "caps_2.9.0");
-DO_TEST("x86_64", "caps_2.10.0");
-DO_TEST("x86_64", "caps_2.11.0");
-DO_TEST("x86_64", "caps_2.12.0");
-DO_TEST("x86_64", "caps_3.0.0");
-DO_TEST("x86_64", "caps_3.1.0");
-DO_TEST("x86_64", "caps_4.0.0");
-DO_TEST("aarch64", "caps_2.6.0");
-DO_TEST("aarch64", "caps_2.10.0");
-DO_TEST("aarch64", "caps_2.12.0");
-DO_TEST("ppc64", "caps_2.6.0");
-DO_TEST("ppc64", "caps_2.9.0");
-DO_TEST("ppc64", "caps_2.10.0");
-DO_TEST("ppc64", "caps_2.12.0");
-DO_TEST("ppc64", "caps_3.0.0");
-DO_TEST("ppc64", "caps_3.1.0");
-DO_TEST("s390x", "caps_2.7.0");
-DO_TEST("s390x", "caps_2.8.0");
-DO_TEST("s390x", "caps_2.9.0");
-DO_TEST("s390x", "caps_2.10.0");
-DO_TEST("s390x", "caps_2.11.0");
-DO_TEST("s390x", "caps_2.12.0");
-DO_TEST("s390x", "caps_3.0.0");
-DO_TEST("riscv32", "caps_3.0.0");
-DO_TEST("riscv32", "caps_4.0.0");
-DO_TEST("riscv64", "caps_3.0.0");
-DO_TEST("riscv64", "caps_4.0.0");
+if (testQemuCapsIterate(data.dataDir, ".replies", doCapsTest, ) < 0)
+return EXIT_FAILURE;
/*
* Run "tests/qemucapsprobe /path/to/qemu/binary >foo.replies"
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index dff4e2a884..e21fde7e0b 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -182,8 +182,9 @@ testQemuCapsXML(const void *opaque)
static int
doCapsTest(const char *base,
const char *archName,
- testQemuDataPtr data)
+ void *opaque)
{
+testQemuDataPtr data = (testQemuDataPtr) opaque;
VIR_AUTOFREE(char *) title = NULL;
if (virAsprintf(, "%s (%s)", base, archName) < 0)
@@ -216,49 +217,8 @@ mymain(void)
if (testQemuDataInit() < 0)
return EXIT_FAILURE;
-#define DO_TEST(arch, name) \
-do { \
-if (doCapsTest(name, arch, ) < 0) \
-return EXIT_FAILURE; \
-} while (0)
-
-/* Keep this in sync with qemucapabilitiestest */
-DO_TEST("x86_64", "caps_1.5.3");
-DO_TEST("x86_64", "caps_1.6.0");
-DO_TEST("x86_64", "caps_1.7.0");
-DO_TEST("x86_64", "caps_2.1.1");
-DO_TEST("x86_64", "caps_2.4.0");
-DO_TEST("x86_64", "caps_2.5.0");
-DO_TEST("x86_64", "caps_2.6.0");
-DO_TEST("x86_64", "caps_2.7.0");
-DO_TEST("x86_64", "caps_2.8.0");
-DO_TEST("x86_64", "caps_2.9.0");
-DO_TEST("x86_64", "caps_2.10.0");
-DO_TEST("x86_64", "caps_2.11.0");
-DO_TEST("x86_64", "caps_2.12.0");
-DO_TEST("x86_64", "caps_3.0.0");
-DO_TEST("x86_64", "caps_3.1.0");
-DO_TEST("x86_64", "caps_4.0.0");
-DO_TEST("aarch64", "caps_2.6.0");
-DO_TEST("aarch64", "caps_2.10.0");
-DO_TEST("aarch64", "caps_2.12.0");
-DO_TEST("ppc64", "caps_2.6.0");
-DO_TEST("ppc64", "caps_2.9.0");
-DO_TEST("ppc64", "caps_2.10.0");
-DO_TEST("ppc64", "caps_2.12.0");
-DO_TEST("ppc64", "caps_3.0.0");
-DO_TEST("ppc64", "caps_3.1.0");
-DO_TEST("s390x", "caps_2.7.0");
-DO_TEST("s390x", "caps_2.8.0");
-DO_TEST("s390x", "caps_2.9.0");
-DO_TEST("s390x", "caps_2.10.0");
-DO_TEST("s390x", "caps_2.11.0");
-DO_TEST("s390x", "caps_2.12.0");
-DO_TEST("s390x", "caps_3.0.0");
-DO_TEST("riscv32", "caps_3.0.0");
-DO_TEST("riscv32", "caps_4.0.0");
-
[libvirt] [PATCH 7/8] tests: Introduce testQemuCapsIterate()
This function iterates over a directory containing
capabilities-related data, extract some useful bits of
information from the file name, and calls a user-provided
callback.
Signed-off-by: Andrea Bolognani
---
tests/testutilsqemu.c | 53 +++
tests/testutilsqemu.h | 8 +++
2 files changed, 61 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 61bf67d5ad..407cf91925 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -865,3 +865,56 @@ testQemuGetLatestCapsForArch(const char *dirname,
virDirClose();
return ret;
}
+
+
+int
+testQemuCapsIterate(const char *dirname,
+const char *suffix,
+testQemuCapsIterateCallback callback,
+void *opaque)
+{
+struct dirent *ent;
+DIR *dir = NULL;
+int rc;
+int ret = -1;
+
+if (!callback)
+return 0;
+
+if (virDirOpen(, dirname) < 0)
+goto cleanup;
+
+while ((rc = virDirRead(dir, , dirname) > 0)) {
+char *tmp = ent->d_name;
+char *base = NULL;
+char *archName = NULL;
+
+/* Strip the trailing suffix, moving on if it's not present */
+if (!virStringStripSuffix(tmp, suffix))
+continue;
+
+/* Find the last dot, moving on if none is present */
+if (!(archName = strrchr(tmp, '.')))
+continue;
+
+/* The base name is everything before the last dot, and
+ * the architecture name everything after it */
+base = tmp;
+archName[0] = '\0';
+archName++;
+
+/* Run the user-provided callback */
+if (callback(base, archName, opaque) < 0)
+goto cleanup;
+}
+
+if (rc < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virDirClose();
+
+return ret;
+}
diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
index 5ae7f19473..183ce915f1 100644
--- a/tests/testutilsqemu.h
+++ b/tests/testutilsqemu.h
@@ -63,6 +63,14 @@ char *testQemuGetLatestCapsForArch(const char *dirname,
const char *arch,
const char *suffix);
+typedef int (*testQemuCapsIterateCallback)(const char *base,
+ const char *archName,
+ void *opaque);
+int testQemuCapsIterate(const char *dirname,
+const char *suffix,
+testQemuCapsIterateCallback callback,
+void *opaque);
+
# endif
#endif /* LIBVIRT_TESTUTILSQEMU_H */
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/8] tests: Introduce testQemuDataInit() and testQemuDataReset()
These functions don't do anything too interesting right now,
but will be extended significantly later on.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 25 ++---
tests/qemucaps2xmltest.c | 16
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 8d47133e6f..882fa57485 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -40,6 +40,23 @@ struct _testQemuData {
};
+static int
+testQemuDataInit(testQemuDataPtr data)
+{
+if (qemuTestDriverInit(>driver) < 0)
+return -1;
+
+return 0;
+}
+
+
+static void
+testQemuDataReset(testQemuDataPtr data)
+{
+qemuTestDriverFree(>driver);
+}
+
+
static int
testQemuCaps(const void *opaque)
{
@@ -164,12 +181,14 @@ mymain(void)
return EXIT_AM_SKIP;
#endif
-if (virThreadInitialize() < 0 ||
-qemuTestDriverInit() < 0)
+if (virThreadInitialize() < 0)
return EXIT_FAILURE;
virEventRegisterDefaultImpl();
+if (testQemuDataInit() < 0)
+return EXIT_FAILURE;
+
#define DO_TEST(arch, name) \
do { \
data.archName = arch; \
@@ -227,7 +246,7 @@ mymain(void)
* "tests/qemucapsfixreplies foo.replies" to fix the replies ids.
*/
-qemuTestDriverFree();
+testQemuDataReset();
return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 0d9b4e679a..5cc9fb635b 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -32,6 +32,17 @@ struct _testQemuData {
const char *archName;
};
+static int
+testQemuDataInit(testQemuDataPtr data ATTRIBUTE_UNUSED)
+{
+return 0;
+}
+
+static void
+testQemuDataReset(testQemuDataPtr data ATTRIBUTE_UNUSED)
+{
+}
+
static virQEMUCapsPtr
testQemuGetCaps(char *caps)
{
@@ -176,6 +187,9 @@ mymain(void)
virEventRegisterDefaultImpl();
+if (testQemuDataInit() < 0)
+return EXIT_FAILURE;
+
#define DO_TEST(arch, name) \
data.archName = arch; \
data.base = name; \
@@ -220,6 +234,8 @@ mymain(void)
DO_TEST("riscv64", "caps_3.0.0");
DO_TEST("riscv64", "caps_4.0.0");
+testQemuDataReset();
+
return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/8] tests: Add testutilsqemu dependency for qemucaps2xmltest
We're not using any of the functionality offered by the module at the moment, but we will in just a second. Signed-off-by: Andrea Bolognani --- tests/Makefile.am| 1 + tests/qemucaps2xmltest.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 72f0420bab..46fa50cc0a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -639,6 +639,7 @@ qemucommandutiltest_LDADD = libqemumonitortestutils.la \ qemucaps2xmltest_SOURCES = \ qemucaps2xmltest.c \ testutils.c testutils.h \ + testutilsqemu.c testutilsqemu.h \ $(NULL) qemucaps2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 4f9cfc459e..dff4e2a884 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -19,6 +19,7 @@ #include #include "testutils.h" +#include "testutilsqemu.h" #include "qemu/qemu_capabilities.h" -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/8] tests: Move code from DO_TEST() to doCapsTest()
This removes the awkard escaping and will allow us to perform
some interesting refactoring later on.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 40 +---
tests/qemucaps2xmltest.c | 28 ++---
2 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 222ac05d79..b4ed081d3e 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -176,6 +176,32 @@ testQemuCapsCopy(const void *opaque)
}
+static int
+doCapsTest(const char *base,
+ const char *archName,
+ testQemuDataPtr data)
+{
+VIR_AUTOFREE(char *) title = NULL;
+VIR_AUTOFREE(char *) copyTitle = NULL;
+
+if (virAsprintf(, "%s (%s)", base, archName) < 0 ||
+virAsprintf(, "copy %s (%s)", base, archName) < 0) {
+return -1;
+}
+
+data->base = base;
+data->archName = archName;
+
+if (virTestRun(title, testQemuCaps, data) < 0)
+data->ret = -1;
+
+if (virTestRun(copyTitle, testQemuCapsCopy, data) < 0)
+data->ret = -1;
+
+return 0;
+}
+
+
static int
mymain(void)
{
@@ -196,18 +222,8 @@ mymain(void)
#define DO_TEST(arch, name) \
do { \
-VIR_AUTOFREE(char *) title = NULL; \
-VIR_AUTOFREE(char *) copyTitle = NULL; \
-if (virAsprintf(, "%s (%s)", name, arch) < 0 || \
-virAsprintf(, "copy %s (%s)", name, arch) < 0) { \
-return -EXIT_FAILURE; \
-} \
-data.archName = arch; \
-data.base = name; \
-if (virTestRun(title, testQemuCaps, ) < 0) \
-data.ret = -1; \
-if (virTestRun(copyTitle, testQemuCapsCopy, ) < 0) \
-data.ret = -1; \
+if (doCapsTest(name, arch, ) < 0) \
+return EXIT_FAILURE; \
} while (0)
/* Keep this in sync with qemucaps2xmltest */
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index be460b42f8..4f9cfc459e 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -178,6 +178,25 @@ testQemuCapsXML(const void *opaque)
return ret;
}
+static int
+doCapsTest(const char *base,
+ const char *archName,
+ testQemuDataPtr data)
+{
+VIR_AUTOFREE(char *) title = NULL;
+
+if (virAsprintf(, "%s (%s)", base, archName) < 0)
+return -1;
+
+data->base = base;
+data->archName = archName;
+
+if (virTestRun(title, testQemuCapsXML, data) < 0)
+data->ret = -1;
+
+return 0;
+}
+
static int
mymain(void)
{
@@ -198,13 +217,8 @@ mymain(void)
#define DO_TEST(arch, name) \
do { \
-VIR_AUTOFREE(char *) title = NULL; \
-if (virAsprintf(, "%s (%s)", name, arch) < 0) \
-return -EXIT_FAILURE; \
-data.archName = arch; \
-data.base = name; \
-if (virTestRun(title, testQemuCapsXML, ) < 0) \
-data.ret = -1; \
+if (doCapsTest(name, arch, ) < 0) \
+return EXIT_FAILURE; \
} while (0)
/* Keep this in sync with qemucapabilitiestest */
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/8] tests: Use virAsprintf() to build titles
We're using static string concatenation at the moment, but
that will no longer be a possibility in a bit.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 11 ---
tests/qemucaps2xmltest.c | 13 +
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index e3c6681dd4..222ac05d79 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -196,12 +196,17 @@ mymain(void)
#define DO_TEST(arch, name) \
do { \
+VIR_AUTOFREE(char *) title = NULL; \
+VIR_AUTOFREE(char *) copyTitle = NULL; \
+if (virAsprintf(, "%s (%s)", name, arch) < 0 || \
+virAsprintf(, "copy %s (%s)", name, arch) < 0) { \
+return -EXIT_FAILURE; \
+} \
data.archName = arch; \
data.base = name; \
-if (virTestRun(name "(" arch ")", testQemuCaps, ) < 0) \
+if (virTestRun(title, testQemuCaps, ) < 0) \
data.ret = -1; \
-if (virTestRun("copy " name "(" arch ")", \
- testQemuCapsCopy, ) < 0) \
+if (virTestRun(copyTitle, testQemuCapsCopy, ) < 0) \
data.ret = -1; \
} while (0)
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 46d2ce8b44..be460b42f8 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -197,10 +197,15 @@ mymain(void)
return EXIT_FAILURE;
#define DO_TEST(arch, name) \
-data.archName = arch; \
-data.base = name; \
-if (virTestRun(name "(" arch ")", testQemuCapsXML, ) < 0) \
-data.ret = -1
+do { \
+VIR_AUTOFREE(char *) title = NULL; \
+if (virAsprintf(, "%s (%s)", name, arch) < 0) \
+return -EXIT_FAILURE; \
+data.archName = arch; \
+data.base = name; \
+if (virTestRun(title, testQemuCapsXML, ) < 0) \
+data.ret = -1; \
+} while (0)
/* Keep this in sync with qemucapabilitiestest */
DO_TEST("x86_64", "caps_1.5.3");
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/8] tests: Move data directories into testQemuData
This removes a little duplication right away, and more
importantly will allow us to perform some interesting
refactoring later on.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 15 +--
tests/qemucaps2xmltest.c | 13 +
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 0f875f9e24..e3c6681dd4 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -35,6 +35,7 @@ typedef struct _testQemuData testQemuData;
typedef testQemuData *testQemuDataPtr;
struct _testQemuData {
virQEMUDriver driver;
+const char *dataDir;
const char *archName;
const char *base;
int ret;
@@ -47,6 +48,8 @@ testQemuDataInit(testQemuDataPtr data)
if (qemuTestDriverInit(>driver) < 0)
return -1;
+data->dataDir = abs_srcdir "/qemucapabilitiesdata";
+
data->ret = 0;
return 0;
@@ -73,10 +76,10 @@ testQemuCaps(const void *opaque)
unsigned int fakeMicrocodeVersion = 0;
const char *p;
-if (virAsprintf(, "%s/qemucapabilitiesdata/%s.%s.replies",
-abs_srcdir, data->base, data->archName) < 0 ||
-virAsprintf(, "%s/qemucapabilitiesdata/%s.%s.xml",
-abs_srcdir, data->base, data->archName) < 0)
+if (virAsprintf(, "%s/%s.%s.replies",
+data->dataDir, data->base, data->archName) < 0 ||
+virAsprintf(, "%s/%s.%s.xml",
+data->dataDir, data->base, data->archName) < 0)
goto cleanup;
if (!(mon = qemuMonitorTestNewFromFileFull(repliesFile, >driver,
NULL)))
@@ -141,8 +144,8 @@ testQemuCapsCopy(const void *opaque)
virQEMUCapsPtr copy = NULL;
char *actual = NULL;
-if (virAsprintf(, "%s/qemucapabilitiesdata/%s.%s.xml",
-abs_srcdir, data->base, data->archName) < 0)
+if (virAsprintf(, "%s/%s.%s.xml",
+data->dataDir, data->base, data->archName) < 0)
goto cleanup;
if (!(caps = virCapabilitiesNew(virArchFromString(data->archName),
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index e9b0b11e35..46d2ce8b44 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -28,6 +28,8 @@
typedef struct _testQemuData testQemuData;
typedef testQemuData *testQemuDataPtr;
struct _testQemuData {
+const char *inputDir;
+const char *outputDir;
const char *base;
const char *archName;
int ret;
@@ -36,6 +38,9 @@ struct _testQemuData {
static int
testQemuDataInit(testQemuDataPtr data)
{
+data->inputDir = abs_srcdir "/qemucapabilitiesdata";
+data->outputDir = abs_srcdir "/qemucaps2xmloutdata";
+
data->ret = 0;
return 0;
@@ -142,12 +147,12 @@ testQemuCapsXML(const void *opaque)
char *capsXml = NULL;
virCapsPtr capsProvided = NULL;
-if (virAsprintf(, "%s/qemucaps2xmloutdata/caps.%s.xml",
-abs_srcdir, data->archName) < 0)
+if (virAsprintf(, "%s/caps.%s.xml",
+data->outputDir, data->archName) < 0)
goto cleanup;
-if (virAsprintf(, "%s/qemucapabilitiesdata/%s.%s.xml",
-abs_srcdir, data->base, data->archName) < 0)
+if (virAsprintf(, "%s/%s.%s.xml",
+data->inputDir, data->base, data->archName) < 0)
goto cleanup;
if (virTestLoadFile(capsFile, ) < 0)
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/8] tests: Improve QEMU capability testing
This series removes the need to fiddle with test programs every time a new .replies file is added to the test suite, and also completely removes the possibility of missing on some test coverage because only one of the two test programs is updated. Andrea Bolognani (8): tests: Introduce testQemuDataInit() and testQemuDataReset() tests: Move ret into testQemuData tests: Move data directories into testQemuData tests: Use virAsprintf() to build titles tests: Move code from DO_TEST() to doCapsTest() tests: Add testutilsqemu dependency for qemucaps2xmltest tests: Introduce testQemuCapsIterate() tests: Use testQemuCapsIterate() tests/Makefile.am| 1 + tests/qemucapabilitiestest.c | 123 ++- tests/qemucaps2xmltest.c | 104 ++--- tests/testutilsqemu.c| 53 +++ tests/testutilsqemu.h| 8 +++ 5 files changed, 179 insertions(+), 110 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/8] tests: Move ret into testQemuData
This is not particularly useful right now, but will allow us
to refactor some functionality later on.
Signed-off-by: Andrea Bolognani
---
tests/qemucapabilitiestest.c | 10 ++
tests/qemucaps2xmltest.c | 11 ++-
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 882fa57485..0f875f9e24 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -37,6 +37,7 @@ struct _testQemuData {
virQEMUDriver driver;
const char *archName;
const char *base;
+int ret;
};
@@ -46,6 +47,8 @@ testQemuDataInit(testQemuDataPtr data)
if (qemuTestDriverInit(>driver) < 0)
return -1;
+data->ret = 0;
+
return 0;
}
@@ -173,7 +176,6 @@ testQemuCapsCopy(const void *opaque)
static int
mymain(void)
{
-int ret = 0;
testQemuData data;
#if !WITH_YAJL
@@ -194,10 +196,10 @@ mymain(void)
data.archName = arch; \
data.base = name; \
if (virTestRun(name "(" arch ")", testQemuCaps, ) < 0) \
-ret = -1; \
+data.ret = -1; \
if (virTestRun("copy " name "(" arch ")", \
testQemuCapsCopy, ) < 0) \
-ret = -1; \
+data.ret = -1; \
} while (0)
/* Keep this in sync with qemucaps2xmltest */
@@ -248,7 +250,7 @@ mymain(void)
testQemuDataReset();
-return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+return (data.ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
VIR_TEST_MAIN(mymain)
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 5cc9fb635b..e9b0b11e35 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -30,11 +30,14 @@ typedef testQemuData *testQemuDataPtr;
struct _testQemuData {
const char *base;
const char *archName;
+int ret;
};
static int
-testQemuDataInit(testQemuDataPtr data ATTRIBUTE_UNUSED)
+testQemuDataInit(testQemuDataPtr data)
{
+data->ret = 0;
+
return 0;
}
@@ -173,8 +176,6 @@ testQemuCapsXML(const void *opaque)
static int
mymain(void)
{
-int ret = 0;
-
testQemuData data;
#if !WITH_YAJL
@@ -194,7 +195,7 @@ mymain(void)
data.archName = arch; \
data.base = name; \
if (virTestRun(name "(" arch ")", testQemuCapsXML, ) < 0) \
-ret = -1
+data.ret = -1
/* Keep this in sync with qemucapabilitiestest */
DO_TEST("x86_64", "caps_1.5.3");
@@ -236,7 +237,7 @@ mymain(void)
testQemuDataReset();
-return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+return (data.ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemucaps2xmlmock.so")
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/4] qemu_domain: NVLink2 device tree functions for PPC64
On 3/7/19 12:15 PM, Erik Skultety wrote:
On Tue, Mar 05, 2019 at 09:46:08AM -0300, Daniel Henrique Barboza wrote:
The NVLink2 support in QEMU implements the detection of NVLink2
capable devices by verfying the attributes of the VFIO mem region
QEMU allocates for the NVIDIA GPUs. To properly allocate an
adequate amount of memLock, Libvirt needs this information before
a QEMU instance is even created.
An alternative is presented in this patch. Given a PCI device,
we'll traverse the device tree at /proc/device-tree to check if
the device has a NPU bridge, retrieve the node of the NVLink2 bus,
find the memory-node that is related to the bus and see if it's a
NVLink2 bus by inspecting its 'reg' value. This logic is contained
inside the 'device_is_nvlink2_capable' function, which uses other
new helper functions to navigate and fetch values from the device
tree nodes.
Signed-off-by: Daniel Henrique Barboza
---
This fails with a bunch of compilation errors, please make sure you run make
check and make syntax-check on each patch of the series.
Ooops, forgot to follow up make syntax-check with 'make' before
submitting ... my bad.
src/qemu/qemu_domain.c | 194 +
1 file changed, 194 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 77548c224c..97de5793e2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10343,6 +10343,200 @@ qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm)
}
+/**
+ * Reads a phandle file and returns the phandle value.
+ */
+static int
+read_dt_phandle(const char* file)
..we prefer camelCase naming of functions...all functions should have a prefix,
e.g. vir,, etc.
+{
+unsigned int buf[1];
+size_t read;
+FILE *f;
+
+f = fopen(file, "r");
+if (!f)
+return -1;
+
+read = fread(buf, sizeof(unsigned int), 1, f);
We already have safe helpers that take care of such operations.
+
+if (!read) {
+VIR_CLOSE(f);
You could use VIR_AUTOCLOSE for this
+return 0;
+}
+
+VIR_CLOSE(f);
+return be32toh(buf[0]);
Is this part of gnulib? We need to make sure it's portable otherwise we'd need
to make the conversion ourselves, just like for le64toh
+}
+
+
+/**
+ * Reads a memory reg file and returns the first 4 int values.
+ *
+ * The caller is responsible for freeing the returned array.
+ */
+static unsigned int *
+read_dt_memory_reg(const char *file)
+{
+unsigned int *buf;
+size_t read, i;
+FILE *f;
+
+f = fopen(file, "r");
+if (!f)
+return NULL;
+
+if (VIR_ALLOC_N(buf, 4) < 0)
+return NULL;
+
+read = fread(buf, sizeof(unsigned int), 4, f);
+
+if (!read && read < 4)
+/* shouldn't happen */
+VIR_FREE(buf);
+else for (i = 0; i < 4; i++)
+buf[i] = be32toh(buf[i]);
+
+VIR_CLOSE(f);
+return buf;
Same comments as above...
+}
+
+
+/**
+ * This wrapper function receives arguments to be used in a
+ * 'find' call to retrieve the file names that matches
+ * the criteria inside the /proc/device-tree dir.
+ *
+ * A 'find' call with '-iname phandle' inside /proc/device-tree
+ * provides more than a thousand matches. Adding '-path' to
+ * narrow it down further is necessary to keep the file
+ * listing sane.
+ *
+ * The caller is responsible to free the buffer returned by
+ * this function.
+ */
+static char *
+retrieve_dt_files_pattern(const char *path_pattern, const char *file_pattern)
+{
+virCommandPtr cmd = NULL;
+char *output = NULL;
+
+cmd = virCommandNew("find");
+virCommandAddArgList(cmd, "/proc/device-tree/", "-path", path_pattern,
+ "-iname", file_pattern, NULL);
+virCommandSetOutputBuffer(cmd, );
+
+if (virCommandRun(cmd, NULL) < 0)
+VIR_FREE(output);
+
+virCommandFree(cmd);
+return output;
+}
+
+
+/**
+ * Helper function that receives a listing of file names and
+ * calls read_dt_phandle() on each one finding for a match
+ * with the given phandle argument. Returns the file name if a
+ * match is found, NULL otherwise.
+ */
+static char *
+find_dt_file_with_phandle(char *files, int phandle)
+{
+char *line, *tmp;
+int ret;
+
+line = strtok_r(files, "\n", );
+do {
+ ret = read_dt_phandle(line);
+ if (ret == phandle)
+ break;
+} while ((line = strtok_r(NULL, "\n", )) != NULL);
+
+return line;
+}
+
+
+/**
+ * This function receives a string that represents a PCI device,
+ * such as '0004:04:00.0', and tells if the device is NVLink2 capable.
+ *
+ * The logic goes as follows:
+ *
+ * 1 - get the phandle of a nvlink of the device, reading the 'ibm,npu'
+ * attribute;
+ * 2 - find the device tree node of the nvlink bus using the phandle
+ * found in (1)
+ * 3 - get the phandle of the memory region of the nvlink bus
+ * 4 - find the device tree node of the memory region using the
+ * phandle found in (3)
+ * 5 - read the 'reg' value of the memory
Re: [libvirt] [PATCH v3 02/18] snapshot: Rework virDomainSnapshotState enum
On 3/7/19 9:20 AM, Ján Tomko wrote:
> On Mon, Mar 04, 2019 at 09:34:29PM -0600, Eric Blake wrote:
>> The existing virDomainSnapshotState is a superset of virDomainState,
>> adding one more state (disk-snapshot) on top of valid domain states.
>> But as written, the enum cannot be used for gcc validation that all
>> enum values are covered in a strongly-typed switch condition, because
>> the enum does not explicitly include the values it is adding to.
>>
>> Copy the style used in qemu_blockjob.h of creating new enum names
>> for every inherited value, and update most clients to use the new
>> enum names anywhere snapshot state is referenced. The exception is
>> two switch statements in qemu code, which instead gain a fixme
>> comment about odd type usage (which will be cleaned up in the next
>> patch). The rest of the patch is mechanical.
>>
>> Signed-off-by: Eric Blake
>> ---
>> src/conf/snapshot_conf.h | 21 ++---
>> src/conf/snapshot_conf.c | 28 ++--
>> src/qemu/qemu_driver.c | 34 ++
>> src/test/test_driver.c | 20 ++--
>> src/vbox/vbox_common.c | 4 ++--
>> 5 files changed, 66 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
>> index 7a175dfc96..9084f5fb8b 100644
>> --- a/src/conf/snapshot_conf.h
>> +++ b/src/conf/snapshot_conf.h
>> @@ -36,11 +36,26 @@ typedef enum {
>> VIR_DOMAIN_SNAPSHOT_LOCATION_LAST
>> } virDomainSnapshotLocation;
>>
>> +/**
>> + * This enum has to map all known domain states from the public enum
>> + * virDomainState, before adding one additional state possible only
>> + * for snapshots.
>> + */
>> typedef enum {
>> - /* Inherit the VIR_DOMAIN_* states from virDomainState. */
>> - VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
>> - VIR_DOMAIN_SNAPSHOT_STATE_LAST
>> + /* Mapped to public enum */
>> + VIR_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
>> + VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
>> + VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
>> + VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
>> + VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
>> + VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
>> + VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
>> + VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
>> + /* Additional enum values local to qemu */
>
> As Eric Blake pointed out in an earlier iteration:
> s/qemu/snapshots/
>
>> + VIR_SNAP_STATE_DISK_SNAPSHOT,
>> + VIR_SNAP_STATE_LAST
>> } virDomainSnapshotState;
>> +verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
>>
>> /* Stores disk-snapshot information */
>> typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 41236d9932..299fc2101b 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -58,7 +58,7 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation,
>> VIR_DOMAIN_SNAPSHOT_LOCATION_LAST,
>> );
>>
>> /* virDomainSnapshotState is really virDomainState plus one extra
>> state */
>> -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST,
>> +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST,
>
> VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix
>
, true, but we could go with virSnapState to ensure we match the
typedef nomenclature with the enum symbol names or change the _SNAP_ to
_SNAPSHOT_ w/ virSnapshotState for the typedef - whatever is easiest.
John
>> "nostate",
>> "running",
>> "blocked",
>
> Jano
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 16/18] qemu: Factor out qemuDomainSnapshotValidate() helper
On 3/4/19 10:34 PM, Eric Blake wrote: > Straight code motion, coupled with changing goto into return -1 > as needed. This change will be important to later patches adding > bulk redefinition (where each snapshot in a list has to meet the > same constraints). > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_driver.c | 120 ++--- > 1 file changed, 66 insertions(+), 54 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/18] qemu: Const-correct snapshot directory name
On 3/4/19 10:34 PM, Eric Blake wrote: > qemuDomainSnapshotWriteMetadata does not modify the directory name, > and making it const-correct aids in writing an upcoming patch. > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_domain.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/4] qemu_domain: NVLink2 device tree functions for PPC64
On Tue, Mar 05, 2019 at 09:46:08AM -0300, Daniel Henrique Barboza wrote:
> The NVLink2 support in QEMU implements the detection of NVLink2
> capable devices by verfying the attributes of the VFIO mem region
> QEMU allocates for the NVIDIA GPUs. To properly allocate an
> adequate amount of memLock, Libvirt needs this information before
> a QEMU instance is even created.
>
> An alternative is presented in this patch. Given a PCI device,
> we'll traverse the device tree at /proc/device-tree to check if
> the device has a NPU bridge, retrieve the node of the NVLink2 bus,
> find the memory-node that is related to the bus and see if it's a
> NVLink2 bus by inspecting its 'reg' value. This logic is contained
> inside the 'device_is_nvlink2_capable' function, which uses other
> new helper functions to navigate and fetch values from the device
> tree nodes.
>
> Signed-off-by: Daniel Henrique Barboza
> ---
This fails with a bunch of compilation errors, please make sure you run make
check and make syntax-check on each patch of the series.
> src/qemu/qemu_domain.c | 194 +
> 1 file changed, 194 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 77548c224c..97de5793e2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10343,6 +10343,200 @@ qemuDomainUpdateCurrentMemorySize(virDomainObjPtr
> vm)
> }
>
>
> +/**
> + * Reads a phandle file and returns the phandle value.
> + */
> +static int
> +read_dt_phandle(const char* file)
..we prefer camelCase naming of functions...all functions should have a prefix,
e.g. vir,, etc.
> +{
> +unsigned int buf[1];
> +size_t read;
> +FILE *f;
> +
> +f = fopen(file, "r");
> +if (!f)
> +return -1;
> +
> +read = fread(buf, sizeof(unsigned int), 1, f);
We already have safe helpers that take care of such operations.
> +
> +if (!read) {
> +VIR_CLOSE(f);
You could use VIR_AUTOCLOSE for this
> +return 0;
> +}
> +
> +VIR_CLOSE(f);
> +return be32toh(buf[0]);
Is this part of gnulib? We need to make sure it's portable otherwise we'd need
to make the conversion ourselves, just like for le64toh
> +}
> +
> +
> +/**
> + * Reads a memory reg file and returns the first 4 int values.
> + *
> + * The caller is responsible for freeing the returned array.
> + */
> +static unsigned int *
> +read_dt_memory_reg(const char *file)
> +{
> +unsigned int *buf;
> +size_t read, i;
> +FILE *f;
> +
> +f = fopen(file, "r");
> +if (!f)
> +return NULL;
> +
> +if (VIR_ALLOC_N(buf, 4) < 0)
> +return NULL;
> +
> +read = fread(buf, sizeof(unsigned int), 4, f);
> +
> +if (!read && read < 4)
> +/* shouldn't happen */
> +VIR_FREE(buf);
> +else for (i = 0; i < 4; i++)
> +buf[i] = be32toh(buf[i]);
> +
> +VIR_CLOSE(f);
> +return buf;
Same comments as above...
> +}
> +
> +
> +/**
> + * This wrapper function receives arguments to be used in a
> + * 'find' call to retrieve the file names that matches
> + * the criteria inside the /proc/device-tree dir.
> + *
> + * A 'find' call with '-iname phandle' inside /proc/device-tree
> + * provides more than a thousand matches. Adding '-path' to
> + * narrow it down further is necessary to keep the file
> + * listing sane.
> + *
> + * The caller is responsible to free the buffer returned by
> + * this function.
> + */
> +static char *
> +retrieve_dt_files_pattern(const char *path_pattern, const char *file_pattern)
> +{
> +virCommandPtr cmd = NULL;
> +char *output = NULL;
> +
> +cmd = virCommandNew("find");
> +virCommandAddArgList(cmd, "/proc/device-tree/", "-path", path_pattern,
> + "-iname", file_pattern, NULL);
> +virCommandSetOutputBuffer(cmd, );
> +
> +if (virCommandRun(cmd, NULL) < 0)
> +VIR_FREE(output);
> +
> +virCommandFree(cmd);
> +return output;
> +}
> +
> +
> +/**
> + * Helper function that receives a listing of file names and
> + * calls read_dt_phandle() on each one finding for a match
> + * with the given phandle argument. Returns the file name if a
> + * match is found, NULL otherwise.
> + */
> +static char *
> +find_dt_file_with_phandle(char *files, int phandle)
> +{
> +char *line, *tmp;
> +int ret;
> +
> +line = strtok_r(files, "\n", );
> +do {
> + ret = read_dt_phandle(line);
> + if (ret == phandle)
> + break;
> +} while ((line = strtok_r(NULL, "\n", )) != NULL);
> +
> +return line;
> +}
> +
> +
> +/**
> + * This function receives a string that represents a PCI device,
> + * such as '0004:04:00.0', and tells if the device is NVLink2 capable.
> + *
> + * The logic goes as follows:
> + *
> + * 1 - get the phandle of a nvlink of the device, reading the 'ibm,npu'
> + * attribute;
> + * 2 - find the device tree node of the nvlink bus using the phandle
> + * found in (1)
> + * 3 - get the phandle of the memory region
Re: [libvirt] [PATCH v3 14/18] virsh: Expose bulk snapshot dumpxml/redefine
On 3/4/19 10:34 PM, Eric Blake wrote:
> Add flags to the 'dumpxml' and 'snapshot-create' commands to pass
> the newly-added bulk snapshot flags through.
>
> For command-line convenience, I intentionally made --redefine-list
> imply --redefine, even though the counterpart C flags are distinct
> (and you get an error if you pass _REDEFINE_LIST without _REDEFINE).
>
> Signed-off-by: Eric Blake
> ---
> tools/virsh-domain.c | 7 +++
> tools/virsh-snapshot.c | 14 ++
> tools/virsh.pod| 18 ++
> 3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5699018dcc..78854b1e0a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10055,6 +10055,10 @@ static const vshCmdOptDef opts_dumpxml[] = {
> .type = VSH_OT_BOOL,
> .help = N_("provide XML suitable for migrations")
> },
> +{.name = "snapshots",
> + .type = VSH_OT_BOOL,
> + .help = N_("include all domain snapshots in XML dump"),
> +},
> {.name = NULL}
> };
>
> @@ -10069,6 +10073,7 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd)
> bool secure = vshCommandOptBool(cmd, "security-info");
> bool update = vshCommandOptBool(cmd, "update-cpu");
> bool migratable = vshCommandOptBool(cmd, "migratable");
> +bool snapshots = vshCommandOptBool(cmd, "snapshots");
>
> if (inactive)
> flags |= VIR_DOMAIN_XML_INACTIVE;
> @@ -10078,6 +10083,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_XML_UPDATE_CPU;
> if (migratable)
> flags |= VIR_DOMAIN_XML_MIGRATABLE;
> +if (snapshots)
> +flags |= VIR_DOMAIN_XML_SNAPSHOTS;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index e38ebb1f28..cbb7d744f9 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -80,6 +80,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom,
> const char *buffer,
> goto cleanup;
> }
>
> +if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
> +vshPrintExtra(ctl, "%s",
> + _("Domain snapshot list imported successfully"));
> +ret = true;
> +goto cleanup;
> +}
> +
> name = virDomainSnapshotGetName(snapshot);
> if (!name) {
> vshError(ctl, "%s", _("Could not get snapshot name"));
> @@ -122,6 +129,10 @@ static const vshCmdOptDef opts_snapshot_create[] = {
> .help = N_("redefine metadata for existing snapshot")
> },
> VIRSH_COMMON_OPT_CURRENT(N_("with redefine, set current snapshot")),
> +{.name = "redefine-list",
> + .type = VSH_OT_BOOL,
> + .help = N_("bulk define a set of snapshots, implies --redefine"),
> +},
> {.name = "no-metadata",
> .type = VSH_OT_BOOL,
> .help = N_("take snapshot but create no metadata")
> @@ -177,6 +188,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> +if (vshCommandOptBool(cmd, "redefine-list"))
> +flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
> +VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> goto cleanup;
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 8e18b30f29..ea51584d3e 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1647,7 +1647,7 @@ is required in order to produce valid ELF file which
> can be later processed by
> the crash utility.
>
> =item B I [I<--inactive>] [I<--security-info>]
> -[I<--update-cpu>] [I<--migratable>]
> +[I<--update-cpu>] [I<--migratable>] [I<--snapshots>]
>
> Output the domain information as an XML dump to stdout, this format can be
> used
> by the B command. Additional options affecting the XML dump may be
> @@ -1660,6 +1660,9 @@ migrations, i.e., compatible with older libvirt
> releases and possibly amended
> with internal run-time options. This option may automatically enable other
> options (I<--update-cpu>, I<--security-info>, ...) as necessary.
>
> +Using I<--snapshots> will expand the output to also include information on
> +all domain snapshots, for servers that understand the flag.
s/, for/ for/
s/understand/support/
(remove comma)...
> +
> =item B I
>
> Edit the XML configuration file for a domain, which will affect the
> @@ -4544,8 +4547,9 @@ used to represent properties of snapshots.
>
> =over 4
>
> -=item B I [I] {[I<--redefine>
> [I<--current>]]
> -| [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>]
> +=item B I [I] {[I<--redefine>
> +{[I<--current>] | [I<--redefine-list>]}] | [I<--no-metadata>]
> +[I<--halt>] [I<--disk-only>] [I<--reuse-external>]
> [I<--quiesce>] [I<--atomic>] [I<--live>]}
>
> Create a
Re: [libvirt] [PATCH v2 00/15] Firmware auto selection
On 3/7/19 1:04 PM, Daniel P. Berrangé wrote: On Thu, Mar 07, 2019 at 10:29:10AM +0100, Michal Privoznik wrote: v2 of: https://www.redhat.com/archives/libvir-list/2019-February/msg01503.html As usual, you can find my patches at my github: https://github.com/zippy2/libvirt/commits/firmware_v2 diff to v1: - Hopefully all Lazlo's comment are worked in (fixing the logic that chooses suitable firmware for secboot/SMM domains, commit message adjustements, sanity check for parsed FW descriptions, etc.) - Added more debug messages around FW selection code, i.e. it'll be visible from the logs why given FS is not suitable (or that it is). I've done a very quick review of the patches and the design and overall implementation strategy looks good to me. A few misc points - We ought to expose the list of firmware types supported in the domain capabilities, so mgmt apps can probe if uefi is available Makes sense, I'll start working on this after these are merged. - What should we do about the 'nvram' config from /etc/libvirt/qemu.conf We can't get rid of it for a while, since the new firmware configs won't be widely supported by distros in forseeable future. In the places which currently use the 'nvram' config though, should we make sure we consult the firmware configs in prefereence to resolve the var store ? Maybe your code is already doing this and I missed the logic from the diffs. I think we can ignore 'nvram' from qemu.conf safely. The whole point of nvram variable is to define which varstore corresponds to which fw image. These pairs are consulted if and only if no template was specified in domain XML. With firmware autoselect I don't think we are going to fulfil the condition as my code takes whatever FW config said the path to NVRAM is and uses that as template for the domain. And even if FW config did not provide any NVRAM path (which can be viewed as bug IMO) then we would consult qemu.conf (or some hardwired defaults). - We support 'bios' and 'efi' right now, but QEMU also reoprts 'openfirmware' and 'uboot' as supported types. In practice no config files probably exist for those yet, but if it is easy to make the code support them we should try todo it to get parity with the QEMU spec Sure. It's just that I don't think there is a way to even configure thos via our domain XML, is there? I don't think any of these are blockers though - all would work as followup patches I expect. Yes, agreed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 7/7] monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove
From: Daniel P. Berrangé
The various ACL related commands are obsolete now that the QAuthZ
framework for authorization is fully integrated throughout QEMU network
services. These only ever worked with VNC and were never used by libvirt.
Mark it as deprecated with no direct replacement to be provided.
Authorization is now provided by using 'object_add' together with
the 'tls-authz' or 'sasl-authz' parameters to the VNC server, and
equivalent for other network services.
Reviewed-by: Juan Quintela
Signed-off-by: Daniel P. Berrangé
Message-id: 20190227145755.26556-3-berra...@redhat.com
Signed-off-by: Gerd Hoffmann
---
monitor.c| 23 +++
qemu-deprecated.texi | 6 ++
2 files changed, 29 insertions(+)
diff --git a/monitor.c b/monitor.c
index defa129319b0..72061d5baeb4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2032,6 +2032,19 @@ static QAuthZList *find_auth(Monitor *mon, const char
*name)
return QAUTHZ_LIST(obj);
}
+static bool warn_acl;
+static void hmp_warn_acl(void)
+{
+if (warn_acl) {
+return;
+}
+error_report("The acl_show, acl_reset, acl_policy, acl_add, acl_remove "
+ "commands are deprecated with no replacement. Authorization "
+ "for VNC should be performed using the pluggable QAuthZ "
+ "objects");
+warn_acl = true;
+}
+
static void hmp_acl_show(Monitor *mon, const QDict *qdict)
{
const char *aclname = qdict_get_str(qdict, "aclname");
@@ -2039,6 +2052,8 @@ static void hmp_acl_show(Monitor *mon, const QDict *qdict)
QAuthZListRuleList *rules;
size_t i = 0;
+hmp_warn_acl();
+
if (!auth) {
return;
}
@@ -2062,6 +2077,8 @@ static void hmp_acl_reset(Monitor *mon, const QDict
*qdict)
const char *aclname = qdict_get_str(qdict, "aclname");
QAuthZList *auth = find_auth(mon, aclname);
+hmp_warn_acl();
+
if (!auth) {
return;
}
@@ -2080,6 +2097,8 @@ static void hmp_acl_policy(Monitor *mon, const QDict
*qdict)
int val;
Error *err = NULL;
+hmp_warn_acl();
+
if (!auth) {
return;
}
@@ -2124,6 +2143,8 @@ static void hmp_acl_add(Monitor *mon, const QDict *qdict)
QAuthZListFormat format;
size_t i = 0;
+hmp_warn_acl();
+
if (!auth) {
return;
}
@@ -2169,6 +2190,8 @@ static void hmp_acl_remove(Monitor *mon, const QDict
*qdict)
QAuthZList *auth = find_auth(mon, aclname);
ssize_t i = 0;
+hmp_warn_acl();
+
if (!auth) {
return;
}
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1258da479535..1e15f57e9cc9 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -104,6 +104,12 @@ The @option{[hub_id name]} parameter tuple of the
'hostfwd_add' and
Use ``device_add'' for hotplugging vCPUs instead of ``cpu-add''. See
documentation of ``query-hotpluggable-cpus'' for additional details.
+@subsection acl_show, acl_reset, acl_policy, acl_add, acl_remove (since 4.0.0)
+
+The ``acl_show'', ``acl_reset'', ``acl_policy'', ``acl_add'', and
+``acl_remove'' commands are deprecated with no replacement. Authorization
+for VNC should be performed using the pluggable QAuthZ objects.
+
@section System emulator devices
@subsection bluetooth (since 3.1)
--
2.18.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 6/7] vnc: allow specifying a custom authorization object name
From: "Daniel P. Berrange"
The VNC server has historically had support for ACLs to check both the
SASL username and the TLS x509 distinguished name. The VNC server was
responsible for creating the initial ACL, and the client app was then
responsible for populating it with rules using the HMP 'acl_add' command.
This is not satisfactory for a variety of reasons. There is no way to
populate the ACLs from the command line, users are forced to use the
HMP. With multiple network services all supporting TLS and ACLs now, it
is desirable to be able to define a single ACL that is referenced by all
services.
To address these limitations, two new options are added to the VNC
server CLI. The 'tls-authz' option takes the ID of a QAuthZ object to
use for checking TLS x509 distinguished names, and the 'sasl-authz'
option takes the ID of another object to use for checking SASL usernames.
In this example, we setup two authorization rules. The first allows any
client with a certificate issued by the 'RedHat' organization in the
'London' locality. The second ACL allows clients with either the
'j...@redhat.com' or 'f...@redhat.com' kerberos usernames. Both checks
must pass for the user to be allowed.
$QEMU -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
endpoint=server,verify-peer=yes \
-object authz-simple,id=authz0,policy=deny,\
rules.0.match=O=RedHat,,L=London,rules.0.policy=allow \
-object authz-simple,id=authz1,policy=deny,\
rules.0.match=f...@redhat.com,rules.0.policy=allow \
rules.0.match=j...@redhat.com,rules.0.policy=allow \
-vnc 0.0.0.0:1,tls-creds=tls0,tls-authz=authz0,
sasl,sasl-authz=authz1 \
...other QEMU args...
Reviewed-by: Juan Quintela
Signed-off-by: Daniel P. Berrange
Message-id: 20190227145755.26556-2-berra...@redhat.com
Signed-off-by: Gerd Hoffmann
---
ui/vnc.c | 58 +---
qemu-deprecated.texi | 5
qemu-options.hx | 35 ++
3 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 2f2ab62fcf71..2d9e8f43b09b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3363,6 +3363,12 @@ static QemuOptsList qemu_vnc_opts = {
},{
.name = "acl",
.type = QEMU_OPT_BOOL,
+},{
+.name = "tls-authz",
+.type = QEMU_OPT_STRING,
+},{
+.name = "sasl-authz",
+.type = QEMU_OPT_STRING,
},{
.name = "lossy",
.type = QEMU_OPT_BOOL,
@@ -3802,6 +3808,8 @@ void vnc_display_open(const char *id, Error **errp)
const char *credid;
bool sasl = false;
int acl = 0;
+const char *tlsauthz;
+const char *saslauthz;
int lock_key_sync = 1;
int key_delay_ms;
@@ -3873,7 +3881,33 @@ void vnc_display_open(const char *id, Error **errp)
goto fail;
}
}
+if (qemu_opt_get(opts, "acl")) {
+error_report("The 'acl' option to -vnc is deprecated. "
+ "Please use the 'tls-authz' and 'sasl-authz' "
+ "options instead");
+}
acl = qemu_opt_get_bool(opts, "acl", false);
+tlsauthz = qemu_opt_get(opts, "tls-authz");
+if (acl && tlsauthz) {
+error_setg(errp, "'acl' option is mutually exclusive with the "
+ "'tls-authz' option");
+goto fail;
+}
+if (tlsauthz && !vd->tlscreds) {
+error_setg(errp, "'tls-authz' provided but TLS is not enabled");
+goto fail;
+}
+
+saslauthz = qemu_opt_get(opts, "sasl-authz");
+if (acl && saslauthz) {
+error_setg(errp, "'acl' option is mutually exclusive with the "
+ "'sasl-authz' option");
+goto fail;
+}
+if (saslauthz && !sasl) {
+error_setg(errp, "'sasl-authz' provided but SASL auth is not enabled");
+goto fail;
+}
share = qemu_opt_get(opts, "share");
if (share) {
@@ -3903,7 +3937,9 @@ void vnc_display_open(const char *id, Error **errp)
vd->non_adaptive = true;
}
-if (acl) {
+if (tlsauthz) {
+vd->tlsauthzid = g_strdup(tlsauthz);
+} else if (acl) {
if (strcmp(vd->id, "default") == 0) {
vd->tlsauthzid = g_strdup("vnc.x509dname");
} else {
@@ -3914,15 +3950,19 @@ void vnc_display_open(const char *id, Error **errp)
_abort));
}
#ifdef CONFIG_VNC_SASL
-if (acl && sasl) {
-if (strcmp(vd->id, "default") == 0) {
-vd->sasl.authzid = g_strdup("vnc.username");
-} else {
-vd->sasl.authzid = g_strdup_printf("vnc.%s.username", vd->id);
+if (sasl) {
+if (saslauthz) {
+vd->sasl.authzid = g_strdup(saslauthz);
+} else if (acl) {
+if (strcmp(vd->id, "default") == 0) {
+
[libvirt] [PULL 0/7] Ui 20190307 patches
The following changes since commit 32694e98b8d7a246345448a8f707d2e11d6c65e2: Merge remote-tracking branch 'remotes/ehabkost/tags/machine-next-pull-request' into staging (2019-03-06 18:52:19 +) are available in the Git repository at: git://git.kraxel.org/qemu tags/ui-20190307-pull-request for you to fetch changes up to 9b3c34835094760732c65a829bf0c07512126496: monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove (2019-03-07 14:23:22 +0100) curses: better wide char support. vnc: acl update, stall fix. Daniel P. Berrangé (2): vnc: allow specifying a custom authorization object name monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove Gerd Hoffmann (1): vnc: fix update stalls Samuel Thibault (4): Reduce curses escdelay from 1s to 25ms curses: support wide input iconv: detect and make curses depend on it curses: add option to specify VGA font encoding configure| 45 - ui/curses_keys.h | 113 +++-- monitor.c| 23 +++ ui/curses.c | 394 --- ui/vnc.c | 64 ++- vl.c | 2 +- qapi/ui.json | 14 ++ qemu-deprecated.texi | 11 ++ qemu-options.hx | 40 +++-- 9 files changed, 574 insertions(+), 132 deletions(-) -- 2.18.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 5/7] vnc: fix update stalls
vnc aborts display update jobs on video mode switches and page flips.
That can cause vnc update stalls in case an unfinished vnc job gets
aborted. The vnc client will never receive the requested update then.
Fix that by copying the state from job_update back to update in that
case.
Reports complain about stalls with two or more clients being connected
at the same time, on some but not all connections. I suspect it can
also happen with a single connection, multiple connections only make
this more much likely to happen.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1662260
Reported-by: Ying Fang
Signed-off-by: Gerd Hoffmann
Reviewed-by: Ying Fang
Message-id: 20190305130930.24516-1-kra...@redhat.com
---
ui/vnc.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ui/vnc.c b/ui/vnc.c
index da4a21d4ce94..2f2ab62fcf71 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -700,6 +700,12 @@ static void vnc_abort_display_jobs(VncDisplay *vd)
}
QTAILQ_FOREACH(vs, >clients, next) {
vnc_lock_output(vs);
+if (vs->update == VNC_STATE_UPDATE_NONE &&
+vs->job_update != VNC_STATE_UPDATE_NONE) {
+/* job aborted before completion */
+vs->update = vs->job_update;
+vs->job_update = VNC_STATE_UPDATE_NONE;
+}
vs->abort = false;
vnc_unlock_output(vs);
}
--
2.18.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 3/7] iconv: detect and make curses depend on it
From: Samuel Thibault
curses will use it for proper wide output support.
Signed-off-by: Samuel Thibault
Message-Id: <20190304210217.7056-2-samuel.thiba...@ens-lyon.org>
Signed-off-by: Gerd Hoffmann
---
configure | 40
vl.c | 2 +-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index cefeb8fcce44..c594d2be18c7 100755
--- a/configure
+++ b/configure
@@ -1214,6 +1214,10 @@ for opt do
;;
--enable-curses) curses="yes"
;;
+ --disable-iconv) iconv="no"
+ ;;
+ --enable-iconv) iconv="yes"
+ ;;
--disable-curl) curl="no"
;;
--enable-curl) curl="yes"
@@ -1703,6 +1707,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
gtk gtk UI
vte vte support for the gtk UI
curses curses UI
+ iconv font glyph conversion support
vnc VNC UI support
vnc-saslSASL encryption for VNC server
vnc-jpegJPEG lossy compression for VNC server
@@ -3424,8 +3429,39 @@ EOF
fi
fi
+##
+# iconv probe
+if test "$iconv" != "no" ; then
+ cat > $TMPC << EOF
+#include
+int main(void) {
+ iconv_t conv = iconv_open("WCHAR_T", "UCS-2");
+ return conv != (iconv_t) -1;
+}
+EOF
+ for iconv_lib in '' -liconv; do
+if compile_prog "" "$iconv_lib" ; then
+ iconv_found=yes
+ libs_softmmu="$iconv_lib $libs_softmmu"
+ break
+fi
+ done
+ if test "$iconv_found" = "yes" ; then
+iconv=yes
+ else
+if test "$iconv" = "yes" ; then
+ feature_not_found "iconv" "Install iconv devel"
+fi
+iconv=no
+ fi
+fi
+
##
# curses probe
+if test "$iconv" = "no" ; then
+ # curses will need iconv
+ curses=no
+fi
if test "$curses" != "no" ; then
if test "$mingw32" = "yes" ; then
curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
@@ -6137,6 +6173,7 @@ echo "libgcrypt $gcrypt"
echo "nettle$nettle $(echo_version $nettle $nettle_version)"
echo "libtasn1 $tasn1"
echo "PAM $auth_pam"
+echo "iconv support $iconv"
echo "curses support$curses"
echo "virgl support $virglrenderer $(echo_version $virglrenderer
$virgl_version)"
echo "curl support $curl"
@@ -6461,6 +6498,9 @@ fi
if test "$cocoa" = "yes" ; then
echo "CONFIG_COCOA=y" >> $config_host_mak
fi
+if test "$iconv" = "yes" ; then
+ echo "CONFIG_ICONV=y" >> $config_host_mak
+fi
if test "$curses" = "yes" ; then
echo "CONFIG_CURSES=m" >> $config_host_mak
echo "CURSES_CFLAGS=$curses_inc" >> $config_host_mak
diff --git a/vl.c b/vl.c
index 4c5cc0d8ad91..b1eef2b60a03 100644
--- a/vl.c
+++ b/vl.c
@@ -3172,7 +3172,7 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_CURSES
dpy.type = DISPLAY_TYPE_CURSES;
#else
-error_report("curses support is disabled");
+error_report("curses or iconv support is disabled");
exit(1);
#endif
break;
--
2.18.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 4/7] curses: add option to specify VGA font encoding
From: Samuel Thibault
This uses iconv to convert glyphs from the specified VGA font encoding to
unicode, and makes use of cchar_t instead of chtype when using ncursesw,
which allows to store all wide char as well as the WACS values. The default
charset is made CP437 since that is the charset of the hardware default VGA
font. This also makes the curses backend set the LC_CTYPE locale to "" to
allow curses to emit wide characters.
Signed-off-by: Samuel Thibault
Cc: Eddie Kohler
Message-Id: <20190304210217.7056-3-samuel.thiba...@ens-lyon.org>
Signed-off-by: Gerd Hoffmann
---
configure | 5 +-
ui/curses.c | 315
qapi/ui.json| 14 +++
qemu-options.hx | 5 +-
4 files changed, 290 insertions(+), 49 deletions(-)
diff --git a/configure b/configure
index c594d2be18c7..8422bf5eef70 100755
--- a/configure
+++ b/configure
@@ -3475,14 +3475,17 @@ if test "$curses" != "no" ; then
#include
#include
#include
+#include
int main(void) {
+ const char *codeset;
wchar_t wch = L'w';
setlocale(LC_ALL, "");
resize_term(0, 0);
addwstr(L"wide chars\n");
addnwstr(, 1);
add_wch(WACS_DEGREE);
- return 0;
+ codeset = nl_langinfo(CODESET);
+ return codeset != 0;
}
EOF
IFS=:
diff --git a/ui/curses.c b/ui/curses.c
index 37954ce1b046..3a7e8649f3de 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -27,6 +27,10 @@
#include
#include
#endif
+#include
+#include
+#include
+#include
#include "qapi/error.h"
#include "qemu-common.h"
@@ -54,25 +58,30 @@ static WINDOW *screenpad = NULL;
static int width, height, gwidth, gheight, invalidate;
static int px, py, sminx, sminy, smaxx, smaxy;
-static chtype vga_to_curses[256];
+static const char *font_charset = "CP437";
+static cchar_t vga_to_curses[256];
static void curses_update(DisplayChangeListener *dcl,
int x, int y, int w, int h)
{
console_ch_t *line;
-chtype curses_line[width];
+cchar_t curses_line[width];
line = screen + y * width;
for (h += y; y < h; y ++, line += width) {
for (x = 0; x < width; x++) {
chtype ch = line[x] & 0xff;
chtype at = line[x] & ~0xff;
-if (vga_to_curses[ch]) {
-ch = vga_to_curses[ch];
+if (vga_to_curses[ch].chars[0]) {
+curses_line[x] = vga_to_curses[ch];
+} else {
+curses_line[x].chars[0] = ch;
+curses_line[x].chars[1] = 0;
+curses_line[x].attr = 0;
}
-curses_line[x] = ch | at;
+curses_line[x].attr |= at;
}
-mvwaddchnstr(screenpad, y, 0, curses_line, width);
+mvwadd_wchnstr(screenpad, y, 0, curses_line, width);
}
pnoutrefresh(screenpad, py, px, sminy, sminx, smaxy - 1, smaxx - 1);
@@ -391,6 +400,254 @@ static void curses_atexit(void)
endwin();
}
+/* Setup wchar glyph for one UCS-2 char */
+static void convert_ucs(int glyph, uint16_t ch, iconv_t conv)
+{
+wchar_t wch;
+char *pch, *pwch;
+size_t sch, swch;
+
+pch = (char *)
+pwch = (char *)
+sch = sizeof(ch);
+swch = sizeof(wch);
+
+if (iconv(conv, , , , ) == (size_t) -1) {
+fprintf(stderr, "Could not convert 0x%04x from UCS-2 to WCHAR_T: %s\n",
+ch, strerror(errno));
+} else {
+vga_to_curses[glyph].chars[0] = wch;
+}
+}
+
+/* Setup wchar glyph for one font character */
+static void convert_font(unsigned char ch, iconv_t conv)
+{
+wchar_t wch;
+char *pch, *pwch;
+size_t sch, swch;
+
+pch = (char *)
+pwch = (char *)
+sch = sizeof(ch);
+swch = sizeof(wch);
+
+if (iconv(conv, , , , ) == (size_t) -1) {
+fprintf(stderr, "Could not convert 0x%02x from %s to WCHAR_T: %s\n",
+ch, font_charset, strerror(errno));
+} else {
+vga_to_curses[ch].chars[0] = wch;
+}
+}
+
+/* Convert one wchar to UCS-2 */
+static uint16_t get_ucs(wchar_t wch, iconv_t conv)
+{
+uint16_t ch;
+char *pch, *pwch;
+size_t sch, swch;
+
+pch = (char *)
+pwch = (char *)
+sch = sizeof(ch);
+swch = sizeof(wch);
+
+if (iconv(conv, , , , ) == (size_t) -1) {
+fprintf(stderr, "Could not convert 0x%02x from WCHAR_T to UCS-2: %s\n",
+wch, strerror(errno));
+return 0xFFFD;
+}
+
+return ch;
+}
+
+/*
+ * Setup mapping for vga to curses line graphics.
+ */
+static void font_setup(void)
+{
+/*
+ * Control characters are normally non-printable, but VGA does have
+ * well-known glyphs for them.
+ */
+static uint16_t control_characters[0x20] = {
+ 0x0020,
+ 0x263a,
+ 0x263b,
+ 0x2665,
+ 0x2666,
+ 0x2663,
+ 0x2660,
+ 0x2022,
+ 0x25d8,
+ 0x25cb,
+ 0x25d9,
+ 0x2642,
+ 0x2640,
+ 0x266a,
+ 0x266b,
+ 0x263c,
+ 0x25ba,
+ 0x25c4,
+
[libvirt] [PULL 2/7] curses: support wide input
From: Samuel Thibault
This makes use of wide curses functions instead of 8bit functions. This
allows to type e.g. accented letters.
Unfortunately, key codes are then returned with values that could be
confused with wide characters by ncurses, so we need to add a maybe_keycode
variable to know whether the returned value is a key code or a character
(curses with wide support), or possibly both (curses without wide support).
The translation tables thus also need to be separated into key code
translation and character translation. The curses2foo helper makes it easier
to use them.
Signed-off-by: Samuel Thibault
Message-id: 20190304210532.7840-1-samuel.thiba...@ens-lyon.org
Signed-off-by: Gerd Hoffmann
---
ui/curses_keys.h | 113 +++
ui/curses.c | 76 +--
2 files changed, 127 insertions(+), 62 deletions(-)
diff --git a/ui/curses_keys.h b/ui/curses_keys.h
index e9195a167192..71e04acdc756 100644
--- a/ui/curses_keys.h
+++ b/ui/curses_keys.h
@@ -49,22 +49,28 @@
/* curses won't detect a Control + Alt + 1, so use Alt + 1 */
#define QEMU_KEY_CONSOLE0 (2 | ALT) /* (curses2keycode['1'] | ALT) */
+#define CURSES_CHARS0x100 /* Support latin1 only */
#define CURSES_KEYS KEY_MAX /* KEY_MAX defined in */
-static const int curses2keysym[CURSES_KEYS] = {
-[0 ... (CURSES_KEYS - 1)] = -1,
+static const int _curses2keysym[CURSES_CHARS] = {
+[0 ... (CURSES_CHARS - 1)] = -1,
[0x7f] = KEY_BACKSPACE,
['\r'] = KEY_ENTER,
['\n'] = KEY_ENTER,
[27] = 27,
+};
+
+static const int _curseskey2keysym[CURSES_KEYS] = {
+[0 ... (CURSES_KEYS - 1)] = -1,
+
[KEY_BTAB] = '\t' | KEYSYM_SHIFT,
[KEY_SPREVIOUS] = KEY_PPAGE | KEYSYM_SHIFT,
[KEY_SNEXT] = KEY_NPAGE | KEYSYM_SHIFT,
};
-static const int curses2keycode[CURSES_KEYS] = {
-[0 ... (CURSES_KEYS - 1)] = -1,
+static const int _curses2keycode[CURSES_CHARS] = {
+[0 ... (CURSES_CHARS - 1)] = -1,
[0x01b] = 1, /* Escape */
['1'] = 2,
@@ -80,7 +86,6 @@ static const int curses2keycode[CURSES_KEYS] = {
['-'] = 12,
['='] = 13,
[0x07f] = 14, /* Backspace */
-[KEY_BACKSPACE] = 14, /* Backspace */
['\t'] = 15, /* Tab */
['q'] = 16,
@@ -97,7 +102,6 @@ static const int curses2keycode[CURSES_KEYS] = {
[']'] = 27,
['\n'] = 28, /* Return */
['\r'] = 28, /* Return */
-[KEY_ENTER] = 28, /* Return */
['a'] = 30,
['s'] = 31,
@@ -126,33 +130,6 @@ static const int curses2keycode[CURSES_KEYS] = {
[' '] = 57,
-[KEY_F(1)] = 59, /* Function Key 1 */
-[KEY_F(2)] = 60, /* Function Key 2 */
-[KEY_F(3)] = 61, /* Function Key 3 */
-[KEY_F(4)] = 62, /* Function Key 4 */
-[KEY_F(5)] = 63, /* Function Key 5 */
-[KEY_F(6)] = 64, /* Function Key 6 */
-[KEY_F(7)] = 65, /* Function Key 7 */
-[KEY_F(8)] = 66, /* Function Key 8 */
-[KEY_F(9)] = 67, /* Function Key 9 */
-[KEY_F(10)] = 68, /* Function Key 10 */
-[KEY_F(11)] = 87, /* Function Key 11 */
-[KEY_F(12)] = 88, /* Function Key 12 */
-
-[KEY_HOME] = 71 | GREY, /* Home */
-[KEY_UP] = 72 | GREY, /* Up Arrow */
-[KEY_PPAGE] = 73 | GREY, /* Page Up */
-[KEY_LEFT] = 75 | GREY, /* Left Arrow */
-[KEY_RIGHT] = 77 | GREY, /* Right Arrow */
-[KEY_END] = 79 | GREY, /* End */
-[KEY_DOWN] = 80 | GREY, /* Down Arrow */
-[KEY_NPAGE] = 81 | GREY, /* Page Down */
-[KEY_IC] = 82 | GREY, /* Insert */
-[KEY_DC] = 83 | GREY, /* Delete */
-
-[KEY_SPREVIOUS] = 73 | GREY | SHIFT, /* Shift + Page Up */
-[KEY_SNEXT] = 81 | GREY | SHIFT, /* Shift + Page Down */
-
['!'] = 2 | SHIFT,
['@'] = 3 | SHIFT,
['#'] = 4 | SHIFT,
@@ -166,7 +143,6 @@ static const int curses2keycode[CURSES_KEYS] = {
['_'] = 12 | SHIFT,
['+'] = 13 | SHIFT,
-[KEY_BTAB] = 15 | SHIFT, /* Shift + Tab */
['Q'] = 16 | SHIFT,
['W'] = 17 | SHIFT,
['E'] = 18 | SHIFT,
@@ -205,19 +181,6 @@ static const int curses2keycode[CURSES_KEYS] = {
['>'] = 52 | SHIFT,
['?'] = 53 | SHIFT,
-[KEY_F(13)] = 59 | SHIFT, /* Shift + Function Key 1 */
-[KEY_F(14)] = 60 | SHIFT, /* Shift + Function Key 2 */
-[KEY_F(15)] = 61 | SHIFT, /* Shift + Function Key 3 */
-[KEY_F(16)] = 62 | SHIFT, /* Shift + Function Key 4 */
-[KEY_F(17)] = 63 | SHIFT, /* Shift + Function Key 5 */
-[KEY_F(18)] = 64 | SHIFT, /* Shift + Function Key 6 */
-[KEY_F(19)] = 65 | SHIFT, /* Shift + Function Key 7 */
-[KEY_F(20)] = 66 | SHIFT, /* Shift + Function Key 8 */
-[KEY_F(21)] = 67 | SHIFT, /* Shift + Function Key 9 */
-[KEY_F(22)] = 68 | SHIFT, /* Shift + Function Key 10 */
-[KEY_F(23)] = 69 | SHIFT, /* Shift + Function Key 11 */
-[KEY_F(24)] = 70 | SHIFT, /* Shift + Function Key 12 */
-
['Q' - '@'] = 16 | CNTRL, /* Control + q */
['W' - '@'] = 17 | CNTRL, /* Control + w */
['E' - '@'] = 18 | CNTRL, /* Control + e */
[libvirt] [PULL 1/7] Reduce curses escdelay from 1s to 25ms
From: Samuel Thibault
By default, curses will only report single ESC key event after 1s delay,
since ESC is also used for keypad escape sequences. This however makes
users believe that ESC is not working. Reducing to 25ms provides good user
experience, while still allowing 25ms for keypad sequences to get in, which
should be enough.
Signed-off-by: Samuel Thibault
Message-Id: <20190303172557.17139-1-samuel.thiba...@ens-lyon.org>
Signed-off-by: Gerd Hoffmann
---
ui/curses.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ui/curses.c b/ui/curses.c
index 6e0091c3b286..870273de51a9 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -231,7 +231,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
keycode = curses2keycode[chr];
keycode_alt = 0;
-/* alt key */
+/* alt or esc key */
if (keycode == 1) {
int nextchr = getch();
@@ -361,6 +361,7 @@ static void curses_setup(void)
initscr(); noecho(); intrflush(stdscr, FALSE);
nodelay(stdscr, TRUE); nonl(); keypad(stdscr, TRUE);
start_color(); raw(); scrollok(stdscr, FALSE);
+set_escdelay(25);
/* Make color pair to match color format (3bits bg:3bits fg) */
for (i = 0; i < 64; i++) {
--
2.18.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/18] snapshot: Add virDomainSnapshotObjListParse
On 3/4/19 10:34 PM, Eric Blake wrote:
> Add a new function to make it possible to parse a list of snapshots
> at once. This is a counterpart to an earlier patch making it
> possible to produce all snapshots in a single XML string, and
> intentionally parses the same top-level element with
> an optional attribute current='name'.
>
> Note that since we know we started with no relations at all, and
> since checking parent relationships per-snapshot is not viable as
> we don't control which order the snapshots appear in, that we are
> fine with doing a final pass to update all parent/child
> relationships among the definitions.
>
> Signed-off-by: Eric Blake
> ---
> src/conf/snapshot_conf.h | 7 +++
> src/conf/snapshot_conf.c | 111 +++
> src/libvirt_private.syms | 1 +
> 3 files changed, 119 insertions(+)
>
> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
> index 69a7750b0b..f8af991907 100644
> --- a/src/conf/snapshot_conf.h
> +++ b/src/conf/snapshot_conf.h
> @@ -132,6 +132,13 @@ virDomainSnapshotDefPtr
> virDomainSnapshotDefParseNode(xmlDocPtr xml,
>virCapsPtr caps,
>virDomainXMLOptionPtr
> xmlopt,
>unsigned int flags);
> +int virDomainSnapshotObjListParse(const char *xmlStr,
> + const unsigned char *domain_uuid,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr *current_snap,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + unsigned int flags);
> void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def);
> char *virDomainSnapshotDefFormat(const char *uuidstr,
> virDomainSnapshotDefPtr def,
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index a5b05eadf4..52742d82d6 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -507,6 +507,117 @@
> virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
> }
>
>
> +/* Parse a XML entry into snapshots, which must start empty.
> + * Any sub-elements of a must match domain_uuid.
> + */
> +int
> +virDomainSnapshotObjListParse(const char *xmlStr,
> + const unsigned char *domain_uuid,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr *current_snap,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + unsigned int flags)
> +{
> +int ret = -1;
> +xmlDocPtr xml;
> +xmlNodePtr root;
> +xmlXPathContextPtr ctxt = NULL;
> +int n;
> +size_t i;
> +int keepBlanksDefault = xmlKeepBlanksDefault(0);
> +VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
> +VIR_AUTOFREE(char *) current = NULL;
> +
> +if (!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) ||
> +(flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("incorrect flags for bulk parse"));
> +return -1;
> +}
> +if (snapshots->metaroot.nchildren || *current_snap) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("bulk define of snapshots only possible with "
> + "no existing snapshot"));
> +return -1;
> +}
> +
> +if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_snapshot)"
> +goto cleanup;
Could just be return -1
> +
> +root = xmlDocGetRootElement(xml);
> +if (!virXMLNodeNameEqual(root, "snapshots")) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("unexpected root element <%s>, "
> + "expecting "), root->name);
> +goto cleanup;
> +}
> +ctxt = xmlXPathNewContext(xml);
> +if (ctxt == NULL) {
> +virReportOOMError();
> +goto cleanup;
> +}
> +ctxt->node = root;
> +current = virXMLPropString(root, "current");
> +
> +if ((n = virXPathNodeSet("./domainsnapshot", ctxt, )) < 0)
> +goto cleanup;
> +if (!n) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("expected at least one child"));
> +goto cleanup;
> +}
> +
> +for (i = 0; i < n; i++) {
> +virDomainSnapshotDefPtr def;
> +virDomainSnapshotObjPtr snap;
> +
> +def = virDomainSnapshotDefParseNode(xml, nodes[i], caps, xmlopt,
> flags);
> +if (!def)
> +goto cleanup;
> +if (!(snap = virDomainSnapshotAssignDef(snapshots, def))) {
> +virDomainSnapshotDefFree(def);
> +goto cleanup;
> +
Re: [libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate
On Thu, Mar 07, 2019 at 02:38:03PM +0100, Michal Privoznik wrote: Problem is that if there are no signatures for a CPU, then we still allocate cpu->signatures (even though with size 0). Later, we access cpu->signatures[0] if cpu->signatures is not NULL. Invalid read of size 4 at 0x5F439D7: virCPUx86Translate (cpu_x86.c:2930) by 0x5F3C239: virCPUTranslate (cpu.c:927) by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870) ... Address 0xf752d40 is 0 bytes after a block of size 0 alloc'd at 0x4C30EC6: calloc (vg_replace_malloc.c:711) by 0x5DBDE4E: virAllocN (viralloc.c:190) by 0x5F3E4FA: x86ModelCopySignatures (cpu_x86.c:990) by 0x5F3E60F: x86ModelCopy (cpu_x86.c:1008) by 0x5F3E7CB: x86ModelFromCPU (cpu_x86.c:1068) by 0x5F4397E: virCPUx86Translate (cpu_x86.c:2922) by 0x5F3C239: virCPUTranslate (cpu.c:927) by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870) ... Signed-off-by: Michal Privoznik Signed-off-by: Michal Privoznik One sign-off ought to be enough for everybody. Jano --- src/cpu/cpu_x86.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 02/18] snapshot: Rework virDomainSnapshotState enum
On Mon, Mar 04, 2019 at 09:34:29PM -0600, Eric Blake wrote:
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update most clients to use the new
enum names anywhere snapshot state is referenced. The exception is
two switch statements in qemu code, which instead gain a fixme
comment about odd type usage (which will be cleaned up in the next
patch). The rest of the patch is mechanical.
Signed-off-by: Eric Blake
---
src/conf/snapshot_conf.h | 21 ++---
src/conf/snapshot_conf.c | 28 ++--
src/qemu/qemu_driver.c | 34 ++
src/test/test_driver.c | 20 ++--
src/vbox/vbox_common.c | 4 ++--
5 files changed, 66 insertions(+), 41 deletions(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 7a175dfc96..9084f5fb8b 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -36,11 +36,26 @@ typedef enum {
VIR_DOMAIN_SNAPSHOT_LOCATION_LAST
} virDomainSnapshotLocation;
+/**
+ * This enum has to map all known domain states from the public enum
+ * virDomainState, before adding one additional state possible only
+ * for snapshots.
+ */
typedef enum {
-/* Inherit the VIR_DOMAIN_* states from virDomainState. */
-VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
-VIR_DOMAIN_SNAPSHOT_STATE_LAST
+/* Mapped to public enum */
+VIR_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
+VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
+VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
+VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
+VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
+VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
+VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
+VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
+/* Additional enum values local to qemu */
As Eric Blake pointed out in an earlier iteration:
s/qemu/snapshots/
+VIR_SNAP_STATE_DISK_SNAPSHOT,
+VIR_SNAP_STATE_LAST
} virDomainSnapshotState;
+verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
/* Stores disk-snapshot information */
typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 41236d9932..299fc2101b 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -58,7 +58,7 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation,
VIR_DOMAIN_SNAPSHOT_LOCATION_LAST,
);
/* virDomainSnapshotState is really virDomainState plus one extra state */
-VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST,
+VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST,
VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix
"nostate",
"running",
"blocked",
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate
On Thu, Mar 07, 2019 at 14:38:03 +0100, Michal Privoznik wrote:
> Problem is that if there are no signatures for a CPU, then we
> still allocate cpu->signatures (even though with size 0). Later,
> we access cpu->signatures[0] if cpu->signatures is not NULL.
>
> Invalid read of size 4
> at 0x5F439D7: virCPUx86Translate (cpu_x86.c:2930)
> by 0x5F3C239: virCPUTranslate (cpu.c:927)
> by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
> ...
> Address 0xf752d40 is 0 bytes after a block of size 0 alloc'd
> at 0x4C30EC6: calloc (vg_replace_malloc.c:711)
> by 0x5DBDE4E: virAllocN (viralloc.c:190)
> by 0x5F3E4FA: x86ModelCopySignatures (cpu_x86.c:990)
> by 0x5F3E60F: x86ModelCopy (cpu_x86.c:1008)
> by 0x5F3E7CB: x86ModelFromCPU (cpu_x86.c:1068)
> by 0x5F4397E: virCPUx86Translate (cpu_x86.c:2922)
> by 0x5F3C239: virCPUTranslate (cpu.c:927)
> by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
> ...
> Signed-off-by: Michal Privoznik
>
> Signed-off-by: Michal Privoznik
> ---
> src/cpu/cpu_x86.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 92941d1287..c8697819ab 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -987,6 +987,9 @@ x86ModelCopySignatures(virCPUx86ModelPtr dst,
> {
> size_t i;
>
> +if (src->nsignatures == 0)
> +return 0;
> +
> if (VIR_ALLOC_N(dst->signatures, src->nsignatures) < 0)
> return -1;
>
Yes, this is what it should have looked like from the beginning :-)
> @@ -2926,7 +2929,7 @@ virCPUx86Translate(virCPUDefPtr cpu,
> virCPUx86DataAddCPUIDInt(>data, >vendor->cpuid) < 0)
> goto cleanup;
>
> -if (model->signatures &&
> +if (model->nsignatures &&
> x86DataAddSignature(>data, model->signatures[0]) < 0)
> goto cleanup;
I don't think this is necessary, but if we're going to change this, we
should change all places where we decide on model->signatures. Anyway,
the first hunk is sufficient to fix this bug so with the second hunk
removed:
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu_domain: add a PPC64 memLockLimit helper
On Tue, Mar 05, 2019 at 09:46:07AM -0300, Daniel Henrique Barboza wrote:
> There are a lot of documentation in the comments about how
> PPC64 handles passthrough VFIO devices to calculate the
> memLockLimit. And more will be added with the PPC64 NVLink2
> support code.
>
> Let's remove the PPC64 code from qemuDomainGetMemLockLimitBytes
> body and put it into a helper function. This will simply the
s/simply/simplify
> flow of qemuDomainGetMemLockLimitBytes that handles all other
> platforms and improves the readability of PPC64 specifics.
>
> Suggested-by: Erik Skultety
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_domain.c | 169 ++---
> 1 file changed, 91 insertions(+), 78 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 099097fe62..77548c224c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10343,6 +10343,95 @@ qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm)
> }
>
>
> +/**
> + * getPPC64MemLockLimitBytes:
> + * @def: domain definition
> + *
> + * A PPC64 helper that calculates the memory locking limit in order for
> + * the guest to operate properly.
> + */
> +static unsigned long long
> +getPPC64MemLockLimitBytes(virDomainDefPtr def)
> +{
> +unsigned long long memKB = 0;
> +unsigned long long baseLimit, memory, maxMemory;
One variable per line.
[...]
I'll squash this in before pushing:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 77548c224c..535ee78c6f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10354,7 +10354,9 @@ static unsigned long long
getPPC64MemLockLimitBytes(virDomainDefPtr def)
{
unsigned long long memKB = 0;
-unsigned long long baseLimit, memory, maxMemory;
+unsigned long long baseLimit = 0;
+unsigned long long memory = 0;
+unsigned long long maxMemory = 0;
unsigned long long passthroughLimit = 0;
size_t i, nPCIHostBridges = 0;
bool usesVFIO = false;
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/18] snapshot: Split out virDomainSnapshotRedefineValidate helper
On 3/4/19 10:34 PM, Eric Blake wrote: > Pull out the portion of virDomainSnapshotRefinePrep() that deals > with definition sanity into a separate helper routine that can > be reused with bulk redefine, leaving behind only the code > specific to loop checking and in-place updates that are only > needed in single-definition handling. > > Signed-off-by: Eric Blake > --- > src/conf/snapshot_conf.c | 186 --- > 1 file changed, 96 insertions(+), 90 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/18] qemu: Refactor snapshot check for _LIVE vs. _REDEFINE
On Mon, Mar 04, 2019 at 09:34:28PM -0600, Eric Blake wrote: The current qemu code rejects the combination of the two flags VIR_DOMAIN_SNAPSHOT_CREATE_LIVE in tandem with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, but rather late in the cycle (after the snapshot was already parsed), and with a rather confusing message (complaining that live snapshots require external storage, even if the redefined snapshot already declares external storage). Hoist the rejection message to occur earlier (before parsing any XML, which also aids upcoming patches that will implement bulk redefine), and with a more typical error message about mutually exclusive flags. Signed-off-by: Eric Blake Reviewed-by: John Ferlan --- src/qemu/qemu_driver.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/18] domain: Expand virDomainDefFormatInternal with snapshots
On 3/4/19 10:34 PM, Eric Blake wrote: > Make it possible to grab all snapshot XMLs via a single API > call, by adding a new internal flag, and expanding the struct > used to pass extra data for formatting. > > Signed-off-by: Eric Blake > --- > src/conf/domain_conf.h | 3 +++ > src/conf/domain_conf.c | 23 --- > 2 files changed, 23 insertions(+), 3 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots
On 3/4/19 10:34 PM, Eric Blake wrote:
> Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata()
> are right before freeing the virDomainSnapshotObjList, so it did not
> matter if the list's metaroot (which points to all the defined root
> snapshots) is left inconsistent. But an upcoming patch will want to
> clear all snapshots if a bulk redefine fails partway through, in
> which case things must be reset. Make this work by teaching the
> existing virDomainSnapshotUpdateRelations() to be safe regardless of
> the incoming state of the metaroot (since we don't want to leak that
> internal detail into qemu code), then fixing the qemu code to use
> it after deleting all snapshots.
>
> Signed-off-by: Eric Blake
> ---
> src/conf/snapshot_conf.c | 7 +--
> src/qemu/qemu_domain.c | 4
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
I have some random personal grumbling about the lack of comments for
existing code. What's described in this patch as being done would seem
correct, but my knowledge of snapshot's is 'cursory'.
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 206b05c172..386ec82d15 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload,
> }
>
> /* Populate parent link and child count of all snapshots, with all
> - * relations starting as 0/NULL. Return 0 on success, -1 if a parent
> - * is missing or if a circular relationship was requested. */
> + * assigned defs having relations starting as 0/NULL. Return 0 on
> + * success, -1 if a parent is missing or if a circular relationship
> + * was requested. */
^^
nit: there's an extra space here
> int
> virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
> {
> struct snapshot_set_relation act = { snapshots, 0 };
>
> +snapshots->metaroot.nchildren = 0;
> +snapshots->metaroot.first_child = NULL;
> virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, );
> return act.err;
> }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index db25e1596c..3ac79fa50b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr
> driver,
> rem.err = 0;
> virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
> );
> +if (rem.current)
> +vm->current_snapshot = NULL;
If rem.err != 0, is this still something that should be done
Reviewed-by: John Ferlan
John
> +if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err)
> +rem.err = -1;
>
> return rem.err;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/18] domain: Add struct for future domain format parameters
On 3/4/19 10:34 PM, Eric Blake wrote: > Upcoming patches will add new flags for increasing the amount of > information present in dumpxml, but where the source > of that information is tied to somewhere other than the active > or offline domain sub-definition. Make those extensions easier > by updating internal callers to pass in a struct, rather than > adding new parameters for each extension, so that later patches > only have to patch callers that care about a new member of the > struct rather than all callers. > > I considered updating virDomainDefFormat(), but as there were > so many existing callers, it was easier to just add a new > wrapper function virDomainDefFormatFull() which takes the full > struct, while making the existing interface forward on to the > full one. > > Since all callers are being adjusted anyway, reorder the > parameters of virDomainDefFormatInternal to put buf first, as > that tends to be more typical. > > Signed-off-by: Eric Blake > --- > src/conf/domain_conf.h | 18 ++ > src/conf/domain_conf.c | 33 ++--- > src/conf/snapshot_conf.c| 5 - > src/network/bridge_driver.c | 2 +- > src/qemu/qemu_domain.c | 12 ++-- > 5 files changed, 51 insertions(+), 19 deletions(-) > The structure formatting of _virDomainDefFormatData and friends is a bit non-standard from what is typically done, but not incorrect. Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 03/18] qemu: Use virDomainSnapshotState for switch statements
On 3/4/19 10:34 PM, Eric Blake wrote: > Clean up the previous patch which abused switch on virDomainState > while working with a variable containing virDomainSnapshotState, by > converting the two affected switch statements to now use the right > enum. > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_driver.c | 67 -- > 1 file changed, 32 insertions(+), 35 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 02/18] snapshot: Rework virDomainSnapshotState enum
On 3/4/19 10:34 PM, Eric Blake wrote: > The existing virDomainSnapshotState is a superset of virDomainState, > adding one more state (disk-snapshot) on top of valid domain states. > But as written, the enum cannot be used for gcc validation that all > enum values are covered in a strongly-typed switch condition, because > the enum does not explicitly include the values it is adding to. > > Copy the style used in qemu_blockjob.h of creating new enum names > for every inherited value, and update most clients to use the new > enum names anywhere snapshot state is referenced. The exception is > two switch statements in qemu code, which instead gain a fixme > comment about odd type usage (which will be cleaned up in the next > patch). The rest of the patch is mechanical. > > Signed-off-by: Eric Blake > --- > src/conf/snapshot_conf.h | 21 ++--- > src/conf/snapshot_conf.c | 28 ++-- > src/qemu/qemu_driver.c | 34 ++ > src/test/test_driver.c | 20 ++-- > src/vbox/vbox_common.c | 4 ++-- > 5 files changed, 66 insertions(+), 41 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/4] qemu_domain: simplify non-VFIO memLockLimit calc for PPC64
On Tue, Mar 05, 2019 at 09:46:06AM -0300, Daniel Henrique Barboza wrote: > passthroughLimit is being calculated even if usesVFIO is false. > After that, a if/else conditional is used to check if we're going > to sum it up with baseLimit. > > This patch initializes passthroughLimit to zero and always > return memKB = baseLimit + passthroughLimit. The conditional > is then used to calculate passthroughLimit if usesVFIO is true. > This results in some cycles spared for the usesVFIO=false > scenario, but the real motivation is to make the code simpler > to add an alternative passthroughLimit formula for NVLink2 > passthrough. > > Signed-off-by: Daniel Henrique Barboza > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] cpu: Don't access invalid memory in virCPUx86Translate
Problem is that if there are no signatures for a CPU, then we
still allocate cpu->signatures (even though with size 0). Later,
we access cpu->signatures[0] if cpu->signatures is not NULL.
Invalid read of size 4
at 0x5F439D7: virCPUx86Translate (cpu_x86.c:2930)
by 0x5F3C239: virCPUTranslate (cpu.c:927)
by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
...
Address 0xf752d40 is 0 bytes after a block of size 0 alloc'd
at 0x4C30EC6: calloc (vg_replace_malloc.c:711)
by 0x5DBDE4E: virAllocN (viralloc.c:190)
by 0x5F3E4FA: x86ModelCopySignatures (cpu_x86.c:990)
by 0x5F3E60F: x86ModelCopy (cpu_x86.c:1008)
by 0x5F3E7CB: x86ModelFromCPU (cpu_x86.c:1068)
by 0x5F4397E: virCPUx86Translate (cpu_x86.c:2922)
by 0x5F3C239: virCPUTranslate (cpu.c:927)
by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
...
Signed-off-by: Michal Privoznik
Signed-off-by: Michal Privoznik
---
src/cpu/cpu_x86.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 92941d1287..c8697819ab 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -987,6 +987,9 @@ x86ModelCopySignatures(virCPUx86ModelPtr dst,
{
size_t i;
+if (src->nsignatures == 0)
+return 0;
+
if (VIR_ALLOC_N(dst->signatures, src->nsignatures) < 0)
return -1;
@@ -2926,7 +2929,7 @@ virCPUx86Translate(virCPUDefPtr cpu,
virCPUx86DataAddCPUIDInt(>data, >vendor->cpuid) < 0)
goto cleanup;
-if (model->signatures &&
+if (model->nsignatures &&
x86DataAddSignature(>data, model->signatures[0]) < 0)
goto cleanup;
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] ui: support for authorization control on TLS connections
On Thu, Mar 07, 2019 at 02:27:30PM +0100, Gerd Hoffmann wrote: > On Thu, Mar 07, 2019 at 12:23:58PM +, Daniel P. Berrangé wrote: > > ping - soft freeze is less than a week away & i'd like this to get into > > a ui queue pull request in time for 4.0 if there's no review objections > > I'm aware, I'm busy preparing the pre-freeze pulls. USB is on the list > already, busy with UI right now. VGA comes next. > > Series queued. Thanks for the quick reply ! 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 0/2] ui: support for authorization control on TLS connections
On Thu, Mar 07, 2019 at 12:23:58PM +, Daniel P. Berrangé wrote: > ping - soft freeze is less than a week away & i'd like this to get into > a ui queue pull request in time for 4.0 if there's no review objections I'm aware, I'm busy preparing the pre-freeze pulls. USB is on the list already, busy with UI right now. VGA comes next. Series queued. cheers, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On Thu, Mar 07, 2019 at 01:34:15PM +0100, Erik Skultety wrote: > On Thu, Mar 07, 2019 at 01:23:14PM +0100, Michal Privoznik wrote: > > On 3/7/19 1:07 PM, Daniel P. Berrangé wrote: > > > On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: > > > > > However, looking at the bigger picture is this a safe thing to do? I > > > > > mean, > > > > > imagine the following scenario: > > > > > > > > > > 1) say there is capability X that affects certain part of cmd line. > > > > > And > > > > > assume that those two possibilities are incompatible. If cmd line is > > > > > generated one way then migration to a qemu which has cmd line > > > > > generated the > > > > > other way fails. > > > > > > > > > > 2) in release R we deprecate X and thus do not format it in > > > > > > > > > > in status XML. > > > > > > > > > > 3) user starts a domain D. > > > > > > > > > > 4) user saves D into a file > > > > > > > > > > 5) sysadmin downgrades libvirt to R-1 > > > > > > > > Do we even support downgrade this way? I know we migrate to older > > > > version but > > > > isn't that different? > > > > Okay, forget about save+restore. Let's just keep the domain D running and > > then downgrade libvirt. Now, instead of X affecting cmd line let it affect > > how we talk to qemu on monitor. Since R assumed X always being there then X > > wouldn't be recorded in status XML. Later when R-1 daemon is starting and > > parsing the status XML it won't find X and thus might make wrong decissions. > > For instance, if X would be some capability that affects the way we generate > > PCI addresses for guest devices, then this would really harm the user. > > > > Worse, let just start the domain with R-1, then upgrade to R to test it, > > call an API over D (at which point the status XML is regenerated, now > > without X stored in it), find that it's not working so roll back to R-1 and > > puff. The capability is gone. > > I understand the X_ flags as something we implicitly expect from QEMU, IOW we > bump the minimal version of QEMU we expect (with all the supported distros in > mind) and so we mark a bunch of capabilities with X, if we drop them, how is > what you're describing an issue since the capability is implicitly assumed to > be supported by given version of QEMU regardless of libvirt version? > Maybe I misunderstood your example, but I don't think we can ever mark a > capability as X if that is still not an integral part of that QEMU version. > Therefore, it should not affect the way we construct the cmdline and how we > talk to the monitor, right? I think in general you are right - the X_ flags represent some feature we now assume exists. There's a few exceptions though. The most notable being the first 'KQEMU' which was simply deleted. 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 v2 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs
On 3/7/19 12:55 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
Implementation for yet another part of firmware description
specification. This one covers selecting which files to parse.
There are three locations from which description files can be
loaded. In order of preference, from most generic to most
specific these are:
/usr/share/qemu/firmware
/etc/qemu/firmware
$XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific
one is used. Moreover, if file is empty then it means it is
overriding some generic description and disabling it.
Again, this is described in more details and with nice examples
in firmware.json specification (qemu commit 3a0adfc9bf).
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_firmware.c | 134 +++
src/qemu/qemu_firmware.h | 3 +
2 files changed, 137 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index 8f718ee2a6..a818f60c91 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -21,9 +21,11 @@
#include
#include "qemu_firmware.h"
+#include "configmake.h"
#include "qemu_capabilities.h"
#include "virarch.h"
#include "virfile.h"
+#include "virhash.h"
#include "virjson.h"
#include "virlog.h"
#include "virstring.h"
@@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
return virJSONValueToString(doc, true);
}
+
+
+static int
+qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
+{
+DIR *dirp;
+struct dirent *ent = NULL;
+int rc;
+int ret = -1;
+
+if ((rc = virDirOpenIfExists(, dir)) < 0)
+return -1;
+
+if (rc == 0)
+return 0;
+
+while ((rc = virDirRead(dirp, , dir)) > 0) {
+VIR_AUTOFREE(char *) filename = NULL;
+VIR_AUTOFREE(char *) path = NULL;
+
+if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
+continue;
+
+if (STRPREFIX(ent->d_name, "."))
+continue;
+
+if (VIR_STRDUP(filename, ent->d_name) < 0)
+goto cleanup;
+
+if (virAsprintf(, "%s/%s", dir, filename) < 0)
+goto cleanup;
+
+if (virHashUpdateEntry(files, filename, path) < 0)
+goto cleanup;
+
+path = NULL;
+}
+
+if (rc < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virDirClose();
+return ret;
+}
+
+static int
+qemuFirmwareFilesSorter(const virHashKeyValuePair *a,
+const virHashKeyValuePair *b)
+{
+return strcmp(a->key, b->key);
+}
+
+#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware"
+#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware"
+
+int
+qemuFirmwareFetchConfigs(char ***firmwares)
+{
+VIR_AUTOPTR(virHashTable) files = NULL;
+VIR_AUTOFREE(char *) homeConfig = NULL;
+VIR_AUTOFREE(char *) xdgConfig = NULL;
+VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL;
+virHashKeyValuePairPtr tmp = NULL;
+
+*firmwares = NULL;
+
+if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0)
+return -1;
+
+if (!xdgConfig) {
+VIR_AUTOFREE(char *) home = virGetUserDirectory();
+
+if (!home)
+return -1;
+
+if (virAsprintf(, "%s/.config", home) < 0)
+return -1;
+}
+
+if (virAsprintf(, "%s/qemu/firmware", xdgConfig) < 0)
+return -1;
+
+if (!(files = virHashCreate(10, virHashValueFree)))
+return -1;
+
+if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0)
+return -1;
+
+if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0)
+return -1;
+
+if (qemuFirmwareBuildFileList(files, homeConfig) < 0)
+return -1;
I wonder if it really makes sense to read /root/ for this. Normally we would
only look at the home config for unprivileged daemon, eg we don't look in
/root for finding PKI x509 certs IIRC.
Fair enough. Root is able to put configs into /etc/qemu/firmware
anyways. Will fix this locally for now.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On 3/7/19 1:27 PM, Daniel P. Berrangé wrote: On Thu, Mar 07, 2019 at 01:23:14PM +0100, Michal Privoznik wrote: On 3/7/19 1:07 PM, Daniel P. Berrangé wrote: On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: However, looking at the bigger picture is this a safe thing to do? I mean, imagine the following scenario: 1) say there is capability X that affects certain part of cmd line. And assume that those two possibilities are incompatible. If cmd line is generated one way then migration to a qemu which has cmd line generated the other way fails. 2) in release R we deprecate X and thus do not format it in in status XML. 3) user starts a domain D. 4) user saves D into a file 5) sysadmin downgrades libvirt to R-1 Do we even support downgrade this way? I know we migrate to older version but isn't that different? Okay, forget about save+restore. Let's just keep the domain D running and then downgrade libvirt. Now, instead of X affecting cmd line let it affect how we talk to qemu on monitor. Since R assumed X always being there then X wouldn't be recorded in status XML. Later when R-1 daemon is starting and parsing the status XML it won't find X and thus might make wrong decissions. For instance, if X would be some capability that affects the way we generate PCI addresses for guest devices, then this would really harm the user. Worse, let just start the domain with R-1, then upgrade to R to test it, call an API over D (at which point the status XML is regenerated, now without X stored in it), find that it's not working so roll back to R-1 and puff. The capability is gone. Yeah, downgrades of libvirt are not something we claim is supported. If will often work but we're not guaranteeing it & have broken it in the past, especially for running guests. You might be lucky if you have upgraded & immedaitely downgrade, but if you've made changes to guests while the new libvirt was running all bets are off. Not even in the case I'm describing above? Yep. IMHO if you are upgrading just to test a new version, you should expect to have to stop & start your guests if you see problems after a subsequent downgrade. This kind of testing is not something to be done in production, only in dev/test where VMs are disposable. We should not go out of our way to intentionally break downgrades, but if the best way to change something for new libvirt happens to break dowgrades of running VMs that's acceptable. Okay then. Jano, can you please send v2? I'll review it. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] implement cgroups v2 cpuset controller support
On 2/20/19 3:55 PM, Pavel Hrdina wrote: Pavel Hrdina (4): util: implement virCgroupV2(Set|Get)CpusetMems util: implement virCgroupV2(Set|Get)CpusetMemoryMigrate util: implement virCgroupV2(Set|Get)CpusetCpus util: enable cgroups v2 cpuset controller for threads src/util/vircgroupv2.c | 74 ++ 1 file changed, 74 insertions(+) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On Thu, Mar 07, 2019 at 01:23:14PM +0100, Michal Privoznik wrote: > On 3/7/19 1:07 PM, Daniel P. Berrangé wrote: > > On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: > > > > However, looking at the bigger picture is this a safe thing to do? I > > > > mean, > > > > imagine the following scenario: > > > > > > > > 1) say there is capability X that affects certain part of cmd line. And > > > > assume that those two possibilities are incompatible. If cmd line is > > > > generated one way then migration to a qemu which has cmd line generated > > > > the > > > > other way fails. > > > > > > > > 2) in release R we deprecate X and thus do not format it in > > > > > > > > in status XML. > > > > > > > > 3) user starts a domain D. > > > > > > > > 4) user saves D into a file > > > > > > > > 5) sysadmin downgrades libvirt to R-1 > > > > > > Do we even support downgrade this way? I know we migrate to older version > > > but > > > isn't that different? > > Okay, forget about save+restore. Let's just keep the domain D running and > then downgrade libvirt. Now, instead of X affecting cmd line let it affect > how we talk to qemu on monitor. Since R assumed X always being there then X > wouldn't be recorded in status XML. Later when R-1 daemon is starting and > parsing the status XML it won't find X and thus might make wrong decissions. > For instance, if X would be some capability that affects the way we generate > PCI addresses for guest devices, then this would really harm the user. > > Worse, let just start the domain with R-1, then upgrade to R to test it, > call an API over D (at which point the status XML is regenerated, now > without X stored in it), find that it's not working so roll back to R-1 and > puff. The capability is gone. I understand the X_ flags as something we implicitly expect from QEMU, IOW we bump the minimal version of QEMU we expect (with all the supported distros in mind) and so we mark a bunch of capabilities with X, if we drop them, how is what you're describing an issue since the capability is implicitly assumed to be supported by given version of QEMU regardless of libvirt version? Maybe I misunderstood your example, but I don't think we can ever mark a capability as X if that is still not an integral part of that QEMU version. Therefore, it should not affect the way we construct the cmdline and how we talk to the monitor, right? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On Thu, Mar 07, 2019 at 01:23:14PM +0100, Michal Privoznik wrote: > On 3/7/19 1:07 PM, Daniel P. Berrangé wrote: > > On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: > > > > However, looking at the bigger picture is this a safe thing to do? I > > > > mean, > > > > imagine the following scenario: > > > > > > > > 1) say there is capability X that affects certain part of cmd line. And > > > > assume that those two possibilities are incompatible. If cmd line is > > > > generated one way then migration to a qemu which has cmd line generated > > > > the > > > > other way fails. > > > > > > > > 2) in release R we deprecate X and thus do not format it in > > > > > > > > in status XML. > > > > > > > > 3) user starts a domain D. > > > > > > > > 4) user saves D into a file > > > > > > > > 5) sysadmin downgrades libvirt to R-1 > > > > > > Do we even support downgrade this way? I know we migrate to older version > > > but > > > isn't that different? > > Okay, forget about save+restore. Let's just keep the domain D running and > then downgrade libvirt. Now, instead of X affecting cmd line let it affect > how we talk to qemu on monitor. Since R assumed X always being there then X > wouldn't be recorded in status XML. Later when R-1 daemon is starting and > parsing the status XML it won't find X and thus might make wrong decissions. > For instance, if X would be some capability that affects the way we generate > PCI addresses for guest devices, then this would really harm the user. > > Worse, let just start the domain with R-1, then upgrade to R to test it, > call an API over D (at which point the status XML is regenerated, now > without X stored in it), find that it's not working so roll back to R-1 and > puff. The capability is gone. > > > > > > Yeah, downgrades of libvirt are not something we claim is supported. If > > will often work but we're not guaranteeing it & have broken it in the > > past, especially for running guests. You might be lucky if you have > > upgraded & immedaitely downgrade, but if you've made changes to guests > > while the new libvirt was running all bets are off. > > Not even in the case I'm describing above? Yep. IMHO if you are upgrading just to test a new version, you should expect to have to stop & start your guests if you see problems after a subsequent downgrade. This kind of testing is not something to be done in production, only in dev/test where VMs are disposable. We should not go out of our way to intentionally break downgrades, but if the best way to change something for new libvirt happens to break dowgrades of running VMs that's acceptable. 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 0/2] ui: support for authorization control on TLS connections
ping - soft freeze is less than a week away & i'd like this to get into a ui queue pull request in time for 4.0 if there's no review objections On Wed, Feb 27, 2019 at 02:57:53PM +, Daniel P. Berrangé wrote: > This series provides the VNC parts of the authorization control series > previously posted as: > > v1: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04482.html > v2: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05727.html > v3: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01639.html > v4: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg04319.html > > The core authz framework is now merged & these patches have all had > positive review. Thus these VNC parts are ready to go into the VNC > maintainer's tree, should the maintainer consider them acceptable. > > Note the 2nd patch is modifying the HMP, but I considered it part > of ui/vnc, since that's the only bit of QEMU which ever used these > HMP commands, and it has a dependancy on the first patch merging > first. > > Daniel P. Berrangé (2): > vnc: allow specifying a custom authorization object name > monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, > acl_remove > > monitor.c| 23 ++ > qemu-deprecated.texi | 11 + > qemu-options.hx | 35 ++ > ui/vnc.c | 58 +--- > 4 files changed, 108 insertions(+), 19 deletions(-) > > -- > 2.20.1 > 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 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On 3/7/19 1:07 PM, Daniel P. Berrangé wrote: On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: However, looking at the bigger picture is this a safe thing to do? I mean, imagine the following scenario: 1) say there is capability X that affects certain part of cmd line. And assume that those two possibilities are incompatible. If cmd line is generated one way then migration to a qemu which has cmd line generated the other way fails. 2) in release R we deprecate X and thus do not format it in in status XML. 3) user starts a domain D. 4) user saves D into a file 5) sysadmin downgrades libvirt to R-1 Do we even support downgrade this way? I know we migrate to older version but isn't that different? Okay, forget about save+restore. Let's just keep the domain D running and then downgrade libvirt. Now, instead of X affecting cmd line let it affect how we talk to qemu on monitor. Since R assumed X always being there then X wouldn't be recorded in status XML. Later when R-1 daemon is starting and parsing the status XML it won't find X and thus might make wrong decissions. For instance, if X would be some capability that affects the way we generate PCI addresses for guest devices, then this would really harm the user. Worse, let just start the domain with R-1, then upgrade to R to test it, call an API over D (at which point the status XML is regenerated, now without X stored in it), find that it's not working so roll back to R-1 and puff. The capability is gone. Yeah, downgrades of libvirt are not something we claim is supported. If will often work but we're not guaranteeing it & have broken it in the past, especially for running guests. You might be lucky if you have upgraded & immedaitely downgrade, but if you've made changes to guests while the new libvirt was running all bets are off. Not even in the case I'm describing above? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On Thu, Mar 07, 2019 at 12:52:46PM +0100, Erik Skultety wrote: > > However, looking at the bigger picture is this a safe thing to do? I mean, > > imagine the following scenario: > > > > 1) say there is capability X that affects certain part of cmd line. And > > assume that those two possibilities are incompatible. If cmd line is > > generated one way then migration to a qemu which has cmd line generated the > > other way fails. > > > > 2) in release R we deprecate X and thus do not format it in > > in status XML. > > > > 3) user starts a domain D. > > > > 4) user saves D into a file > > > > 5) sysadmin downgrades libvirt to R-1 > > Do we even support downgrade this way? I know we migrate to older version but > isn't that different? Yeah, downgrades of libvirt are not something we claim is supported. If will often work but we're not guaranteeing it & have broken it in the past, especially for running guests. You might be lucky if you have upgraded & immedaitely downgrade, but if you've made changes to guests while the new libvirt was running all bets are off. 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 v2 00/15] Firmware auto selection
On Thu, Mar 07, 2019 at 10:29:10AM +0100, Michal Privoznik wrote: > v2 of: > > https://www.redhat.com/archives/libvir-list/2019-February/msg01503.html > > As usual, you can find my patches at my github: > > https://github.com/zippy2/libvirt/commits/firmware_v2 > > diff to v1: > - Hopefully all Lazlo's comment are worked in (fixing the logic that > chooses suitable firmware for secboot/SMM domains, commit message > adjustements, sanity check for parsed FW descriptions, etc.) > > - Added more debug messages around FW selection code, i.e. it'll be > visible from the logs why given FS is not suitable (or that it is). I've done a very quick review of the patches and the design and overall implementation strategy looks good to me. A few misc points - We ought to expose the list of firmware types supported in the domain capabilities, so mgmt apps can probe if uefi is available - What should we do about the 'nvram' config from /etc/libvirt/qemu.conf We can't get rid of it for a while, since the new firmware configs won't be widely supported by distros in forseeable future. In the places which currently use the 'nvram' config though, should we make sure we consult the firmware configs in prefereence to resolve the var store ? Maybe your code is already doing this and I missed the logic from the diffs. - We support 'bios' and 'efi' right now, but QEMU also reoprts 'openfirmware' and 'uboot' as supported types. In practice no config files probably exist for those yet, but if it is easy to make the code support them we should try todo it to get parity with the QEMU spec I don't think any of these are blockers though - all would work as followup patches I expect. 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 v2 09/15] qemu_firmware: Introduce qemuFirmwareFetchConfigs
On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
> Implementation for yet another part of firmware description
> specification. This one covers selecting which files to parse.
>
> There are three locations from which description files can be
> loaded. In order of preference, from most generic to most
> specific these are:
>
> /usr/share/qemu/firmware
> /etc/qemu/firmware
> $XDG_CONFIG_HOME/qemu/firmware
>
> If a file is found in two or more locations then the most specific
> one is used. Moreover, if file is empty then it means it is
> overriding some generic description and disabling it.
>
> Again, this is described in more details and with nice examples
> in firmware.json specification (qemu commit 3a0adfc9bf).
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_firmware.c | 134 +++
> src/qemu/qemu_firmware.h | 3 +
> 2 files changed, 137 insertions(+)
>
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 8f718ee2a6..a818f60c91 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -21,9 +21,11 @@
> #include
>
> #include "qemu_firmware.h"
> +#include "configmake.h"
> #include "qemu_capabilities.h"
> #include "virarch.h"
> #include "virfile.h"
> +#include "virhash.h"
> #include "virjson.h"
> #include "virlog.h"
> #include "virstring.h"
> @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
>
> return virJSONValueToString(doc, true);
> }
> +
> +
> +static int
> +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
> +{
> +DIR *dirp;
> +struct dirent *ent = NULL;
> +int rc;
> +int ret = -1;
> +
> +if ((rc = virDirOpenIfExists(, dir)) < 0)
> +return -1;
> +
> +if (rc == 0)
> +return 0;
> +
> +while ((rc = virDirRead(dirp, , dir)) > 0) {
> +VIR_AUTOFREE(char *) filename = NULL;
> +VIR_AUTOFREE(char *) path = NULL;
> +
> +if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
> +continue;
> +
> +if (STRPREFIX(ent->d_name, "."))
> +continue;
> +
> +if (VIR_STRDUP(filename, ent->d_name) < 0)
> +goto cleanup;
> +
> +if (virAsprintf(, "%s/%s", dir, filename) < 0)
> +goto cleanup;
> +
> +if (virHashUpdateEntry(files, filename, path) < 0)
> +goto cleanup;
> +
> +path = NULL;
> +}
> +
> +if (rc < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +virDirClose();
> +return ret;
> +}
> +
> +static int
> +qemuFirmwareFilesSorter(const virHashKeyValuePair *a,
> +const virHashKeyValuePair *b)
> +{
> +return strcmp(a->key, b->key);
> +}
> +
> +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware"
> +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware"
> +
> +int
> +qemuFirmwareFetchConfigs(char ***firmwares)
> +{
> +VIR_AUTOPTR(virHashTable) files = NULL;
> +VIR_AUTOFREE(char *) homeConfig = NULL;
> +VIR_AUTOFREE(char *) xdgConfig = NULL;
> +VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL;
> +virHashKeyValuePairPtr tmp = NULL;
> +
> +*firmwares = NULL;
> +
> +if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0)
> +return -1;
> +
> +if (!xdgConfig) {
> +VIR_AUTOFREE(char *) home = virGetUserDirectory();
> +
> +if (!home)
> +return -1;
> +
> +if (virAsprintf(, "%s/.config", home) < 0)
> +return -1;
> +}
> +
> +if (virAsprintf(, "%s/qemu/firmware", xdgConfig) < 0)
> +return -1;
> +
> +if (!(files = virHashCreate(10, virHashValueFree)))
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0)
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0)
> +return -1;
> +
> +if (qemuFirmwareBuildFileList(files, homeConfig) < 0)
> +return -1;
I wonder if it really makes sense to read /root/ for this. Normally we would
only look at the home config for unprivileged daemon, eg we don't look in
/root for finding PKI x509 certs IIRC.
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 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
> However, looking at the bigger picture is this a safe thing to do? I mean, > imagine the following scenario: > > 1) say there is capability X that affects certain part of cmd line. And > assume that those two possibilities are incompatible. If cmd line is > generated one way then migration to a qemu which has cmd line generated the > other way fails. > > 2) in release R we deprecate X and thus do not format it in > in status XML. > > 3) user starts a domain D. > > 4) user saves D into a file > > 5) sysadmin downgrades libvirt to R-1 Do we even support downgrade this way? I know we migrate to older version but isn't that different? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/15] virmock: Initialize both symbols in VIR_MOCK_REAL_INIT_ALT
On Thu, Mar 07, 2019 at 10:29:11AM +0100, Michal Privoznik wrote: > It may happen that both symbols are present. Especially when > chaining mocks. For instance if a test is using virpcimock and > then both stat and __xstat would be present in the address space > as virpcimock implements both. Then, if the test would try to use > say virfilewrapper (which again uses VIR_MOCK_REAL_INIT_ALT() to > init real_stat and real___xstat) it would find stat() from > virpcimock and stop there. The virfilewrapper.c:real___xstat > wouldn't be initialized and thus it may result in a segfault. > > The reason for segfault is that sys/stat.h may redefine stat() to > call __xstat(). > > Signed-off-by: Michal Privoznik > --- > tests/virmock.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé This looks independant of the series so can just be pushed 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] docs: remove Google+ link from page footer
On Thu, Mar 07, 2019 at 12:27:44PM +0100, Peter Krempa wrote: > On Thu, Mar 07, 2019 at 11:07:40 +, Daniel Berrange wrote: > > Google is shutting down Google+, with no replacement, in the very near > > future so we are loosing the Libvirt community group there. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > docs/page.xsl | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/docs/page.xsl b/docs/page.xsl > > index c1804eab34..4698e2789e 100644 > > --- a/docs/page.xsl > > +++ b/docs/page.xsl > > @@ -175,7 +175,6 @@ > > Community > > > > > href="https://twitter.com/hashtag/libvirt;>twitter > > How about deleting this one as well? Asking questions on twitter will > not get you resposne from much of the libvirt developer comunity. Not every site has to be about getting help from developers. Sharing information & publicising content is relevant too. 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 3/7] util: Make virStringMatchesNameSuffix() return bool
On Thu, Mar 07, 2019 at 11:14:42 +0100, Andrea Bolognani wrote: > It's a predicate, so bool is the appropriate return type. > > Signed-off-by: Andrea Bolognani > --- > src/util/virstring.c | 6 +++--- > src/util/virstring.h | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] util: Make virStringStripSuffix() return bool
On Thu, Mar 07, 2019 at 11:14:41 +0100, Andrea Bolognani wrote: > While this function is not, strictly speaking, a predicate, > it still mostly behaves like one as evidenced by the vast > majority of its callers, so using bool rather than int as > the return type makes sense. > > Signed-off-by: Andrea Bolognani > --- > src/util/virstring.c | 8 > src/util/virstring.h | 4 ++-- > tests/testutilsqemu.c | 2 +- > 3 files changed, 7 insertions(+), 7 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] util: Make virStringHasCaseSuffix() return bool
On Thu, Mar 07, 2019 at 11:14:40 +0100, Andrea Bolognani wrote: > It's a predicate, so bool is the appropriate return type. > > Signed-off-by: Andrea Bolognani > --- ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] util: Tweak virStringHasSuffix()
On Thu, Mar 07, 2019 at 11:14:43 +0100, Andrea Bolognani wrote: > Make it more similar to virStringStripSuffix(), which also > results in types being more correct since STRNEQ() returns > int rather than bool. # define STREQ(a, b) (strcmp(a, b) == 0) # define STRNEQ(a, b) (strcmp(a, b) != 0) > Signed-off-by: Andrea Bolognani > --- > src/util/virstring.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/util/virstring.c b/src/util/virstring.c > index f669d59f58..a938669b63 100644 > --- a/src/util/virstring.c > +++ b/src/util/virstring.c > @@ -1245,7 +1245,10 @@ virStringHasSuffix(const char *str, > if (len < suffixlen) > return false; > > -return STREQ(str + len - suffixlen, suffix); > +if (STRNEQ(str + len - suffixlen, suffix)) > +return false; > + > +return true; NACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: use VIR_AUTOUNREF in storagepoolcapstest
On Thu, Mar 07, 2019 at 12:13:00 +0100, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > tests/storagepoolcapstest.c | 31 +++ > 1 file changed, 11 insertions(+), 20 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: remove Google+ link from page footer
On Thu, Mar 07, 2019 at 11:07:40AM +, Daniel P. Berrangé wrote: > Google is shutting down Google+, with no replacement, in the very near > future so we are loosing the Libvirt community group there. > > Signed-off-by: Daniel P. Berrangé > --- > docs/page.xsl | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: remove Google+ link from page footer
On Thu, Mar 07, 2019 at 11:07:40AM +, Daniel P. Berrangé wrote: Google is shutting down Google+, with no replacement, in the very near future so we are loosing the Libvirt community group there. s/loosing/losing/ Signed-off-by: Daniel P. Berrangé --- docs/page.xsl | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/page.xsl b/docs/page.xsl index c1804eab34..4698e2789e 100644 --- a/docs/page.xsl +++ b/docs/page.xsl @@ -175,7 +175,6 @@ Community https://twitter.com/hashtag/libvirt;>twitter Hopefully Twitter will be the next one to go. - https://plus.google.com/communities/109522598353007505282;>google+ http://stackoverflow.com/questions/tagged/libvirt;>stackoverflow http://serverfault.com/questions/tagged/libvirt;>serverfault Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: remove Google+ link from page footer
On Thu, Mar 07, 2019 at 11:07:40 +, Daniel Berrange wrote: > Google is shutting down Google+, with no replacement, in the very near > future so we are loosing the Libvirt community group there. > > Signed-off-by: Daniel P. Berrangé > --- > docs/page.xsl | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/docs/page.xsl b/docs/page.xsl > index c1804eab34..4698e2789e 100644 > --- a/docs/page.xsl > +++ b/docs/page.xsl > @@ -175,7 +175,6 @@ > Community > > href="https://twitter.com/hashtag/libvirt;>twitter How about deleting this one as well? Asking questions on twitter will not get you resposne from much of the libvirt developer comunity. > - href="https://plus.google.com/communities/109522598353007505282;>google+ ACK. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: use VIR_AUTOUNREF in storagepoolcapstest
Signed-off-by: Pavel Hrdina
---
tests/storagepoolcapstest.c | 31 +++
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/tests/storagepoolcapstest.c b/tests/storagepoolcapstest.c
index f8b560c2f0..518a5fdfe7 100644
--- a/tests/storagepoolcapstest.c
+++ b/tests/storagepoolcapstest.c
@@ -54,30 +54,26 @@ test_virStoragePoolCapsFormat(const void *opaque)
struct test_virStoragePoolCapsFormatData *data =
(struct test_virStoragePoolCapsFormatData *) opaque;
virCapsPtr driverCaps = data->driverCaps;
-virStoragePoolCapsPtr poolCaps = NULL;
-int ret = -1;
+VIR_AUTOUNREF(virStoragePoolCapsPtr) poolCaps = NULL;
VIR_AUTOFREE(char *) path = NULL;
VIR_AUTOFREE(char *) poolCapsXML = NULL;
if (!(poolCaps = virStoragePoolCapsNew(driverCaps)))
-goto cleanup;
+return -1;
if (virAsprintf(, "%s/storagepoolcapsschemadata/poolcaps-%s.xml",
-abs_srcdir, data->filename) < 0)
-goto cleanup;
+abs_srcdir, data->filename) < 0) {
+return -1;
+}
if (!(poolCapsXML = virStoragePoolCapsFormat(poolCaps)))
-goto cleanup;
+return -1;
if (virTestCompareToFile(poolCapsXML, path) < 0)
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-virObjectUnref(poolCaps);
-return ret;
+return 0;
}
@@ -85,8 +81,8 @@ static int
mymain(void)
{
int ret = 0;
-virCapsPtr fullCaps = NULL;
-virCapsPtr fsCaps = NULL;
+VIR_AUTOUNREF(virCapsPtr) fullCaps = NULL;
+VIR_AUTOUNREF(virCapsPtr) fsCaps = NULL;
#define DO_TEST(Filename, DriverCaps) \
do { \
@@ -98,8 +94,7 @@ mymain(void)
if (!(fullCaps = virCapabilitiesNew(VIR_ARCH_NONE, false, false)) ||
!(fsCaps = virCapabilitiesNew(VIR_ARCH_NONE, false, false))) {
-ret = -1;
-goto cleanup;
+return -1;
}
test_virCapabilitiesAddFullStoragePool(fullCaps);
@@ -108,10 +103,6 @@ mymain(void)
DO_TEST("full", fullCaps);
DO_TEST("fs", fsCaps);
- cleanup:
-virObjectUnref(fullCaps);
-virObjectUnref(fsCaps);
-
return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: remove Google+ link from page footer
Google is shutting down Google+, with no replacement, in the very near future so we are loosing the Libvirt community group there. Signed-off-by: Daniel P. Berrangé --- docs/page.xsl | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/page.xsl b/docs/page.xsl index c1804eab34..4698e2789e 100644 --- a/docs/page.xsl +++ b/docs/page.xsl @@ -175,7 +175,6 @@ Community https://twitter.com/hashtag/libvirt;>twitter - https://plus.google.com/communities/109522598353007505282;>google+ http://stackoverflow.com/questions/tagged/libvirt;>stackoverflow http://serverfault.com/questions/tagged/libvirt;>serverfault -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Define XDG variables for hypervisor domains
On Thu, Mar 07, 2019 at 10:37:14AM +, Daniel P. Berrangé wrote:
> On Thu, Mar 07, 2019 at 09:27:29AM +0100, Erik Skultety wrote:
> > On Wed, Mar 06, 2019 at 04:05:13PM +, Daniel P. Berrangé wrote:
> > > On Wed, Mar 06, 2019 at 03:41:16PM +0100, Erik Skultety wrote:
> > > > So, according to this report [1], Mesa enabled on-disk cache for its
> > > > shaders
> > > > which it tries to create either under XDG_CACHE_HOME/mesa_shader_cache
> > > > or under
> > > > HOME/.cache/mesa_shader_cache. Since libvirt doesn't set XDGs and HOME
> > > > is only
> > > > passed iff the QEMU process isn't run with SUID, Mesa will default to
> > > > the home
> > > > directory specified in /etc/passwd.
> > > > Because of this, Mesa will fail to create the cache and QEMU refuses to
> > > > start
> > > > the VM (interestingly enough, Intel reports the problem too, but Mesa
> > > > disables
> > > > the cache and the VM starts). This series proposes usage of XDG
> > > > variables to
> > > > fix the problem by pointing XDG variables to hypervisor specific
> > > > directory
> > > > under libvirt's libDir. Additionally, HOME is also enforced for all VM
> > > > processes and points to the base hypervisor libDir directory.
> > > > To illustrate this further:
> > > >
> > > > system qemu:
> > > > HOME=/var/lib/libvirt/qemu/domain-5-f-live \
> > > > XDG_DATA_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.local/share \
> > > > XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.cache \
> > > > XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.config \
> > > >
> > > > session qemu:
> > > > HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live \
> > > > XDG_DATA_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.local/share
> > > > \
> > > > XDG_CACHE_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.cache
> > > > \
> > > > XDG_CONFIG_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.config
> > > > \
> > >
> > > For the bug report IIUC we should only need to set XDG_CACHE_HOME
> > > as that's the one which needs to be writable and which SELinux quite
> > > rightly denies. It is also safe to redefine this because cache data
> > > is liable to be purged at any time, so apps must expect to see an
> > > empty cache directory.
> >
> > Yes, XDG_CACHE_HOME is the only one needed to fix the BZ.
> >
> > >
> > > I'm very wary about setting HOME or XDG_DATA_HOME or XDG_CONFIG_HOME
> > > as I think that is pretty likely to break valid usage.
> > >
> > > For example, when QEMU is configured to use pulseaudio as its audio
> > > backend, I believe this will want to read config files in normal
> > > $HOME and/or $XDG_{CONFIG|DATA}_HOME.
> >
> > I'm glad you mentioned pulseaudio, it's a good example, because pulseaudio
> > should never run as root, instead, it will run in context of the 'qemu'
> > user,
> > right? This is all fine in session mode, but in system mode, having set
> > none of
> > the above apart from cache, pulseaudio might try to read from '/' as that's
> > QEMU's home dir. Any reasonable app should default to reading configs and
> > data
> > from /etc or /usr/local if there's nothing found in HOME or XDG location.
> > But
> > when it comes to writing, having apps write anything to root's home doesn't
> > seem like a good idea to me, so I believe that at least for system mode, we
> > want to override all of the variables.
>
> Yes, ok, for system mode setting all of them is sensible for the reasons
> you say.
>
> > I'd also be okay with not redefining the variables for session mode, but the
> > following motivated me to do so:
> > 1) consistency, if we do it in 1 mode, why not the other (although
> >personally, I don't like enforcing session mode that much)
>
> To me one of key reasons for session mode (aside from access to disk images
> in $HOME) is that it allows VMs to integrate with services / apps in that
> users session, such as PA. So isolating them is going to eliminate that
> key feture of session mode & break valid usage IMHO.
>
> > 2) isolation, configs are fine, but any data an app wants to write, I'm
> >really not sure how they handle multiple instances, so I approached
> > this
> >like non-shared filesystems, any 2 apps need to have their own view
> > of
> >the location and I also don't want them to even think of trying to
> > write
> >something to root's home
>
> We don't really expect QEMU (or things it links to) to write to these
> dirs, but if they did try SELinux would block it. IMHO if you wanted
> stricter isolation then QEMU needs to be running a different UID.
>
> > > Similary if using the GTK frontend these dirs are needed to load
> > > the toolkit preferences such as theme.
> >
> > You mean for ? If so, we don't support GTK.
>
> Yes for graphics. Lack of GTK support is a bug :-)
Alright, thanks for comments, I'll respin with the changes we talked about.
Erik
--
libvir-list mailing list
Re: [libvirt] [PATCH 2/8] Introduce virQEMUCapsDeprecated array
On Thu, Mar 07, 2019 at 11:37:00AM +0100, Michal Privoznik wrote:
> On 2/21/19 4:42 PM, Ján Tomko wrote:
> > To prevent domains started with older libvirtd from disappearing, we
> > need to be able to parse the string representation of QEMU capabilities,
> > even if we no longer use them in the code.
> >
> > Put those in a separate array where we won't bother with enum constants.
> >
> > Signed-off-by: Ján Tomko
> > ---
> > src/qemu/qemu_capabilities.c | 95
> > src/qemu/qemu_capabilities.h | 1 +
> > 2 files changed, 96 insertions(+)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index b48bcbebee..7160860ab4 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -524,6 +524,101 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> > "scsi-disk.device_id",
> > );
> > +#define VIR_QEMU_CAPS_DEPRECATED_LAST 92 /* chosen by fair dice roll */
>
> Problem I have with this apporach is that as soon as we deprecate some other
> capability we have to change this number too. Since we don't need str<->enum
> translation can't we just have a static array of these strings and then in
> 6/8 use virStringListHasString() instead of
> virQEMUCapsDeprecatedTypeFromString()?
Yes, I'm really not seeing a benefit from this change. We haven't
really eliminated the enums, because the enums constants are still
there in the comments - merely not in the headers. Since it turns
something that was previously compile time guaranteed correct into
a magic constant we can screw up it is arguably a step backwards.
I could perhaps see us splitting the enums
typedef enum {
X_QEMU_CAPS_KQEMU,
X_QEMU_CAPS_VNC_COLON,
X_QEMU_CAPS_NO_REBOOT,
...
X_QEMU_CAPS_DISPLAY,
X_QEMU_CAPS_LAST
} virQEMUCapsDeprecated
typedef enum {
QEMU_CAPS_KVM = X_QEMU_CAPS_LAST
} virQEMUCaps;
This would need an VIR_ENUM_IMPL_OFFSET() which let us
pass in the offset value of the first enum constant.
Not sure it buys us very much over what we have today
though.
>
> > +VIR_ENUM_IMPL(virQEMUCapsDeprecated, VIR_QEMU_CAPS_DEPRECATED_LAST,
> > +"kqemu", /* X_QEMU_CAPS_KQEMU, Whether KQEMU is compiled in */
> > +"vnc-colon", /* X_QEMU_CAPS_VNC_COLON, VNC takes or address + display
> > */
> > +"no-reboot", /* X_QEMU_CAPS_NO_REBOOT, Is the -no-reboot flag
> > available */
> > +"drive", /* X_QEMU_CAPS_DRIVE, Is the new -drive arg available */
> > +"drive-boot", /* X_QEMU_CAPS_DRIVE_BOOT, Does -drive support boot=on */
> > +"name", /* X_QEMU_CAPS_NAME, Is the -name flag available */
> > +"uuid", /* X_QEMU_CAPS_UUID, Is the -uuid flag available */
> > +"domid", /* X_QEMU_CAPS_DOMID, Xenner: -domid flag available */
> > +"vnet-hdr", /* X_QEMU_CAPS_VNET_HDR */
> > +"migrate-kvm-stdio", /* X_QEMU_CAPS_MIGRATE_KVM_STDIO, avoid kvm tcp
> > migration bug */
> > +"migrate-qemu-tcp", /* X_QEMU_CAPS_MIGRATE_QEMU_TCP, have qemu tcp
> > migration */
> > +"migrate-qemu-exec", /* X_QEMU_CAPS_MIGRATE_QEMU_EXEC, have qemu exec
> > migration */
> > +"drive-cache-v2", /* X_QEMU_CAPS_DRIVE_CACHE_V2, cache= flag wanting
> > new v2 values */
> > +"drive-format", /* X_QEMU_CAPS_DRIVE_FORMAT, Is -drive format= avail */
> > +"vga", /* X_QEMU_CAPS_VGA, Is -vga avail */
> > +"0.10", /* X_QEMU_CAPS_0_10, features added in qemu-0.10.0 or later */
> > +"pci-device", /* X_QEMU_CAPS_PCIDEVICE, PCI device assignment
> > supported */
> > +"mem-path", /* X_QEMU_CAPS_MEM_PATH, mmap'ped guest backing supported
> > */
> > +"drive-serial", /* X_QEMU_CAPS_DRIVE_SERIAL, -driver serial=
> > available */
> > +"xen-domid", /* X_QEMU_CAPS_XEN_DOMID, -xen-domid */
> > +"migrate-qemu-unix", /* X_QEMU_CAPS_MIGRATE_QEMU_UNIX, qemu migration
> > via unix sockets */
> > +"chardev", /* X_QEMU_CAPS_CHARDEV, Is the new -chardev arg available */
> > +"enable-kvm", /* X_QEMU_CAPS_ENABLE_KVM, -enable-kvm flag */
> > +"monitor-json", /* X_QEMU_CAPS_MONITOR_JSON, JSON mode for monitor */
> > +"balloon", /* X_QEMU_CAPS_BALLOON, -balloon available */
> > +"device", /* X_QEMU_CAPS_DEVICE, Is the -device arg available */
> > +"sdl", /* X_QEMU_CAPS_SDL, Is the new -sdl arg available */
> > +"smp-topology", /* X_QEMU_CAPS_SMP_TOPOLOGY, -smp has
> > sockets/cores/threads */
> > +"netdev", /* X_QEMU_CAPS_NETDEV, -netdev flag & netdev_add/remove */
> > +"rtc", /* X_QEMU_CAPS_RTC, The -rtc flag for clock options */
> > +"vhost-net", /* X_QEMU_CAPS_VHOST_NET, vhost-net support available */
> > +"rtc-td-hack", /* X_QEMU_CAPS_RTC_TD_HACK, -rtc-td-hack available */
> > +"no-kvm-pit", /* X_QEMU_CAPS_NO_KVM_PIT, -no-kvm-pit-reinjection
> > supported */
> > +"tdf", /* X_QEMU_CAPS_TDF, -tdf flag (user-mode pit catchup) */
> > +"pci-configfd", /* X_QEMU_CAPS_PCI_CONFIGFD,
Re: [libvirt] [PATCH 0/3] Define XDG variables for hypervisor domains
On Thu, Mar 07, 2019 at 09:27:29AM +0100, Erik Skultety wrote:
> On Wed, Mar 06, 2019 at 04:05:13PM +, Daniel P. Berrangé wrote:
> > On Wed, Mar 06, 2019 at 03:41:16PM +0100, Erik Skultety wrote:
> > > So, according to this report [1], Mesa enabled on-disk cache for its
> > > shaders
> > > which it tries to create either under XDG_CACHE_HOME/mesa_shader_cache or
> > > under
> > > HOME/.cache/mesa_shader_cache. Since libvirt doesn't set XDGs and HOME is
> > > only
> > > passed iff the QEMU process isn't run with SUID, Mesa will default to the
> > > home
> > > directory specified in /etc/passwd.
> > > Because of this, Mesa will fail to create the cache and QEMU refuses to
> > > start
> > > the VM (interestingly enough, Intel reports the problem too, but Mesa
> > > disables
> > > the cache and the VM starts). This series proposes usage of XDG variables
> > > to
> > > fix the problem by pointing XDG variables to hypervisor specific directory
> > > under libvirt's libDir. Additionally, HOME is also enforced for all VM
> > > processes and points to the base hypervisor libDir directory.
> > > To illustrate this further:
> > >
> > > system qemu:
> > > HOME=/var/lib/libvirt/qemu/domain-5-f-live \
> > > XDG_DATA_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.local/share \
> > > XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.cache \
> > > XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain-5-f-live/.config \
> > >
> > > session qemu:
> > > HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live \
> > > XDG_DATA_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.local/share
> > > \
> > > XDG_CACHE_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.cache
> > > \
> > > XDG_CONFIG_HOME=/home/eskultet/.config/libvirt/qemu/lib/domain-4-f-live/.config
> > > \
> >
> > For the bug report IIUC we should only need to set XDG_CACHE_HOME
> > as that's the one which needs to be writable and which SELinux quite
> > rightly denies. It is also safe to redefine this because cache data
> > is liable to be purged at any time, so apps must expect to see an
> > empty cache directory.
>
> Yes, XDG_CACHE_HOME is the only one needed to fix the BZ.
>
> >
> > I'm very wary about setting HOME or XDG_DATA_HOME or XDG_CONFIG_HOME
> > as I think that is pretty likely to break valid usage.
> >
> > For example, when QEMU is configured to use pulseaudio as its audio
> > backend, I believe this will want to read config files in normal
> > $HOME and/or $XDG_{CONFIG|DATA}_HOME.
>
> I'm glad you mentioned pulseaudio, it's a good example, because pulseaudio
> should never run as root, instead, it will run in context of the 'qemu' user,
> right? This is all fine in session mode, but in system mode, having set none
> of
> the above apart from cache, pulseaudio might try to read from '/' as that's
> QEMU's home dir. Any reasonable app should default to reading configs and data
> from /etc or /usr/local if there's nothing found in HOME or XDG location. But
> when it comes to writing, having apps write anything to root's home doesn't
> seem like a good idea to me, so I believe that at least for system mode, we
> want to override all of the variables.
Yes, ok, for system mode setting all of them is sensible for the reasons
you say.
> I'd also be okay with not redefining the variables for session mode, but the
> following motivated me to do so:
> 1) consistency, if we do it in 1 mode, why not the other (although
>personally, I don't like enforcing session mode that much)
To me one of key reasons for session mode (aside from access to disk images
in $HOME) is that it allows VMs to integrate with services / apps in that
users session, such as PA. So isolating them is going to eliminate that
key feture of session mode & break valid usage IMHO.
> 2) isolation, configs are fine, but any data an app wants to write, I'm
>really not sure how they handle multiple instances, so I approached
> this
>like non-shared filesystems, any 2 apps need to have their own view of
>the location and I also don't want them to even think of trying to
> write
>something to root's home
We don't really expect QEMU (or things it links to) to write to these
dirs, but if they did try SELinux would block it. IMHO if you wanted
stricter isolation then QEMU needs to be running a different UID.
> > Similary if using the GTK frontend these dirs are needed to load
> > the toolkit preferences such as theme.
>
> You mean for ? If so, we don't support GTK.
Yes for graphics. Lack of GTK support is a bug :-)
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 6/8] virQEMUCapsSetFromNodes: Skip setting deprecated flags
On 2/21/19 4:42 PM, Ján Tomko wrote:
Prepare for removal of deprecated flags from the capability
string array and quietly drop flags present in the deprecated
string array if they were not found in the main array.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b12f5914aa..ce50cfdfa5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3460,9 +3460,15 @@ virQEMUCapsSetFromNodes(virQEMUCapsPtr qemuCaps,
}
flag = virQEMUCapsTypeFromString(str);
if (flag < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown qemu capabilities flag %s"), str);
-goto cleanup;
+flag = virQEMUCapsDeprecatedTypeFromString(str);
As mentioned in 2/8 I think this should be replaced with
virStringListHasString().
+if (flag < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown qemu capabilities flag %s"), str);
+goto cleanup;
+} else {
+VIR_FREE(str);
+continue;
+}
}
VIR_FREE(str);
virQEMUCapsSet(qemuCaps, flag);
However, looking at the bigger picture is this a safe thing to do? I
mean, imagine the following scenario:
1) say there is capability X that affects certain part of cmd line. And
assume that those two possibilities are incompatible. If cmd line is
generated one way then migration to a qemu which has cmd line generated
the other way fails.
2) in release R we deprecate X and thus do not format it in
in status XML.
3) user starts a domain D.
4) user saves D into a file
5) sysadmin downgrades libvirt to R-1
6) user tries to restore D from the saved file
At this point, since X is not in the saved XML, R - 1 assumes that D is
lacking the capability and generates incompatible cmd line which makes
qemu fail on loading migration stream.
IOW, what one libvirt assumes the other might not.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] Split out virQEMUCapsSetFromNodes
On 2/21/19 4:42 PM, Ján Tomko wrote:
Add a function that will convert the "name" attributes from the passed
nodes to QEMU capabilities.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 51 +---
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7160860ab4..a355ee2e37 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3442,6 +3442,38 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps,
xmlXPathContextPtr ctxt)
}
+static int
+virQEMUCapsSetFromNodes(virQEMUCapsPtr qemuCaps,
+xmlNodePtr *nodes,
+size_t n)
+{
+size_t i;
+char *str = NULL;
VIR_AUTOFREE()
And if you declare the variable only in the loop body then ..
+int ret = -1;
+
+for (i = 0; i < n; i++) {
+int flag;
+if (!(str = virXMLPropString(nodes[i], "name"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing flag name in QEMU capabilities cache"));
+goto cleanup;
+}
+flag = virQEMUCapsTypeFromString(str);
+if (flag < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown qemu capabilities flag %s"), str);
+goto cleanup;
+}
+VIR_FREE(str);
.. this can be dropped too.
+virQEMUCapsSet(qemuCaps, flag);
+}
+ret = 0;
+ cleanup:
+VIR_FREE(str);
+return ret;
+}
+
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] Introduce virQEMUCapsDeprecated array
On 2/21/19 4:42 PM, Ján Tomko wrote: To prevent domains started with older libvirtd from disappearing, we need to be able to parse the string representation of QEMU capabilities, even if we no longer use them in the code. Put those in a separate array where we won't bother with enum constants. Signed-off-by: Ján Tomko --- src/qemu/qemu_capabilities.c | 95 src/qemu/qemu_capabilities.h | 1 + 2 files changed, 96 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b48bcbebee..7160860ab4 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -524,6 +524,101 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "scsi-disk.device_id", ); +#define VIR_QEMU_CAPS_DEPRECATED_LAST 92 /* chosen by fair dice roll */ Problem I have with this apporach is that as soon as we deprecate some other capability we have to change this number too. Since we don't need str<->enum translation can't we just have a static array of these strings and then in 6/8 use virStringListHasString() instead of virQEMUCapsDeprecatedTypeFromString()? +VIR_ENUM_IMPL(virQEMUCapsDeprecated, VIR_QEMU_CAPS_DEPRECATED_LAST, +"kqemu", /* X_QEMU_CAPS_KQEMU, Whether KQEMU is compiled in */ +"vnc-colon", /* X_QEMU_CAPS_VNC_COLON, VNC takes or address + display */ +"no-reboot", /* X_QEMU_CAPS_NO_REBOOT, Is the -no-reboot flag available */ +"drive", /* X_QEMU_CAPS_DRIVE, Is the new -drive arg available */ +"drive-boot", /* X_QEMU_CAPS_DRIVE_BOOT, Does -drive support boot=on */ +"name", /* X_QEMU_CAPS_NAME, Is the -name flag available */ +"uuid", /* X_QEMU_CAPS_UUID, Is the -uuid flag available */ +"domid", /* X_QEMU_CAPS_DOMID, Xenner: -domid flag available */ +"vnet-hdr", /* X_QEMU_CAPS_VNET_HDR */ +"migrate-kvm-stdio", /* X_QEMU_CAPS_MIGRATE_KVM_STDIO, avoid kvm tcp migration bug */ +"migrate-qemu-tcp", /* X_QEMU_CAPS_MIGRATE_QEMU_TCP, have qemu tcp migration */ +"migrate-qemu-exec", /* X_QEMU_CAPS_MIGRATE_QEMU_EXEC, have qemu exec migration */ +"drive-cache-v2", /* X_QEMU_CAPS_DRIVE_CACHE_V2, cache= flag wanting new v2 values */ +"drive-format", /* X_QEMU_CAPS_DRIVE_FORMAT, Is -drive format= avail */ +"vga", /* X_QEMU_CAPS_VGA, Is -vga avail */ +"0.10", /* X_QEMU_CAPS_0_10, features added in qemu-0.10.0 or later */ +"pci-device", /* X_QEMU_CAPS_PCIDEVICE, PCI device assignment supported */ +"mem-path", /* X_QEMU_CAPS_MEM_PATH, mmap'ped guest backing supported */ +"drive-serial", /* X_QEMU_CAPS_DRIVE_SERIAL, -driver serial= available */ +"xen-domid", /* X_QEMU_CAPS_XEN_DOMID, -xen-domid */ +"migrate-qemu-unix", /* X_QEMU_CAPS_MIGRATE_QEMU_UNIX, qemu migration via unix sockets */ +"chardev", /* X_QEMU_CAPS_CHARDEV, Is the new -chardev arg available */ +"enable-kvm", /* X_QEMU_CAPS_ENABLE_KVM, -enable-kvm flag */ +"monitor-json", /* X_QEMU_CAPS_MONITOR_JSON, JSON mode for monitor */ +"balloon", /* X_QEMU_CAPS_BALLOON, -balloon available */ +"device", /* X_QEMU_CAPS_DEVICE, Is the -device arg available */ +"sdl", /* X_QEMU_CAPS_SDL, Is the new -sdl arg available */ +"smp-topology", /* X_QEMU_CAPS_SMP_TOPOLOGY, -smp has sockets/cores/threads */ +"netdev", /* X_QEMU_CAPS_NETDEV, -netdev flag & netdev_add/remove */ +"rtc", /* X_QEMU_CAPS_RTC, The -rtc flag for clock options */ +"vhost-net", /* X_QEMU_CAPS_VHOST_NET, vhost-net support available */ +"rtc-td-hack", /* X_QEMU_CAPS_RTC_TD_HACK, -rtc-td-hack available */ +"no-kvm-pit", /* X_QEMU_CAPS_NO_KVM_PIT, -no-kvm-pit-reinjection supported */ +"tdf", /* X_QEMU_CAPS_TDF, -tdf flag (user-mode pit catchup) */ +"pci-configfd", /* X_QEMU_CAPS_PCI_CONFIGFD, pci-assign.configfd */ +"nodefconfig", /* X_QEMU_CAPS_NODEFCONFIG, -nodefconfig */ +"boot-menu", /* X_QEMU_CAPS_BOOT_MENU, -boot menu=on support */ +"enable-kqemu", /* X_QEMU_CAPS_ENABLE_KQEMU, -enable-kqemu flag */ +"fsdev", /* X_QEMU_CAPS_FSDEV, -fstype filesystem passthrough */ +"nesting", /* X_QEMU_CAPS_NESTING, -enable-nesting (SVM/VMX) */ +"name-process", /* X_QEMU_CAPS_NAME_PROCESS, Is -name process= available */ +"drive-readonly", /* X_QEMU_CAPS_DRIVE_READONLY, -drive readonly=on|off */ +"smbios-type", /* X_QEMU_CAPS_SMBIOS_TYPE, Is -smbios type= available */ +"vga-qxl", /* X_QEMU_CAPS_VGA_QXL, The 'qxl' arg for '-vga' */ +"vga-none", /* X_QEMU_CAPS_VGA_NONE, The 'none' arg for '-vga' */ +"migrate-qemu-fd", /* X_QEMU_CAPS_MIGRATE_QEMU_FD, -incoming fd:n */ +"boot-index", /* X_QEMU_CAPS_BOOTINDEX, -device bootindex property */ +"drive-aio", /* X_QEMU_CAPS_DRIVE_AIO, -drive aio= supported */ +"pci-multibus", /* X_QEMU_CAPS_PCI_MULTIBUS, bus=pci.0 vs bus=pci */ +"pci-bootindex", /* X_QEMU_CAPS_PCI_BOOTINDEX, pci-assign.bootindex */ +"chardev-spicevmc", /* X_QEMU_CAPS_CHARDEV_SPICEVMC, newer -chardev spicevmc */ +
[libvirt] [PATCH 7/7] util: Tweak virStringMatchesNameSuffix() some more
We can use STRNEQ() instead of STRNEQLEN() since we're only interested in the trailing part of the string. Signed-off-by: Andrea Bolognani --- src/util/virstring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index ba36562f85..f23daca0bd 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1300,7 +1300,7 @@ virStringMatchesNameSuffix(const char *file, if (STRNEQLEN(file, name, namelen)) return false; -if (STRNEQLEN(file + namelen, suffix, suffixlen)) +if (STRNEQ(file + namelen, suffix)) return false; return true; -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
