[libvirt] esx: What does "No storage volume with key or path" mean?

[Richard W.M. Jones]

We've had a report of a particular vCenter server (using VMware
clustering, we think) where virStorageVolInfo of a guest volume fails
with:

  Storage volume not found: No storage volume with key or path '[...] ...'

The error comes from esxVI_LookupFileInfoByDatastorePath:
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/esx/esx_vi.c;h=f7eeeb5c539914f1a0ba8d974b49e56ecf01e5fd;hb=HEAD#l3571

What does it mean?

The reporter also said they could do some (frankly voodoo) moving
guests around and then the error went away.  We're not sure if that
was just coincidence.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/3] Couple of patches to resolve recent Coverity issues

[John Ferlan]

Recent changes have tripped my Coverity checker.

John Ferlan (3):
  Use virGetLastErrorMessage to avoid Coverity message
  admin: Clean up error path in adminServerListClients
  conf: Fix error path in virNodeDevPCICapabilityParseXML

 daemon/admin_server.c | 3 +--
 src/conf/node_device_conf.c   | 5 -
 src/network/bridge_driver.c   | 3 +--
 src/node_device/node_device_hal.c | 3 +--
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message

[John Ferlan]

Both instances use VIR_WARN() to print the error from a failed
virDBusGetSystemBus() call.  Rather than use the virGetLastError
and need to check for valid return err pointer, just use the
virGetLastErrorMessage.

Signed-off-by: John Ferlan 
---
 src/network/bridge_driver.c   | 3 +--
 src/node_device/node_device_hal.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a34da2a..f406f04 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -695,9 +695,8 @@ networkStateInitialize(bool privileged,
 
 #ifdef HAVE_FIREWALLD
 if (!(sysbus = virDBusGetSystemBus())) {
-virErrorPtr err = virGetLastError();
 VIR_WARN("DBus not available, disabling firewalld support "
- "in bridge_network_driver: %s", err->message);
+ "in bridge_network_driver: %s", virGetLastErrorMessage());
 } else {
 /* add matches for
  * NameOwnerChanged on org.freedesktop.DBus for firewalld start/stop
diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index 6d18a87..6ddfad0 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -641,9 +641,8 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED,
 
 dbus_error_init();
 if (!(sysbus = virDBusGetSystemBus())) {
-virErrorPtr verr = virGetLastError();
 VIR_ERROR(_("DBus not available, disabling HAL driver: %s"),
-verr->message);
+virGetLastErrorMessage());
 ret = 0;
 goto failure;
 }
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] admin: Clean up error path in adminServerListClients

[John Ferlan]

Coverity noted that in adminServerListClients if virNetServerGetClients
returns a -1 into ret, then the call virObjectListFreeCount in cleanup
will not be very happy.

Adjust the code to skip the cleanup label and just return -1 if
virNetServerGetClients fails.

Signed-off-by: John Ferlan 
---
 daemon/admin_server.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 2fc4675..41f6e82 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -191,14 +191,13 @@ adminServerListClients(virNetServerPtr srv,
 virCheckFlags(0, -1);
 
 if ((ret = virNetServerGetClients(srv, )) < 0)
-goto cleanup;
+return -1;
 
 if (clients) {
 *clients = clts;
 clts = NULL;
 }
 
- cleanup:
 virObjectListFreeCount(clts, ret);
 return ret;
 }
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] conf: Fix error path in virNodeDevPCICapabilityParseXML

[John Ferlan]

If the call to virXPathNodeSet to set naddresses fails, Coverity notes
that the subsequent VIR_ALLOC_N cannot have a negative value (well it
probably wouldn't be negative per se).

Signed-off-by: John Ferlan 
---
 src/conf/node_device_conf.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index a06e450..aed95d5 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1305,7 +1305,10 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt,
 data->pci_dev.physical_function) < 0)
 goto out;
 } else if (STREQ(type, "virt_functions")) {
-int naddresses = virXPathNodeSet("./address", ctxt, );
+int naddresses;
+
+if ((naddresses = virXPathNodeSet("./address", ctxt, )) < 0)
+goto out;
 
 if (maxFuncsStr &&
 virStrToLong_uip(maxFuncsStr, NULL, 10,
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: set vlan tag for macvtap passthrough mode on SRIOV VFs

[John Ferlan]

On 05/05/2016 12:39 PM, Laine Stump wrote:
> SRIOV VFs used in macvtap passthrough mode can take advantage of the
> SRIOV card's transparent vlan tagging. All the code was there to set
> the vlan tag, and it has been used for SRIOV VFs used for hostdev
> interfaces for several years, but for some reason, the vlan tag for
> macvtap passthrough devices was stubbed out with a -1.
> 
> This patch moves a bit of common validation down to a lower level
> (virNetDevReplaceNetConfig()) so it is shared by hostdev and macvtap
> modes, and updates the macvtap caller to actually send the vlan config
> instead of -1.
> ---
> 
> While adding the info to the docs, I got a bit carried away fixing up
> the vlan-specific paragraphs. It someone really wants me to I can move
> it into a separate patch, but it seemed like more trouble than it was
> worth at the time...
> 
> 
>  docs/formatdomain.html.in   | 61 
> +++--
>  src/lxc/lxc_process.c   |  3 ++-
>  src/network/bridge_driver.c | 17 -
>  src/qemu/qemu_interface.c   |  1 +
>  src/util/virhostdev.c   | 23 ++---
>  src/util/virnetdev.c| 29 +
>  src/util/virnetdev.h|  6 +++--
>  src/util/virnetdevmacvlan.c |  4 ++-
>  src/util/virnetdevmacvlan.h |  2 ++
>  9 files changed, 86 insertions(+), 60 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 97794b7..68f0295 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4032,7 +4032,7 @@
>  
>On Linux systems, the bridge device is normally a standard Linux
>host bridge. On hosts that support Open vSwitch, it is also
> -  possible to connect to an open vSwitch bridge device by adding
> +  possible to connect to an Open vSwitch bridge device by adding
>a virtualport type='openvswitch'/ to the
>interface definition.  (Since
>0.9.11). The Open vSwitch type virtualport accepts two
> @@ -4816,34 +4816,47 @@ qemu-kvm -net nic,model=? /dev/null
>  
>  
>If (and only if) the network connection used by the guest
> -  supports vlan tagging transparent to the guest, an
> +  supports VLAN tagging transparent to the guest, an
>optional vlan element can specify one or
> -  more vlan tags to apply to the guest's network
> -  traffic Since 0.10.0. (openvswitch
  ^
[1] note: Formerly the "open" parenthesis...

> -  and type='hostdev' SR-IOV interfaces do support transparent vlan
> -  tagging of guest traffic; everything else, including standard
> +  more VLAN tags to apply to the guest's network
> +  traffic Since 0.10.0. Network
> +  connections that support guest-transparent VLAN tagging include
> +  1) type='bridge' interfaces connected to an Open vSwitch bridge
> +  Since 0.10.0, 2) SRIOV Virtual
> +  Functions (VF) used via type='hostdev' (direct device
> +  assignment) Since 0.10.0, and 3)
> +  SRIOV VFs used via type='direct' with mode='passthrough'
> +  (macvtap "passthru" mode) Since
> +  1.3.4; all other connection types, including standard

nit: "s/; all/. All/"   (already a long enough).

>linux bridges and libvirt's own virtual networks, do not
>support it. 802.1Qbh (vn-link) and 802.1Qbg (VEPA) switches
>provide their own way (outside of libvirt) to tag guest traffic
> -  onto specific vlans.) To allow for specification of multiple
> -  tags (in the case of vlan trunking), a
> -  subelement, tag, specifies which vlan tag
> -  to use (for example: tag id='42'/. If an
> -  interface has more than one vlan element
> -  defined, it is assumed that the user wants to do VLAN trunking
> -  using all the specified tags. In the case that vlan trunking
> -  with a single tag is desired, the optional
> +  onto specific vlans.) Each tag is given in a

Should this be VLANs?  or "onto a specific VLAN." ?

[1] Don't need the close parentheis ')' since the open one was removed.

> +  separate tag subelement
> +  of vlan (for example: tag
> +  id='42'/). For VLAN trunking of multiple tags (which
> +  is supported only on Open vSwitch connections),
> +  multiple tag subelements can be specified,

This reads strange... There's multiple multiples - one "multiple tags"
and one "multiple  subelements"... I guess it's right, just was a
tough read the first time through!

> +  which implies that the user wants to do VLAN trunking on the
> +  interface for all the specified tags. In the case that VLAN
> +  trunking of a single tag is desired, the optional
>attribute trunk='yes' can be added to the toplevel
> -  vlan element.
> -
> -
> -
> -  For network connections using openvswitch it is possible to
> -  configure the 'native-tagged' and 'native-untagged' vlan modes
> -  

[libvirt] [PATCH 4/9] target-i386: List CPU models using subclass list

[Eduardo Habkost]

Instead of using the builtin_x86_defs array, use the QOM subclass list
to list CPU models on "-cpu ?" and "query-cpu-definitions".

Signed-off-by: Andreas Färber 
[ehabkost: copied code from a patch by Andreas:
 "target-i386: QOM'ify CPU", from March 2012]
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |   4 ++
 target-i386/cpu.c | 111 +-
 2 files changed, 86 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index cb75017..c23b634 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -64,6 +64,10 @@ typedef struct X86CPUClass {
 
 bool kvm_required;
 
+/* Optional description of CPU model.
+ * If unavailable, cpu_def->model_id is used */
+const char *model_description;
+
 DeviceRealize parent_realize;
 void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 26a5a6b..037b602 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -676,6 +676,14 @@ static ObjectClass *x86_cpu_class_by_name(const char 
*cpu_model)
 return oc;
 }
 
+static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
+{
+const char *class_name = object_class_get_name(OBJECT_CLASS(cc));
+assert(g_str_has_suffix(class_name, X86_CPU_TYPE_SUFFIX));
+return g_strndup(class_name,
+ strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
+}
+
 struct X86CPUDefinition {
 const char *name;
 uint32_t level;
@@ -1484,6 +1492,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void 
*data)
 cpu_x86_fill_model_id(host_cpudef.model_id);
 
 xcc->cpu_def = _cpudef;
+xcc->model_description =
+"KVM processor with all supported host features "
+"(only available in KVM mode)";
 
 /* level, xlevel, xlevel2, and the feature words are initialized on
  * instance_init, because they require KVM to be initialized.
@@ -2009,23 +2020,62 @@ static void listflags(FILE *f, fprintf_function print, 
const char **featureset)
 }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *class_a = (ObjectClass *)a;
+ObjectClass *class_b = (ObjectClass *)b;
+X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+const char *name_a, *name_b;
+
+if (cc_a->kvm_required != cc_b->kvm_required) {
+/* kvm_required items go last */
+return cc_a->kvm_required ? 1 : -1;
+} else {
+name_a = object_class_get_name(class_a);
+name_b = object_class_get_name(class_b);
+return strcmp(name_a, name_b);
+}
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+list = g_slist_sort(list, x86_cpu_list_compare);
+return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CPUListState *s = user_data;
+char *name = x86_cpu_class_get_model_name(cc);
+const char *desc = cc->model_description;
+if (!desc) {
+desc = cc->cpu_def->model_id;
+}
+
+(*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+  name, desc);
+g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-X86CPUDefinition *def;
-char buf[256];
 int i;
+CPUListState s = {
+.file = f,
+.cpu_fprintf = cpu_fprintf,
+};
+GSList *list;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-def = _x86_defs[i];
-snprintf(buf, sizeof(buf), "%s", def->name);
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-}
-#ifdef CONFIG_KVM
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-   "KVM processor with all supported host features "
-   "(only available in KVM mode)");
-#endif
+(*cpu_fprintf)(f, "Available CPUs:\n");
+list = get_sorted_cpu_model_list();
+g_slist_foreach(list, x86_cpu_list_entry, );
+g_slist_free(list);
 
 (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2037,26 +2087,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 }
 }
 
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
-CpuDefinitionInfoList *cpu_list = NULL;
-X86CPUDefinition *def;
-int i;
-
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-CpuDefinitionInfoList *entry;
-CpuDefinitionInfo *info;
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CpuDefinitionInfoList 

[libvirt] [PATCH 9/9] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "runnable" and "unavailable-features" fields on the x86
implementation of query-cpu-definitions.

Example command output:

  {
  "return": [
  { "runnable": true, "name": "host"},
  { "runnable": true, "name": "qemu64"},
  { "runnable": true, "name": "qemu32"},
  { "runnable": false, "name": "phenom",
"unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", 
"fxsr-opt", "mmxext"] },
  { "runnable": true, "name": "pentium3"},
  { "runnable": true, "name": "pentium2"},
  { "runnable": true, "name": "pentium"},
  { "runnable": true, "name": "n270"},
  { "runnable": true, "name": "kvm64"},
  { "runnable": true, "name": "kvm32"},
  { "runnable": true, "name": "coreduo"},
  { "runnable": true, "name": "core2duo"},
  { "runnable": false, "name": "athlon",
"unavailable-features": ["3dnow", "3dnowext", "mmxext"] },
  { "runnable": true, "name": "Westmere"},
  { "runnable": true, "name": "SandyBridge"},
  { "runnable": true, "name": "Penryn"},
  { "runnable": false, "name": "Opteron_G5",
"unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"] },
  { "runnable": false, "name": "Opteron_G4",
"unavailable-features": ["fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"] },
  { "runnable": false, "name": "Opteron_G3",
"unavailable-features": ["misalignsse", "sse4a"] },
  { "runnable": true, "name": "Opteron_G2"},
  { "runnable": true, "name": "Opteron_G1"},
  { "runnable": true, "name": "Nehalem"},
  { "runnable": true, "name": "IvyBridge"},
  { "runnable": false, "name": "Haswell",
"unavailable-features": ["rtm", "hle"] },
  { "runnable": true, "name": "Haswell-noTSX"},
  { "runnable": true, "name": "Conroe"},
  { "runnable": false, "name": "Broadwell",
"unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed", 
"rtm", "hle"] },
  { "runnable": false, "name": "Broadwell-noTSX",
"unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed"] 
},
  { "runnable": true, "name": "486"}
  ]
  }

Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 40 
 1 file changed, 40 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7365d1..f6f0f83 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2041,6 +2041,41 @@ static void x86_cpu_report_filtered_features(X86CPU *cpu)
 }
 }
 
+static bool x86_cpu_class_is_runnable(X86CPUClass *xcc, strList 
**missing_feats)
+{
+X86CPU *xc;
+bool r;
+FeatureWord w;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+return false;
+}
+
+xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
+r = !x86_cpu_filter_features(xc);
+if (!r) {
+for (w = 0; w < FEATURE_WORDS; w++) {
+FeatureWordInfo *fi = _word_info[w];
+uint32_t filtered = xc->filtered_features[w];
+int i;
+for (i = 0; i < 32; i++) {
+if (filtered & (1UL << i)) {
+char **parts = g_strsplit(fi->feat_names[i], "|", 2);
+strList *new = g_new0(strList, 1);
+new->value = g_strdup(parts[0]);
+feat2prop(new->value);
+new->next = *missing_feats;
+*missing_feats = new;
+g_strfreev(parts);
+}
+}
+}
+}
+
+object_unref(OBJECT(xc));
+return r;
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2133,6 +2168,11 @@ static void x86_cpu_definition_entry(gpointer data, 
gpointer user_data)
 
 info = g_malloc0(sizeof(*info));
 info->name = x86_cpu_class_get_model_name(cc);
+info->has_runnable = true;
+info->runnable = x86_cpu_class_is_runnable(cc, 
>unavailable_features);
+if (!info->runnable) {
+info->has_unavailable_features = true;
+}
 
 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 6/9] target-i386: Define CPUID filtering functions before x86_cpu_list()

[Eduardo Habkost]

Just move code to another place so the it can be reused by the
query-cpu-definitions code.

Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 68 +++
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 465c125..309ef55 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2005,6 +2005,40 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char 
*features,
 }
 }
 
+/*
+ * Filters CPU feature words based on host availability of each feature.
+ *
+ * Returns: 0 if all flags are supported by the host, non-zero otherwise.
+ */
+static int x86_cpu_filter_features(X86CPU *cpu)
+{
+CPUX86State *env = >env;
+FeatureWord w;
+int rv = 0;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+uint32_t host_feat =
+x86_cpu_get_supported_feature_word(w, cpu->migratable);
+uint32_t requested_features = env->features[w];
+env->features[w] &= host_feat;
+cpu->filtered_features[w] = requested_features & ~env->features[w];
+if (cpu->filtered_features[w]) {
+rv = 1;
+}
+}
+
+return rv;
+}
+
+static void x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+FeatureWord w;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+report_unavailable_features(w, cpu->filtered_features[w]);
+}
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2134,40 +2168,6 @@ static uint32_t 
x86_cpu_get_supported_feature_word(FeatureWord w,
 return r;
 }
 
-/*
- * Filters CPU feature words based on host availability of each feature.
- *
- * Returns: 0 if all flags are supported by the host, non-zero otherwise.
- */
-static int x86_cpu_filter_features(X86CPU *cpu)
-{
-CPUX86State *env = >env;
-FeatureWord w;
-int rv = 0;
-
-for (w = 0; w < FEATURE_WORDS; w++) {
-uint32_t host_feat =
-x86_cpu_get_supported_feature_word(w, cpu->migratable);
-uint32_t requested_features = env->features[w];
-env->features[w] &= host_feat;
-cpu->filtered_features[w] = requested_features & ~env->features[w];
-if (cpu->filtered_features[w]) {
-rv = 1;
-}
-}
-
-return rv;
-}
-
-static void x86_cpu_report_filtered_features(X86CPU *cpu)
-{
-FeatureWord w;
-
-for (w = 0; w < FEATURE_WORDS; w++) {
-report_unavailable_features(w, cpu->filtered_features[w]);
-}
-}
-
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 {
 PropValue *pv;
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Eduardo Habkost]

This makes the feature name tables in feature_word_info all match
the actual QOM property names we use.

This will make the command-line interface more consistent,
allowing the QOM property names to be used as "-cpu" arguments
directly.

Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
compatibility with the old that had underscores.

Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 309ef55..e7365d1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -187,7 +187,7 @@ static const char *feature_name[] = {
 };
 static const char *ext_feature_name[] = {
 "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor",
-"ds_cpl", "vmx", "smx", "est",
+"ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
 NULL, "pcid", "dca", "sse4.1|sse4_1",
@@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
 "nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
+NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
 NULL, "lm|i64", "3dnowext", "3dnow",
 };
 static const char *ext3_feature_name[] = {
-"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
ExtApicSpace */,
+"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
ExtApicSpace */,
 "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
 "3dnowprefetch", "osvw", "ibs", "xop",
 "skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid_msr",
-NULL, "tbm", "topoext", "perfctr_core",
-"perfctr_nb", NULL, NULL, NULL,
+"fma4", "tce", NULL, "nodeid-msr",
+NULL, "tbm", "topoext", "perfctr-core",
+"perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 };
 
@@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
 };
 
 static const char *kvm_feature_name[] = {
-"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
 };
 
 static const char *svm_feature_name[] = {
-"npt", "lbrv", "svm_lock", "nrip_save",
-"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-NULL, NULL, "pause_filter", NULL,
+"npt", "lbrv", "svm-lock", "nrip-save",
+"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+NULL, NULL, "pause-filter", NULL,
 "pfthreshold", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
 };
 
 static const char *cpuid_7_0_ebx_feature_name[] = {
-"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
+"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
 "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
 "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
 "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
@@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
 .set   = x86_set_hv_spinlocks,
 };
 
-/* Convert all '_' in a feature string option name to '-', to make feature
- * name conform to QOM property naming rule, which uses '-' instead of '_'.
+/* Convert all '_' in a feature string option name to '-', to keep 
compatibility
+ * with old feature names that used "_" instead of "-".
  */
 static inline void feat2prop(char *s)
 {
@@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char 
*features,
 while (featurestr) {
 char *val;
 if (featurestr[0] == '+') {
+feat2prop(featurestr);
 add_flagname_to_bitmaps(featurestr + 1, plus_features, _err);
 } else if (featurestr[0] == '-') {
+feat2prop(featurestr);
 add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);
 } else if ((val = strchr(featurestr, '='))) {
 *val = 0; val++;
@@ -3137,11 +3139,9 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
 
 names = g_strsplit(fi->feat_names[bitnr], "|", 0);
 
-feat2prop(names[0]);
 x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
 
 for (i = 1; names[i]; i++) {
-feat2prop(names[i]);
 object_property_add_alias(obj, names[i], obj, names[0],
 

[libvirt] [PATCH 1/9] target-i386: Move TCG initialization check to tcg_x86_init()

[Eduardo Habkost]

Instead of requiring cpu.c to check if TCG was already initialized,
simply let the function be called multiple times.

Suggested-by: Igor Mammedov 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c   | 4 +---
 target-i386/translate.c | 6 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4856cd4..a689fec 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3087,7 +3087,6 @@ static void x86_cpu_initfn(Object *obj)
 X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
 CPUX86State *env = >env;
 FeatureWord w;
-static int inited;
 
 cs->env_ptr = env;
 cpu_exec_init(cs, _abort);
@@ -3138,8 +3137,7 @@ static void x86_cpu_initfn(Object *obj)
 x86_cpu_load_def(cpu, xcc->cpu_def, _abort);
 
 /* init various static tables used in TCG mode */
-if (tcg_enabled() && !inited) {
-inited = 1;
+if (tcg_enabled()) {
 tcg_x86_init();
 }
 }
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 1a1214d..92570b4 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8133,6 +8133,12 @@ void tcg_x86_init(void)
 "bnd0_ub", "bnd1_ub", "bnd2_ub", "bnd3_ub"
 };
 int i;
+static bool initialized = false;
+
+if (initialized) {
+return;
+}
+initialized = true;
 
 cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
 cpu_cc_op = tcg_global_mem_new_i32(cpu_env,
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/9] target-i386: Move TCG initialization to realize time

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

Move TCG initialization to realize time so it won't be called when just
doing object_new() on a X86CPU subclass.

Signed-off-by: Eduardo Habkost 
---
Changes v1 -> v2:
 * Now the inited/tcg_initialized variable doesn't exist anymore
 * Move tcg_x86_init() call after basic parameter validation inside
   realizefn
---
 target-i386/cpu.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a689fec..bde649a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,6 +2901,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 
+if (tcg_enabled()) {
+tcg_x86_init();
+}
+
 #ifndef CONFIG_USER_ONLY
 qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
@@ -3135,11 +3139,6 @@ static void x86_cpu_initfn(Object *obj)
 }
 
 x86_cpu_load_def(cpu, xcc->cpu_def, _abort);
-
-/* init various static tables used in TCG mode */
-if (tcg_enabled()) {
-tcg_x86_init();
-}
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/9] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

x86_cpu_filter_features() will be reused by code that shouldn't
print any warning. Move the warning code to a new
x86_cpu_report_filtered_features() function, and call it from
x86_cpu_realizefn().

Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 037b602..465c125 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2152,9 +2152,6 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 env->features[w] &= host_feat;
 cpu->filtered_features[w] = requested_features & ~env->features[w];
 if (cpu->filtered_features[w]) {
-if (cpu->check_cpuid || cpu->enforce_cpuid) {
-report_unavailable_features(w, cpu->filtered_features[w]);
-}
 rv = 1;
 }
 }
@@ -2162,6 +2159,15 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 return rv;
 }
 
+static void x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+FeatureWord w;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+report_unavailable_features(w, cpu->filtered_features[w]);
+}
+}
+
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 {
 PropValue *pv;
@@ -2936,12 +2942,16 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 env->cpuid_level = 7;
 }
 
-if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-error_setg(_err,
-   kvm_enabled() ?
-   "Host doesn't support requested features" :
-   "TCG doesn't support requested features");
-goto out;
+if (x86_cpu_filter_features(cpu) &&
+(cpu->check_cpuid || cpu->enforce_cpuid)) {
+x86_cpu_report_filtered_features(cpu);
+if (cpu->enforce_cpuid) {
+error_setg(_err,
+   kvm_enabled() ?
+   "Host doesn't support requested features" :
+   "TCG doesn't support requested features");
+goto out;
+}
 }
 
 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Extend query-cpu-definitions schema to allow it to return two new
optional fields: "runnable" and "unavailable-features".
"runnable" will tell if the CPU model can be run in the current
host. "unavailable-features" will contain a list of CPU
properties that are preventing the CPU model from running in the
current host.

Cc: David Hildenbrand 
Cc: Michael Mueller 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost 
---
 qapi-schema.json | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 54634c4..450e6e7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2948,11 +2948,19 @@
 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
+# @runnable: true if the CPU model is runnable using the current
+#machine and accelerator. Optional. Since 2.6.
+# @unavailable-features: List of properties that prevent the CPU
+#model from running in the current host,
+#if @runnable is false. Optional.
+#Since 2.6.
 #
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str',
+'*runnable': 'bool',
+'*unavailable-features': [ 'str' ] } }
 
 ##
 # @query-cpu-definitions:
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/9] target-i386: Call cpu_exec_init() on realize

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

Calling cpu_exec_init() also affects QEMU's ability to handle errors
during CPU creation, as some actions done by cpu_exec_init() can't be
reverted.

Move cpu_exec_init() call to realize so a simple object_new() won't
trigger it, and so that it is called after some basic validation of CPU
parameters.

Signed-off-by: Eduardo Habkost 
---
Changes v1 -> v2:
 * Call cpu_exec_init() after basic parameter validation
---
 target-i386/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bde649a..26a5a6b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,6 +2901,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 
+cpu_exec_init(cs, _abort);
+
 if (tcg_enabled()) {
 tcg_x86_init();
 }
@@ -3093,7 +3095,6 @@ static void x86_cpu_initfn(Object *obj)
 FeatureWord w;
 
 cs->env_ptr = env;
-cpu_exec_init(cs, _abort);
 
 object_property_add(obj, "family", "int",
 x86_cpuid_version_get_family,
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/9] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

This series extends query-cpu-definitions to include two extra
fields: "runnable", and "unavailable-features".

This will return information based on the current machine and
accelerator only. In the future we may extend these mechanisms to
allow querying other machines and other accelerators without
restarting QEMU, but it will require some reorganization of
QEMU's main code.

This series is based on my 'x86-next' branch, at:
  git://github.com/ehabkost/qemu.git x86-next

Cc: David Hildenbrand 
Cc: Michael Mueller 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Jiri Denemark 
Cc: libvir-list@redhat.com

Eduardo Habkost (9):
  target-i386: Move TCG initialization check to tcg_x86_init()
  target-i386: Move TCG initialization to realize time
  target-i386: Call cpu_exec_init() on realize
  target-i386: List CPU models using subclass list
  target-i386: Move warning code outside x86_cpu_filter_features()
  target-i386: Define CPUID filtering functions before x86_cpu_list()
  qmp: Add runnability information to query-cpu-definitions
  target-i386: Use "-" instead of "_" on all feature names
  target-i386: Return runnability information on query-cpu-definitions

 qapi-schema.json|  10 +-
 target-i386/cpu-qom.h   |   4 +
 target-i386/cpu.c   | 275 +---
 target-i386/translate.c |   6 ++
 4 files changed, 207 insertions(+), 88 deletions(-)

-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/4] qemu_monitor_json: Refactor even more

[John Ferlan]

On 05/04/2016 08:33 AM, Michal Privoznik wrote:
> All these patches will be squashed into one, but I've split them
> into multiple because of easier review.
> 
> Michal Privoznik (4):
>   qemu_monitor_json: Follow refactor
>   qemu_monitor_json: Follow refactor
>   qemu_monitor_json: Follow refactor
>   qemu_monitor_json: Follow refactor
> 
>  src/qemu/qemu_monitor_json.c | 673 
> ++-
>  1 file changed, 349 insertions(+), 324 deletions(-)
> 

Not a deal breaker, but most code has:

if (qemuMonitorJSONCheckError(cmd, reply) < 0)
goto cleanup;

ret = 0;
 cleanup:

while a few instances [1] have:

 ret = qemuMonitorJSONCheckError(cmd, reply);

  cleanup:

IDC either way since the result is the same, but I suppose they "could"
be all done the same way.  I'd probably lean towards the latter, but
since you already went with the former in commit '7884d089' it's
probably best to keep using that.

[1]
qemuMonitorJSONSetMigrationCompression
qemuMonitorJSONBlockCommit
qemuMonitorJSONInjectNMI  [2]
qemuMonitorJSONSendKey  [2]
qemuMonitorJSONSetMigrationCapability

[2]
both instances use the same HMP call attempt to return ret - they could
follow the qemuMonitorJSONSetCPU model...


Finally, I see the comments move 4 spaces to the right in
qemuMonitorJSONSetObjectProperty?   (patch 4)...  Was that a remnant or
expected?


ACK series (your call if you want to adjust [2] now or in a followup
patch. it's trivial enough as long as you remember to also remove the {}
around what would become "one line" if then else statements).


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/17] genericxml2xml: add several graphics tests

[Pavel Hrdina]

On Fri, May 06, 2016 at 02:33:03PM +0200, Peter Krempa wrote:
> On Thu, May 05, 2016 at 18:20:25 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> 
> [...]
> 
> > diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> > index 70a5203..70ecd2d 100644
> > --- a/tests/genericxml2xmltest.c
> > +++ b/tests/genericxml2xmltest.c
> > @@ -77,9 +77,16 @@ mymain(void)
> >  
> >  DO_TEST_DIFFERENT("disk-virtio");
> >  
> > +DO_TEST_DIFFERENT("graphics-vnc-minimal");
> > +DO_TEST_DIFFERENT("graphics-vnc-manual-port");
> > +DO_TEST_DIFFERENT("graphics-vnc-socket");
> > +DO_TEST_DIFFERENT("graphics-vnc-socket-listen");
> >  DO_TEST_DIFFERENT("graphics-listen-back-compat");
> >  DO_TEST_FULL("graphics-listen-back-compat-mismatch", 0, false,
> >  TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> > +DO_TEST_DIFFERENT("graphics-vnc-listen-attr-only");
> > +DO_TEST_DIFFERENT("graphics-vnc-listen-element-minimal");
> > +DO_TEST_DIFFERENT("graphics-vnc-listen-element-with-address");
> 
> I guess the test case names are self explanatory enough to require a
> commit message describing the tests.
> 
> ACK

Thanks, I've pushed patches 1-6 except patch 4.

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: domain: Don't treat unknown storage type as not having backing chain

[John Ferlan]

On 05/06/2016 09:48 AM, Peter Krempa wrote:
> qemuDomainCheckDiskPresence has short-circuit code to skip the
> determination of the disk backing chain for storage formats that can't
> have backing volumes. The code treats VIR_STORAGE_FILE_NONE as not
> having backing chain and skips the call to qemuDomainDetermineDiskChain.
> 
> This is wrong as qemuDomainDetermineDiskChain is responsible for storage
> format detection and has logic to determine the default type if format
> detection is disabled.
> 
> This allows to storage passed via  to circumvent the
> enforcement to have correct storage format or that we shall default to
> format='raw', since we don't set the default type via the post parse
> callback for "volume" backed disks as the translation code could come up
> with a better guess.
> 
> This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1328003
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK...

John

Do you believe it still would be worthwhile to add:

if (virDomainDiskGetFormat(def) == VIR_STORAGE_FILE_NONE)
virDomainDiskSetFormat(def, VIR_STORAGE_FILE_RAW);

in virStorageTranslateDiskSourcePool in the ISCSI case for MODE_HOST?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 8/8] Revert "conf: Validate disk lun using correct types"

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> This reverts commit c79ebf53b5fe0a33bf407b3bcb49e3a27ec97eb4.
> 
> We can't just add checks to the XML parser once we've accepted such
> configuration in the past.
> ---
>  src/conf/domain_conf.c   | 22 --
>  tests/qemuxml2argvtest.c |  3 +--
>  2 files changed, 1 insertion(+), 24 deletions(-)
> 

There was a bz associated with that commit - that'll need to be
addressed in some manner...

While I understand your point here, the configuration didn't work - that
is it couldn't be started anyway so there could not be a domain running
with that configuration and thus it wouldn't disappear on a subsequent
reload, hence why checking the config and rejecting "earlier" seemed
proper even though we hadn't rejected such a config when the
"mode='host'" was first implemented.

Commit 'c79ebf53b' is a followup of sorts to commit '33188c9f'...

For me it's a NACK, but someone else may feel differently

John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 48a220f..c4cbbff 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4181,28 +4181,6 @@ 
> virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>  }
>  }
> 
> -/* Validate LUN configuration
> - * NOTE: virStorageTranslateDiskSourcePool is not run yet, so for
> - *   disk "volume"'s, the closest we can get at config time is
> - *   to ensure mode isn't direct since host/default will allow
> - *   lun/block usage. At run time if it's determined the wrong
> - *   voltype and pooltype values are set, then failure occurs
> - */
> -if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN &&
> -!(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
> -  (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> -   disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) ||
> -  (disk->src->type == VIR_STORAGE_TYPE_VOLUME &&
> -   disk->src->srcpool &&
> -   disk->src->srcpool->mode !=
> -   VIR_STORAGE_SOURCE_POOL_MODE_DIRECT))) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("disk '%s' improperly configured for a "
> - "device='lun'"),
> -   disk->dst);
> -return -1;
> -}
> -
>  if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>  virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0)
>  return -1;
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 8842b2f..4211e82 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -780,8 +780,7 @@ mymain(void)
>  DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE);
>  DO_TEST("disk-drive-no-boot",
>  QEMU_CAPS_BOOTINDEX);
> -DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",
> -QEMU_CAPS_VIRTIO_SCSI);
> +DO_TEST_FAILURE("disk-device-lun-type-invalid", QEMU_CAPS_VIRTIO_SCSI);
>  DO_TEST_FAILURE("disk-usb-nosupport", NONE);
>  DO_TEST("disk-usb-device",
>  QEMU_CAPS_DEVICE_USB_STORAGE,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/8] qemu: Reject invalid block copy targets for

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> Extract the relevant parts of the existing checker and reuse them for
> blockcopy since copying to a non-block device creates an invalid
> configuration.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1209802
> ---
>  src/qemu/qemu_command.c | 14 +-
>  src/qemu/qemu_domain.c  | 31 +++
>  src/qemu/qemu_domain.h  |  3 +++
>  src/qemu/qemu_driver.c  |  4 
>  4 files changed, 39 insertions(+), 13 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/8] qemu: command: Remove unnecessary label in qemuCheckDiskConfig

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_command.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/8] conf: Kill now unused virDomainDiskSourceIsBlockType

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> ---
>  src/conf/domain_conf.c   | 52 
> 
>  src/conf/domain_conf.h   |  3 ---
>  src/libvirt_private.syms |  1 -
>  3 files changed, 56 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/8] qemu: command: Use more appropriate checking function for block devices

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> In qemuCheckDiskConfig would now use virDomainDiskSourceIsBlockType just
> as a glorified version of virStorageSourceIsBlockLocal that reports
> error messages. Replace it with the latter including the message for
> clarity.
> ---
>  src/qemu/qemu_command.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Hmmm... I wonder if the previous commit should remove or at least
reference the check in virDomainDiskSourceIsBlockType that neglected to
consider the translation and actualtype...

ACK for this - your call on whether to adjust the previous commit...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/8] qemu: Support for iSCSI direct mapped volumes

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> Commit c820fbff9fbfe1f2549a5b60967496587f8d8bfc added support for iSCSI
> disk as backing for . We would not use it for a disk
> type="volume" with direct access mode which basically maps to direct
> iSCSI usage. Fix it by adding the storage source type accessor that
> resolves the volume type.
> ---
>  src/qemu/qemu_command.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/8] lxc: Fix wrong error message on disk hotplug

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> Commit 36025c552 tried to improve error reporting for 
> but reused the code in LXC which doesn't care about the actual disk
> type. The error messages would then contain a bogous hint that the
> config for the 'lun' device is invalid which might not be the case.
> 
> Re-do the relevant portion of the commit with the original message.
> ---
>  src/lxc/lxc_driver.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 1dfbde3..7b76daf 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -4106,9 +4106,6 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
>  goto cleanup;
>  }
> 
> -if (!virDomainDiskSourceIsBlockType(def->src, true))
> -goto cleanup;
> -
>  src = virDomainDiskGetSource(def);
>  if (src == NULL) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -4116,6 +4113,12 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
>  goto cleanup;
>  }
> 
> +if (virStorageSourceIsBlockLocal(def->src)) {

Shouldn't this be "if (!virStorageSourceIsBlockLocal(def->src)" ?

ACK w/ the adjustment

John

> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Can't setup disk for non-block device"));
> +goto cleanup;
> +}
> +

FWIW:
Prior to my commit referenced above the check was:

if (!virDomainDiskSourceIsBlockType(def->src)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
   _("Can't setup disk for non-block device"));

And yes, of course since virDomainDiskSourceIsBlockType checks
"src->path" first (and provides a different message), moving this after
that check seems right; otherwise, the error message is "source path not
found for device='lun' using type='%d'" if (!src->path).

All that commit did was try to have the lower layer generate the error
message. Of course in this case erroneously swapping "Can't setup disk
for non-block device" for "disk device='lun' is only valid for block
type disk source".

Still going back to 'a7785ccf' we see the call to "if
(!virDomainDiskSourceIsBlockType(def)"...


>  if (virDomainDiskIndexByName(vm->def, def->dst, true) >= 0) {
>  virReportError(VIR_ERR_OPERATION_FAILED,
> _("target %s already exists"), def->dst);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/8] util: Replace virDomainDiskSourceIsBlockType with a new helper

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> For disks sources described by a libvirt volume we don't need to do a
> complicated check since virStorageTranslateDiskSourcePool already
> correctly determines the actual disk type.
> 
> Replace the checks using a new accessor that does not open-code the
> whole logic.
> ---
>  src/libvirt_private.syms  |  1 +
>  src/lxc/lxc_cgroup.c  |  3 ++-
>  src/qemu/qemu_conf.c  | 10 +++---
>  src/util/virstoragefile.c | 16 +++-
>  src/util/virstoragefile.h |  3 ++-
>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5030ec3..73c9fdb 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2277,6 +2277,7 @@ virStorageSourceFree;
>  virStorageSourceGetActualType;
>  virStorageSourceGetSecurityLabelDef;
>  virStorageSourceInitChainElement;
> +virStorageSourceIsBlockLocal;
>  virStorageSourceIsEmpty;
>  virStorageSourceIsLocalStorage;
>  virStorageSourceNewFromBacking;
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 4afe7e2..ea86d36 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -393,7 +393,8 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
> 
>  VIR_DEBUG("Allowing any disk block devs");
>  for (i = 0; i < def->ndisks; i++) {
> -if (!virDomainDiskSourceIsBlockType(def->disks[i]->src, false))
> +if (virStorageSourceIsEmpty(def->disks[i]->src) ||
> +!virStorageSourceIsBlockLocal(def->disks[i]->src))

Could an LXC domain use a storage pool for a volume? If so, then
somewhere in the LXC code wouldn't we need to translate the source pool
in order to get "actualtype" and the volume check...


>  continue;
> 
>  if (virCgroupAllowDevicePath(cgroup,
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 5ed5776..e00ddca 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1243,7 +1243,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
>  char *key = NULL;
>  int ret = -1;
> 
> -if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk->src, 
> false))
> +if (virStorageSourceIsEmpty(disk->src) ||
> +!disk->src->shared ||
> +!virStorageSourceIsBlockLocal(disk->src))
>  return 0;
> 
>  qemuDriverLock(driver);
> @@ -1388,7 +1390,9 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
>  char *key = NULL;
>  int ret = -1;
> 
> -if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk->src, 
> false))
> +if (virStorageSourceIsEmpty(disk->src) ||
> +!disk->src->shared ||
> +!virStorageSourceIsBlockLocal(disk->src))
>  return 0;
> 
>  qemuDriverLock(driver);
> @@ -1476,7 +1480,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
>  disk = dev->data.disk;
> 
>  if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN ||
> -!virDomainDiskSourceIsBlockType(disk->src, false))
> +!virStorageSourceIsBlockLocal(disk->src))
>  return 0;
> 
>  path = virDomainDiskGetSource(disk);
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 05ac254..4f44e05 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1955,7 +1955,7 @@ virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr 
> def)
> 
> 
>  int
> -virStorageSourceGetActualType(virStorageSourcePtr def)
> +virStorageSourceGetActualType(const virStorageSource *def)
>  {
>  if (def->type == VIR_STORAGE_TYPE_VOLUME && def->srcpool)
>  return def->srcpool->actualtype;
> @@ -2012,6 +2012,20 @@ virStorageSourceIsEmpty(virStorageSourcePtr src)
> 
> 
>  /**
> + * virStorageSourceIsBlockLocal:
> + * @src: disk source definition
> + *
> + * Returns true if @src describes a locally accessible block storage source.
> + * This includes block devices and host-mapped iSCSI volumes.
> + */
> +bool
> +virStorageSourceIsBlockLocal(const virStorageSource *src)
> +{
> +return virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK;

This assumes that virStorageTranslateDiskSourcePool has been run, which
is true is for the non-LXC paths changed in this patch...

So as long as you're comfortable with the LXC adjustment, then

ACK -

John

> +}
> +
> +
> +/**
>   * virStorageSourceBackingStoreClear:
>   *
>   * @src: disk source to clear
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b98fe25..17e1277 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -357,9 +357,10 @@ int virStorageSourceInitChainElement(virStorageSourcePtr 
> newelem,
>   bool force);
>  void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def);
>  void virStorageSourceClear(virStorageSourcePtr def);
> -int virStorageSourceGetActualType(virStorageSourcePtr def);
> +int virStorageSourceGetActualType(const virStorageSource *def);
>  bool 

[libvirt] [PATCH] qemu: domain: Don't treat unknown storage type as not having backing chain

[Peter Krempa]

qemuDomainCheckDiskPresence has short-circuit code to skip the
determination of the disk backing chain for storage formats that can't
have backing volumes. The code treats VIR_STORAGE_FILE_NONE as not
having backing chain and skips the call to qemuDomainDetermineDiskChain.

This is wrong as qemuDomainDetermineDiskChain is responsible for storage
format detection and has logic to determine the default type if format
detection is disabled.

This allows to storage passed via  to circumvent the
enforcement to have correct storage format or that we shall default to
format='raw', since we don't set the default type via the post parse
callback for "volume" backed disks as the translation code could come up
with a better guess.

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1328003
---
 src/qemu/qemu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cae356c..78a717a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3817,7 +3817,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
  * without backing support, the fact that the file exists is
  * more than enough */
 if (virStorageSourceIsLocalStorage(disk->src) &&
-format >= VIR_STORAGE_FILE_NONE &&
+format > VIR_STORAGE_FILE_NONE &&
 format < VIR_STORAGE_FILE_BACKING &&
 virFileExists(virDomainDiskGetSource(disk)))
 continue;
-- 
2.8.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/17] genericxml2xml: add several graphics tests

[Cole Robinson]

On 05/06/2016 09:21 AM, Pavel Hrdina wrote:
> On Fri, May 06, 2016 at 08:43:25AM -0400, Cole Robinson wrote:
>> On 05/05/2016 12:20 PM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  .../generic-graphics-vnc-listen-attr-only.xml  | 28 
>>> 
>>>  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
>>> ++
>>>  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
>>> ++
>>>  .../generic-graphics-vnc-manual-port.xml   | 28 
>>> 
>>>  .../generic-graphics-vnc-minimal.xml   | 28 
>>> 
>>>  .../generic-graphics-vnc-socket-listen.xml | 30 
>>> ++
>>>  .../generic-graphics-vnc-socket.xml| 28 
>>> 
>>>  .../generic-graphics-vnc-listen-attr-only.xml  | 30 
>>> ++
>>>  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
>>> ++
>>>  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
>>> ++
>>>  .../generic-graphics-vnc-manual-port.xml   | 28 
>>> 
>>>  .../generic-graphics-vnc-minimal.xml   | 28 
>>> 
>>>  .../generic-graphics-vnc-socket-listen.xml | 30 
>>> ++
>>>  .../generic-graphics-vnc-socket.xml| 28 
>>> 
>>>  tests/genericxml2xmltest.c |  7 +
>>
>> The generic XML parser doesn't have any restriction on duplicate > type='vnc'/>, so these could be combined to possibly one XML file.
>>
>> I know there really isn't much precedent for that at the moment, but given 
>> the
>> massive size of our test suite and the fact that much of the time is spent on
>> redundant parsing and XML formatting, personally I'd like to see things move
>> in that direction.
> 
> The idea itself isn't bad, but I think that it's better to have vnc and spice 
> in
> separate XML to make it clear which test failed so you know that it's vnc
> graphics or spice graphics directly only from the test name.
> 

Yes, agreed with splitting on VNC vs spice at least.

> I've tried this command on my desktop:
> 
> "time ./run tests/.libs/qemuxml2xmltest"
> 
> and considering that there are 837 test cases the output of time is:
> 
> real0m0.821s
> user0m0.746s
> sys 0m0.034s
> 
> So we don't spent that much time while parsing and formatting XML.
> 

Fair enough, but truly the big time sink is domainschematest...

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/17] genericxml2xml: add several graphics tests

[Pavel Hrdina]

On Fri, May 06, 2016 at 08:43:25AM -0400, Cole Robinson wrote:
> On 05/05/2016 12:20 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  .../generic-graphics-vnc-listen-attr-only.xml  | 28 
> > 
> >  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
> > ++
> >  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
> > ++
> >  .../generic-graphics-vnc-manual-port.xml   | 28 
> > 
> >  .../generic-graphics-vnc-minimal.xml   | 28 
> > 
> >  .../generic-graphics-vnc-socket-listen.xml | 30 
> > ++
> >  .../generic-graphics-vnc-socket.xml| 28 
> > 
> >  .../generic-graphics-vnc-listen-attr-only.xml  | 30 
> > ++
> >  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
> > ++
> >  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
> > ++
> >  .../generic-graphics-vnc-manual-port.xml   | 28 
> > 
> >  .../generic-graphics-vnc-minimal.xml   | 28 
> > 
> >  .../generic-graphics-vnc-socket-listen.xml | 30 
> > ++
> >  .../generic-graphics-vnc-socket.xml| 28 
> > 
> >  tests/genericxml2xmltest.c |  7 +
> 
> The generic XML parser doesn't have any restriction on duplicate  type='vnc'/>, so these could be combined to possibly one XML file.
> 
> I know there really isn't much precedent for that at the moment, but given the
> massive size of our test suite and the fact that much of the time is spent on
> redundant parsing and XML formatting, personally I'd like to see things move
> in that direction.

The idea itself isn't bad, but I think that it's better to have vnc and spice in
separate XML to make it clear which test failed so you know that it's vnc
graphics or spice graphics directly only from the test name.

I've tried this command on my desktop:

"time ./run tests/.libs/qemuxml2xmltest"

and considering that there are 837 test cases the output of time is:

real0m0.821s
user0m0.746s
sys 0m0.034s

So we don't spent that much time while parsing and formatting XML.

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/17] qemu_hotplug: cleanup qemuDomainChangeGraphics

[Pavel Hrdina]

On Fri, May 06, 2016 at 02:30:35PM +0200, Peter Krempa wrote:
> On Thu, May 05, 2016 at 18:20:24 +0200, Pavel Hrdina wrote:
> 
> In subject: This mostly improves error messages, so cleanup is not
> really a spot-on description of this patch.

Not exactly, the error messages are the same, but to make the subject better
I'll change it to "cleanup error messages in qemuDomainChangeGraphics".

Thanks

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/17] qemu_process: move listen code out of qemuProcessSetupGraphics

[Peter Krempa]

On Fri, May 06, 2016 at 14:52:02 +0200, Pavel Hrdina wrote:
> On Fri, May 06, 2016 at 02:10:10PM +0200, Peter Krempa wrote:
> > On Thu, May 05, 2016 at 18:20:21 +0200, Pavel Hrdina wrote:
> > > Move adding the config listen type=address if there is none in
> > > qemuProcessPrepareDomain and move check for multiple listens to
> > > qemuProcessStartValidate.
> > > 
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  src/qemu/qemu_process.c | 75 
> > > -
> > >  1 file changed, 55 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > > index 348b392..17fd566 100644
> > > --- a/src/qemu/qemu_process.c
> > > +++ b/src/qemu/qemu_process.c
> > 
> > > @@ -5138,6 +5143,35 @@ qemuProcessPrepareDomain(virConnectPtr conn,
> > >  if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
> > >  goto cleanup;
> > >  
> > > +/* Fill in run-time values for graphics devices. */
> > > +for (i = 0; i < vm->def->ngraphics; i++) {
> > > +virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
> > > +char *listenAddr = NULL;
> > > +
> > > +switch (graphics->type) {
> > > +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> > > +listenAddr = cfg->vncListen;
> > > +case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> > > +if (!listenAddr)
> > 
> > This fallthrough case with checking whether this was set is weird.
> > Looking at the caps code it looks like its guaranteed that both
> > vncListen and spiceListen are always allocated, but still it looks like
> > both types should either have individual calls to
> > virDomainGraphicsListenAppendAddress or the call should happen after the
> > switch so that we don't obscure it using the fallthrough case.
> 
> Right, it can be easily updated that the listenAddr is set only for vnc or 
> spice
> and the virDomainGraphicsListenAppendAddress could be moved out of the switch
> with this condition: if (graphics->nListens == 0 && listeAddr).  Do you 
> require
> v2 for this?

Not really. This is what I'd do.

ACK using that approach


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/17] qemu_process: move listen code out of qemuProcessSetupGraphics

[Pavel Hrdina]

On Fri, May 06, 2016 at 02:10:10PM +0200, Peter Krempa wrote:
> On Thu, May 05, 2016 at 18:20:21 +0200, Pavel Hrdina wrote:
> > Move adding the config listen type=address if there is none in
> > qemuProcessPrepareDomain and move check for multiple listens to
> > qemuProcessStartValidate.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_process.c | 75 
> > -
> >  1 file changed, 55 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 348b392..17fd566 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> 
> > @@ -5138,6 +5143,35 @@ qemuProcessPrepareDomain(virConnectPtr conn,
> >  if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
> >  goto cleanup;
> >  
> > +/* Fill in run-time values for graphics devices. */
> > +for (i = 0; i < vm->def->ngraphics; i++) {
> > +virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
> > +char *listenAddr = NULL;
> > +
> > +switch (graphics->type) {
> > +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> > +listenAddr = cfg->vncListen;
> > +case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> > +if (!listenAddr)
> 
> This fallthrough case with checking whether this was set is weird.
> Looking at the caps code it looks like its guaranteed that both
> vncListen and spiceListen are always allocated, but still it looks like
> both types should either have individual calls to
> virDomainGraphicsListenAppendAddress or the call should happen after the
> switch so that we don't obscure it using the fallthrough case.

Right, it can be easily updated that the listenAddr is set only for vnc or spice
and the virDomainGraphicsListenAppendAddress could be moved out of the switch
with this condition: if (graphics->nListens == 0 && listeAddr).  Do you require
v2 for this?

> 
> > +listenAddr = cfg->spiceListen;
> > +
> > +if (graphics->nListens == 0) {
> > +if (virDomainGraphicsListenAppendAddress(graphics,
> > + listenAddr) < 0)
> > +goto cleanup;
> > +
> > +graphics->listens[0].fromConfig = true;
> > +}
> > +break;
> > +
> > +case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> > +case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> > +case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> > +case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> > +break;
> > +}
> > +}
> > +
> >  /* "volume" type disk's source must be translated before
> >   * cgroup and security setting.
> >   */


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 4/4] qemu: command: Use -name guest= if available

[Cole Robinson]

On 05/06/2016 07:54 AM, John Ferlan wrote:
> 
> 
> On 05/04/2016 10:56 AM, Cole Robinson wrote:
>> -name guest= is the explicit parameter for passing a VM name. Using
>> it is required to allow a VM with an '=' in the name
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1276485
>> ---
>>  src/qemu/qemu_capabilities.c | 2 ++
>>  src/qemu/qemu_capabilities.h | 1 +
>>  src/qemu/qemu_command.c  | 4 
>>  tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 +
>>  tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 +
>>  tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 +
>>  tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 +
>>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 8 
>>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml  | 2 +-
>>  tests/qemuxml2argvtest.c | 2 +-
>>  10 files changed, 17 insertions(+), 6 deletions(-)
>>
> 
> You're overloading the name-escape test with the "guest=" option which
> I'm never quite sure how others feel about. I would think you'd want a
> separate test that just adds the "guest=".
> 

That's generally the pattern, to add a new XML file for every qemu addition,
but that's overkill IMO

> I also see this biting us at some point in the future in some test when
> 'guest=' becomes the default and all the test outputs have to change to
> add that.
> 

That's not really any different than the unconditional -name debug-threads=on
option we add for qemu 2.5 and later, or secrets initializing, etc

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/17] genericxml2xml: add several graphics tests

[Cole Robinson]

On 05/05/2016 12:20 PM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  .../generic-graphics-vnc-listen-attr-only.xml  | 28 
>  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
> ++
>  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
> ++
>  .../generic-graphics-vnc-manual-port.xml   | 28 
>  .../generic-graphics-vnc-minimal.xml   | 28 
>  .../generic-graphics-vnc-socket-listen.xml | 30 
> ++
>  .../generic-graphics-vnc-socket.xml| 28 
>  .../generic-graphics-vnc-listen-attr-only.xml  | 30 
> ++
>  ...generic-graphics-vnc-listen-element-minimal.xml | 30 
> ++
>  ...ic-graphics-vnc-listen-element-with-address.xml | 30 
> ++
>  .../generic-graphics-vnc-manual-port.xml   | 28 
>  .../generic-graphics-vnc-minimal.xml   | 28 
>  .../generic-graphics-vnc-socket-listen.xml | 30 
> ++
>  .../generic-graphics-vnc-socket.xml| 28 
>  tests/genericxml2xmltest.c |  7 +

The generic XML parser doesn't have any restriction on duplicate , so these could be combined to possibly one XML file.

I know there really isn't much precedent for that at the moment, but given the
massive size of our test suite and the fact that much of the time is spent on
redundant parsing and XML formatting, personally I'd like to see things move
in that direction.

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] domain_conf: fix migration/managedsave with usb keyboad

[Peter Krempa]

On Fri, May 06, 2016 at 14:19:39 +0200, Pavel Hrdina wrote:
> Commint 36785c7e refactored the code for input devices but introduced a

commit

> bug that we removed all keyboard from migratable XML.  We have to remove

bug where

> only implicit keyboards like PS2 or XEN.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/domain_conf.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

ACK and I think this might be worth backporting to the appropriate maint
branches if they were created.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/17] genericxml2xml: add several graphics tests

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:25 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---

[...]

> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index 70a5203..70ecd2d 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -77,9 +77,16 @@ mymain(void)
>  
>  DO_TEST_DIFFERENT("disk-virtio");
>  
> +DO_TEST_DIFFERENT("graphics-vnc-minimal");
> +DO_TEST_DIFFERENT("graphics-vnc-manual-port");
> +DO_TEST_DIFFERENT("graphics-vnc-socket");
> +DO_TEST_DIFFERENT("graphics-vnc-socket-listen");
>  DO_TEST_DIFFERENT("graphics-listen-back-compat");
>  DO_TEST_FULL("graphics-listen-back-compat-mismatch", 0, false,
>  TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
> +DO_TEST_DIFFERENT("graphics-vnc-listen-attr-only");
> +DO_TEST_DIFFERENT("graphics-vnc-listen-element-minimal");
> +DO_TEST_DIFFERENT("graphics-vnc-listen-element-with-address");

I guess the test case names are self explanatory enough to require a
commit message describing the tests.

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/17] qemu_hotplug: cleanup qemuDomainChangeGraphics

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:24 +0200, Pavel Hrdina wrote:

In subject: This mostly improves error messages, so cleanup is not
really a spot-on description of this patch.

> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_hotplug.c | 33 +
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 03e5309..97f4152 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c

> @@ -2618,8 +2619,9 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
>  }
>  
>  if (dev->nListens != olddev->nListens) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   _("cannot change the number of listen addresses"));
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("cannot change the number of listen addresses "
> + "on '%s' graphics"), type);

I'm not quite sure whether the 'on' preposition is correct in this
context, but I don't have a better suggestion.

>  goto cleanup;
>  }
>  
> @@ -2628,30 +2630,30 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
>  virDomainGraphicsListenDefPtr oldlisten = >listens[i];
>  
>  if (newlisten->type != oldlisten->type) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   _("cannot change the type of listen address"));
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("cannot change the type of listen address "
> + "on '%s' graphics"), type);
>  goto cleanup;
>  }
>  
>  switch ((virDomainGraphicsListenType) newlisten->type) {
>  case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
>  if (STRNEQ_NULLABLE(newlisten->address, oldlisten->address)) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   dev->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ?
> -   _("cannot change listen address setting on 
> vnc graphics") :
> -   _("cannot change listen address setting on 
> spice graphics"));
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("cannot change listen address setting "
> + "on '%s' graphics"), type);
>  goto cleanup;
>  }
> -break;
>  
> +break;
>  case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:

I prefer the empty line before the next 'case' statement.

>  if (STRNEQ_NULLABLE(newlisten->network, oldlisten->network)) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   dev->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ?
> -   _("cannot change listen network setting on vnc 
> graphics") :
> -   _("cannot change listen network setting on spice 
> graphics"));
... ah so it was pre-existing. I don't care about it then :)
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("cannot change listen address setting "
> + "on '%s' graphics"), type);
>  goto cleanup;
>  }
> +
>  break;
>  
>  case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/17] graphics: generate fake ports also for tests

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:23 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c  | 12 
>  src/qemu/qemu_process.c | 13 
> +
>  tests/qemuxml2argvdata/qemuxml2argv-controller-order.args   |  2 +-
>  tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args |  2 +-
>  .../qemuxml2argv-video-device-pciaddr-default.args  |  2 +-
>  5 files changed, 16 insertions(+), 15 deletions(-)

While I'm okay by this change itself I don't really see a reason why 
qemuProcessSPICEAllocatePorts and qemuProcessVNCAllocatePorts should be
called in two distinct places. With the new helpers I think we have the
facility to select the @alloc parameter according to the
VIR_QEMU_PROCESS_START_PRETEND flag and call it just once.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] domain_conf: fix migration/managedsave with usb keyboad

[Pavel Hrdina]

Commint 36785c7e refactored the code for input devices but introduced a
bug that we removed all keyboard from migratable XML.  We have to remove
only implicit keyboards like PS2 or XEN.

Signed-off-by: Pavel Hrdina 
---
 src/conf/domain_conf.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c97bc1..b90c2b6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21193,8 +21193,10 @@ virDomainInputDefFormat(virBufferPtr buf,
 const char *bus = virDomainInputBusTypeToString(def->bus);
 
 /* don't format keyboard into migratable XML for backward compatibility */
-if (def->type == VIR_DOMAIN_INPUT_TYPE_KBD &&
-flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE)
+if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
+def->type == VIR_DOMAIN_INPUT_TYPE_KBD &&
+(def->bus == VIR_DOMAIN_INPUT_BUS_PS2 ||
+ def->bus == VIR_DOMAIN_INPUT_BUS_XEN))
 return 0;
 
 if (!type) {
-- 
2.8.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/17] qemu_process: handle port allocation for VNC the same way as for Spice

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:22 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c  |  6 +++---
>  src/qemu/qemu_process.c | 29 -
>  src/qemu/qemu_process.h |  3 +++
>  3 files changed, 30 insertions(+), 8 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/4] qemu: handle ',' and '=' in VM name

[Cole Robinson]

On 05/06/2016 07:54 AM, John Ferlan wrote:
> 
> 
> On 05/04/2016 10:56 AM, Cole Robinson wrote:
>> This series adds qemu cli comma escaping to several places that
>> are dependent on the VM name, to enable names with embedded commas.
>>
>> Patch 4 makes use of qemu -name guest=X value to allow names with
>> '=' in them.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=639926
>> https://bugzilla.redhat.com/show_bug.cgi?id=1276485
>>
>> Note: There's likely other places that are VM name dependent that need
>> comma escaping too, but this hits the mandatory ones. I've listed some
>> more on the BiteSizedTasks page:
>>
>> http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values
>>
>> v2:
>> Rebase to master
>>
>> Cole Robinson (4):
>>   qemu: command: escape commas in VM name
>>   qemu: command: escape commas in secret master path
>>   qemu: command: escape commas in chardev socket path
>>   qemu: command: Use -name guest= if available
>>
>>  src/qemu/qemu_capabilities.c   |  2 ++
>>  src/qemu/qemu_capabilities.h   |  1 +
>>  src/qemu/qemu_command.c| 22 
>>  tests/qemucapabilitiesdata/caps_2.1.1-1.caps   |  1 +
>>  tests/qemucapabilitiesdata/caps_2.4.0-1.caps   |  1 +
>>  tests/qemucapabilitiesdata/caps_2.5.0-1.caps   |  1 +
>>  tests/qemucapabilitiesdata/caps_2.6.0-1.caps   |  1 +
>>  .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 
>> ++
>>  .../qemuxml2argvdata/qemuxml2argv-name-escape.xml  | 18 
>>  tests/qemuxml2argvtest.c   |  2 ++
>>  10 files changed, 65 insertions(+), 8 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml
>>
> 
> ewww again ;-)
> 
> While I cannot imagine a reason for someone (other than testing perhaps)
> to want a "," or an "=" in a vm name, I suppose since it's possible to
> handle by qemu, then we should 'handle' it.
> 
> Sorry I dropped the ball after my initial pass - I guess I was wondering
> if there were any other (similar) feelings!
> 

No worries

> After reading the bz, I see why you've gone with double commas, although
> it wasn't apparent without that read...  So maybe each commit that adds
> that double comma escape could explain why the double comma works (or
> even at points in the code where it occurs?).
> 

Yeah the double comma is the qemu convention. I'll add an explicit function
like qemuBufEscapeCommas or something to make this a bit more obvious

> Searching qemu_command.c for domainChannelTargetDir and it's seems
> things are covered (good)... Searching for domainLibDir and I think
> there's one usage not covered - qemuBuildGraphicsVNCCommandLine uses it
> to build "$path_with_domainLibDir/vnc.sock".
> 

Okay, I'll add that.

> Of course finding these "one off" type issues makes me think at some
> point in the future one of those will be used for "something" and the
> double comma will be missed.  So should we generate a local "escaped"
> names for domainLibDir and domainChannelTargetDir that would include the
> double comma and that would be passed to the corresponding users?
> 

TBH I don't really think this is important enough to try and come up with a
safe generalized solution at this point. I think it will largely work itself
out if I add the explicit escaping function, and there's enough usages in
qemu_command.c that they will get cargoculted around when new code is added.

> I think as long as you cover the VNC case and add a few more words why
> the ",," can be used on the command line, then the series seems
> reasonable to me, although you may want to wait a bit to push to see if
> anyone else has angst over the two changes, especially now having
> "guest=$name" be the default for anyone using qemu 2.1 or later.
> 

I'll send a v3 anyways so we can see if anyone cares

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 3/4] qemu: command: escape commas in chardev socket path

[Cole Robinson]

On 05/06/2016 07:54 AM, John Ferlan wrote:
> 
> 
> On 05/04/2016 10:56 AM, Cole Robinson wrote:
>> After this, a default virt-manager VM will startup with a comma
>> in the VM name:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=639926
>> ---
>>  src/qemu/qemu_command.c  | 9 -
>>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +-
>>  2 files changed, 5 insertions(+), 6 deletions(-)
>>
> 
> FWIW:
> w/r/t my comment in your v1 about vmagent... It's the "guest_agent" I
> was thinking about.  In any case, I see it uses the chardev path
> generation code, so I think that's covered by your patch 3.
> 
> 
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index d54d0d1..c2f55b5 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4833,11 +4833,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>>  break;
>>  
>>  case VIR_DOMAIN_CHR_TYPE_UNIX:
>> -virBufferAsprintf(,
>> -  "socket,id=char%s,path=%s%s",
>> -  alias,
>> -  dev->data.nix.path,
>> -  dev->data.nix.listen ? ",server,nowait" : "");
>> +virBufferAsprintf(, "socket,id=char%s,path=", alias);
>> +virBufferEscape(, ',', ",", "%s", dev->data.nix.path);
> 
> Unless you know/see that the data.nix.path is built using
> domainChannelTargetDir during qemuBuildChannelsCommandLine it may not be
> "intuitively obvious" why doing the escape here is necessary...
> 

It's not specific to the autocreated socket path based on
domainChannelTargetDir, the user could have passed in a manual path with a
comma in it

The rule is that any time a user specified string is passed to the qemu
command line, we should be escaping commas.  happens to trickle out into
several other places, but doing this for the unix path is still beneficial
regardless of the  focus of this series

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/17] qemu_process: move listen code out of qemuProcessSetupGraphics

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:21 +0200, Pavel Hrdina wrote:
> Move adding the config listen type=address if there is none in
> qemuProcessPrepareDomain and move check for multiple listens to
> qemuProcessStartValidate.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_process.c | 75 
> -
>  1 file changed, 55 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 348b392..17fd566 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

> @@ -5138,6 +5143,35 @@ qemuProcessPrepareDomain(virConnectPtr conn,
>  if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
>  goto cleanup;
>  
> +/* Fill in run-time values for graphics devices. */
> +for (i = 0; i < vm->def->ngraphics; i++) {
> +virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
> +char *listenAddr = NULL;
> +
> +switch (graphics->type) {
> +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> +listenAddr = cfg->vncListen;
> +case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> +if (!listenAddr)

This fallthrough case with checking whether this was set is weird.
Looking at the caps code it looks like its guaranteed that both
vncListen and spiceListen are always allocated, but still it looks like
both types should either have individual calls to
virDomainGraphicsListenAppendAddress or the call should happen after the
switch so that we don't obscure it using the fallthrough case.

> +listenAddr = cfg->spiceListen;
> +
> +if (graphics->nListens == 0) {
> +if (virDomainGraphicsListenAppendAddress(graphics,
> + listenAddr) < 0)
> +goto cleanup;
> +
> +graphics->listens[0].fromConfig = true;
> +}
> +break;
> +
> +case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> +case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> +case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> +case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> +break;
> +}
> +}
> +
>  /* "volume" type disk's source must be translated before
>   * cgroup and security setting.
>   */


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virCgroupValidateMachineGroup: Reflect change in CGroup struct naming

[Cole Robinson]

On 05/05/2016 11:56 AM, Michal Privoznik wrote:
> Fron c3bd0019c0e on instead of creating the following path for
> cgroups:
> 
>   /sys/fs/cgroupX/$name.libvirt-$driver
> 
> we generate rather more verbose one:
> 
>   /sys/fs/cgroupX/$driver-$id-$name.libvirt-$driver
> 
> where $name is optional and included iff contains allowed chars.
> See original commit for more reasoning. Now, problem with the
> original commit is that we are unable to start any LXC domain
> after it. Because when starting LXC container, the CGroup layout
> is created by our lxc_controller process and then detected and
> validated by libvirtd. The validation is done by trying to match
> detected layout against all the possible patterns for cgroup
> paths that we've ever had. And the commit in question forgot to
> update this part of the code.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> I am really surprised how long this bug went unnoticed. Is
> anybody using LXC? When pushed, it should be backported to
> 1.3.2+.

FWIW libvirt.git without this patch (and with f24 libvirt 1.3.2 package) my
containers start fine, and as part of the upstream bug cleanup I did a handful
of lxc testing. So I don't think the failure is quite as unconditional as you
suggest

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 3/4] qemu: command: escape commas in chardev socket path

[John Ferlan]

On 05/04/2016 10:56 AM, Cole Robinson wrote:
> After this, a default virt-manager VM will startup with a comma
> in the VM name:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=639926
> ---
>  src/qemu/qemu_command.c  | 9 -
>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +-
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 

FWIW:
w/r/t my comment in your v1 about vmagent... It's the "guest_agent" I
was thinking about.  In any case, I see it uses the chardev path
generation code, so I think that's covered by your patch 3.


> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d54d0d1..c2f55b5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4833,11 +4833,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> -virBufferAsprintf(,
> -  "socket,id=char%s,path=%s%s",
> -  alias,
> -  dev->data.nix.path,
> -  dev->data.nix.listen ? ",server,nowait" : "");
> +virBufferAsprintf(, "socket,id=char%s,path=", alias);
> +virBufferEscape(, ',', ",", "%s", dev->data.nix.path);

Unless you know/see that the data.nix.path is built using
domainChannelTargetDir during qemuBuildChannelsCommandLine it may not be
"intuitively obvious" why doing the escape here is necessary...

John
> +if (dev->data.nix.listen)
> +virBufferAddLit(, ",server,nowait");
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
> index a5e35b8..772d94f 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
> @@ -15,7 +15,7 @@ bar/master-key.aes \
>  -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>  -nographic \
>  -nodefaults \
> --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,bar/monitor.sock,\
> +-chardev 
> socket,id=charmonitor,path=/tmp/lib/domain--1-foo,,bar/monitor.sock,\
>  server,nowait \
>  -mon chardev=charmonitor,id=monitor,mode=readline \
>  -no-acpi \
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/4] qemu: handle ',' and '=' in VM name

[John Ferlan]

On 05/04/2016 10:56 AM, Cole Robinson wrote:
> This series adds qemu cli comma escaping to several places that
> are dependent on the VM name, to enable names with embedded commas.
> 
> Patch 4 makes use of qemu -name guest=X value to allow names with
> '=' in them.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=639926
> https://bugzilla.redhat.com/show_bug.cgi?id=1276485
> 
> Note: There's likely other places that are VM name dependent that need
> comma escaping too, but this hits the mandatory ones. I've listed some
> more on the BiteSizedTasks page:
> 
> http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values
> 
> v2:
> Rebase to master
> 
> Cole Robinson (4):
>   qemu: command: escape commas in VM name
>   qemu: command: escape commas in secret master path
>   qemu: command: escape commas in chardev socket path
>   qemu: command: Use -name guest= if available
> 
>  src/qemu/qemu_capabilities.c   |  2 ++
>  src/qemu/qemu_capabilities.h   |  1 +
>  src/qemu/qemu_command.c| 22 
>  tests/qemucapabilitiesdata/caps_2.1.1-1.caps   |  1 +
>  tests/qemucapabilitiesdata/caps_2.4.0-1.caps   |  1 +
>  tests/qemucapabilitiesdata/caps_2.5.0-1.caps   |  1 +
>  tests/qemucapabilitiesdata/caps_2.6.0-1.caps   |  1 +
>  .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 
> ++
>  .../qemuxml2argvdata/qemuxml2argv-name-escape.xml  | 18 
>  tests/qemuxml2argvtest.c   |  2 ++
>  10 files changed, 65 insertions(+), 8 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml
> 

ewww again ;-)

While I cannot imagine a reason for someone (other than testing perhaps)
to want a "," or an "=" in a vm name, I suppose since it's possible to
handle by qemu, then we should 'handle' it.

Sorry I dropped the ball after my initial pass - I guess I was wondering
if there were any other (similar) feelings!

After reading the bz, I see why you've gone with double commas, although
it wasn't apparent without that read...  So maybe each commit that adds
that double comma escape could explain why the double comma works (or
even at points in the code where it occurs?).

Searching qemu_command.c for domainChannelTargetDir and it's seems
things are covered (good)... Searching for domainLibDir and I think
there's one usage not covered - qemuBuildGraphicsVNCCommandLine uses it
to build "$path_with_domainLibDir/vnc.sock".

Of course finding these "one off" type issues makes me think at some
point in the future one of those will be used for "something" and the
double comma will be missed.  So should we generate a local "escaped"
names for domainLibDir and domainChannelTargetDir that would include the
double comma and that would be passed to the corresponding users?

I think as long as you cover the VNC case and add a few more words why
the ",," can be used on the command line, then the series seems
reasonable to me, although you may want to wait a bit to push to see if
anyone else has angst over the two changes, especially now having
"guest=$name" be the default for anyone using qemu 2.1 or later.


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 4/4] qemu: command: Use -name guest= if available

[John Ferlan]

On 05/04/2016 10:56 AM, Cole Robinson wrote:
> -name guest= is the explicit parameter for passing a VM name. Using
> it is required to allow a VM with an '=' in the name
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1276485
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  src/qemu/qemu_command.c  | 4 
>  tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 +
>  tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 +
>  tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 +
>  tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 +
>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 8 
>  tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml  | 2 +-
>  tests/qemuxml2argvtest.c | 2 +-
>  10 files changed, 17 insertions(+), 6 deletions(-)
> 

You're overloading the name-escape test with the "guest=" option which
I'm never quite sure how others feel about. I would think you'd want a
separate test that just adds the "guest=".

I also see this biting us at some point in the future in some test when
'guest=' becomes the default and all the test outputs have to change to
add that.

I'm sure it seems you're chasing a moving target with changes to the
*.caps files... Recent changes in that space will cause more adjustments
here .

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 65c3d69..da47759 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -328,6 +328,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"device-tray-moved-event",
>"nec-usb-xhci-ports",
>"virtio-scsi-pci.iothread",
> +  "name-guest",
>  );
>  
>  
> @@ -2666,6 +2667,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>  { "spice", "gl", QEMU_CAPS_SPICE_GL },
>  { "chardev", "logfile", QEMU_CAPS_CHARDEV_LOGFILE },
>  { "name", "debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS },
> +{ "name", "guest", QEMU_CAPS_NAME_GUEST },
>  };
>  
>  static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index e7d0a60..9145f2d 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -359,6 +359,7 @@ typedef enum {
>  QEMU_CAPS_DEVICE_TRAY_MOVED, /* DEVICE_TRAY_MOVED event */
>  QEMU_CAPS_NEC_USB_XHCI_PORTS, /* -device nec-usb-xhci.p3 ports setting */
>  QEMU_CAPS_VIRTIO_SCSI_IOTHREAD, /* virtio-scsi-{pci,ccw}.iothread */
> +QEMU_CAPS_NAME_GUEST, /* -name guest= */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c2f55b5..570566c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6790,6 +6790,10 @@ qemuBuildNameCommandLine(virCommandPtr cmd,
>  
>  virCommandAddArg(cmd, "-name");
>  
> +/* The 'guest' option let's us handle a name with '=' embedded in it */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_GUEST))
> +virBufferAddLit(, "guest=");
> +

So, any domain that has qemu 2.1 or later will use the "-name
guest=$name..." naming scheme rather than just "-name $name..."
(regardless of whether there's a '=' in $name or not).

I don't necessarily mind using the guest=$name syntax; however, it could
be quite a surprise to anything that searches on/for "-name $name"
rather than "-name guest=$name" in/for the running qemu

John
>  virBufferEscape(, ',', ",", "%s", def->name);
>  
>  if (cfg->setProcessName &&
> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps 
> b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps
> index 729000f..29214bb 100644
> --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps
> +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps
> @@ -164,4 +164,5 @@
>  
>  
>  
> +
>
> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps 
> b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps
> index 8009b8f..7b1de29 100644
> --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps
> +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps
> @@ -177,4 +177,5 @@
>  
>  
>  
> +
>
> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps 
> b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps
> index e139455..19192da 100644
> --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps
> +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps
> @@ -178,4 +178,5 @@
>  
>  
>  
> +
>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps 
> b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps
> index ec5bfcc..59f637f 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps
> @@ -183,4 +183,5 @@
>  
>  
>  
> +
>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args 

Re: [libvirt] [PATCH v5 0/2] qemu: Support for QXL heads

[Daniel P. Berrange]

On Fri, May 06, 2016 at 01:27:15PM +0200, Pavel Hrdina wrote:
> On Thu, May 05, 2016 at 07:18:51PM -0400, John Ferlan wrote: 
> 
> [...]
> 
> > Been sitting on list for a while 
> > 
> > Obviously I think you know you have to update to top of tree
> > 
> > Would be nice to perhaps add a few intro comments to
> > qemuMigratePrepareDomain at least with respect to what the purpose is
> > and what "could" be done in the future in the code.  Looks like to me
> > it's now a shim to qemuProcessPrepareDomain to take care of any of those
> > "inconsistencies" between what can be supported in/on the new system
> > (perhaps could work in the opposite direction too ;-))... When I'm
> > reading code, I'm not necessarily looking at the commit message that
> > added which may have that information.
> > 
> > I think you could also update the commit message to point at the
> > previous code that was reverted to help understand the history.
> > 
> > ACK for the concept - looks like things are OK to me.
> > 
> > John
> 
> The only issue with this patch is that it breaks migration back to older
> libvirt.  We generally try to not break migration to old libvirt if you 
> migrate
> from new libvirt to old libvirt with the same XML that would be also valid for
> the old libvirt.  Since there is no change in the XML and we start using the
> 'heads' attribute and we now pass that value to qemu you cannot migrate back 
> to
> some older libvirt.
> 
> NACK, we need to figure this out.

Yep, the major use of QXL is RHEV/oVirt and they explicitly require libvirt
to support migration to old versions. So we need to figure this out.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 0/2] qemu: Support for QXL heads

[Pavel Hrdina]

On Thu, May 05, 2016 at 07:18:51PM -0400, John Ferlan wrote: 

[...]

> Been sitting on list for a while 
> 
> Obviously I think you know you have to update to top of tree
> 
> Would be nice to perhaps add a few intro comments to
> qemuMigratePrepareDomain at least with respect to what the purpose is
> and what "could" be done in the future in the code.  Looks like to me
> it's now a shim to qemuProcessPrepareDomain to take care of any of those
> "inconsistencies" between what can be supported in/on the new system
> (perhaps could work in the opposite direction too ;-))... When I'm
> reading code, I'm not necessarily looking at the commit message that
> added which may have that information.
> 
> I think you could also update the commit message to point at the
> previous code that was reverted to help understand the history.
> 
> ACK for the concept - looks like things are OK to me.
> 
> John

The only issue with this patch is that it breaks migration back to older
libvirt.  We generally try to not break migration to old libvirt if you migrate
from new libvirt to old libvirt with the same XML that would be also valid for
the old libvirt.  Since there is no change in the XML and we start using the
'heads' attribute and we now pass that value to qemu you cannot migrate back to
some older libvirt.

NACK, we need to figure this out.

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[Jiri Denemark]

On Fri, May 06, 2016 at 12:29:34 +0200, Peter Krempa wrote:
> On Fri, May 06, 2016 at 12:23:04 +0200, Jiri Denemark wrote:
> > On Fri, May 06, 2016 at 10:02:12 +0200, Peter Krempa wrote:
> > > On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
> > > > Signed-off-by: Jiri Denemark 
> > > 
> > > Please explain the difference between caps_2.6.0-1.aarch64.replies
> > > caps_2.6.0-2.aarch64.replies. At least the difference how they were
> > > obtained.
> > 
> > Well, I asked Andrea to provide me some data from aarch64 and ppc64le
> > hosts :-) The difference is in GIC version supported on a two different
> > hosts.
> 
> Well I kind of implied that you should put it into the commit message
> for further reference. I don't really care at this point about the
> difference but somebody might care in the future when touching the
> tests.

Yeah, sorry about that. It wasn't obvious to me that you wanted me to
update the commit message. Try to be explicit next time :-)

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[Jiri Denemark]

On Fri, May 06, 2016 at 06:39:15 -0400, John Ferlan wrote:
> 
> 
> On 05/06/2016 06:29 AM, Peter Krempa wrote:
> > On Fri, May 06, 2016 at 12:23:04 +0200, Jiri Denemark wrote:
> >> On Fri, May 06, 2016 at 10:02:12 +0200, Peter Krempa wrote:
> >>> On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
>  Signed-off-by: Jiri Denemark 
> >>>
> >>> Please explain the difference between caps_2.6.0-1.aarch64.replies
> >>> caps_2.6.0-2.aarch64.replies. At least the difference how they were
> >>> obtained.
> >>
> >> Well, I asked Andrea to provide me some data from aarch64 and ppc64le
> >> hosts :-) The difference is in GIC version supported on a two different
> >> hosts.
> > 
> > Well I kind of implied that you should put it into the commit message
> > for further reference. I don't really care at this point about the
> > difference but somebody might care in the future when touching the
> > tests.
> > 
> > 
> 
> FWIW: For example, is there a process or script (or something) somewhere
> that you use in order to generate the files?

Yes, "tests/qemucapsprobe /path/to/binary >caps.replies". It's mainly
useful for adding data files. It can be used for updating existing ones
in case you still have the binary and can just re-probe it. If you want
to add staff to existing files manually, qemucapsprobe won't help you.
But, Peter is planning to enhance the fake monitor and *.replies data
format so that you don't have to care about ids or order (to some
extent) in which you add the new replies.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cpu: add ABM to Haswell* and Broadwell* CPU models

[Jiri Denemark]

On Fri, May 06, 2016 at 17:53:04 +0800, Eli Qiao wrote:
> Corresponding QEMU commits:
> becb66673ec30cb604926d247ab9449a60ad8b11
> 
> Signed-off-by: Eli Qiao 
> ---
>  src/cpu/cpu_map.xml | 4 
>  1 file changed, 4 insertions(+)

NACK. We don't add features to models since it could break existing
setups. In any case, presence of any flag in CPU models does not really
influence whether that flag is going to be provided to a guest. That
part is driven by QEMU.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[John Ferlan]

On 05/06/2016 06:29 AM, Peter Krempa wrote:
> On Fri, May 06, 2016 at 12:23:04 +0200, Jiri Denemark wrote:
>> On Fri, May 06, 2016 at 10:02:12 +0200, Peter Krempa wrote:
>>> On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
 Signed-off-by: Jiri Denemark 
>>>
>>> Please explain the difference between caps_2.6.0-1.aarch64.replies
>>> caps_2.6.0-2.aarch64.replies. At least the difference how they were
>>> obtained.
>>
>> Well, I asked Andrea to provide me some data from aarch64 and ppc64le
>> hosts :-) The difference is in GIC version supported on a two different
>> hosts.
> 
> Well I kind of implied that you should put it into the commit message
> for further reference. I don't really care at this point about the
> difference but somebody might care in the future when touching the
> tests.
> 
> 

FWIW: For example, is there a process or script (or something) somewhere
that you use in order to generate the files?

I know when I recently touched these to add something for
virtio-scsi-{pci,ccw} - it took me a bit to recall "how" to get the
information necessary.  Then as Cole pointed out during review -
inserting a new "id" really doesn't require one to adjust all the
following id's, but since it was done before.

John

(I know - patches welcome, right? ;-))

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[Peter Krempa]

On Fri, May 06, 2016 at 12:23:04 +0200, Jiri Denemark wrote:
> On Fri, May 06, 2016 at 10:02:12 +0200, Peter Krempa wrote:
> > On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
> > > Signed-off-by: Jiri Denemark 
> > 
> > Please explain the difference between caps_2.6.0-1.aarch64.replies
> > caps_2.6.0-2.aarch64.replies. At least the difference how they were
> > obtained.
> 
> Well, I asked Andrea to provide me some data from aarch64 and ppc64le
> hosts :-) The difference is in GIC version supported on a two different
> hosts.

Well I kind of implied that you should put it into the commit message
for further reference. I don't really care at this point about the
difference but somebody might care in the future when touching the
tests.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[Jiri Denemark]

On Fri, May 06, 2016 at 10:02:12 +0200, Peter Krempa wrote:
> On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark 
> 
> Please explain the difference between caps_2.6.0-1.aarch64.replies
> caps_2.6.0-2.aarch64.replies. At least the difference how they were
> obtained.

Well, I asked Andrea to provide me some data from aarch64 and ppc64le
hosts :-) The difference is in GIC version supported on a two different
hosts.

> ACK

Thanks for the review. I implemented the changes you suggested and
pushed this series.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] cpu: add ABM to Haswell* and Broadwell* CPU models

[Eli Qiao]

Corresponding QEMU commits:
becb66673ec30cb604926d247ab9449a60ad8b11

Signed-off-by: Eli Qiao 
---
 src/cpu/cpu_map.xml | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 0b6d424..d9eb729 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -919,6 +919,7 @@
 
 
   
+  
   
   
   
@@ -971,6 +972,7 @@
 
 
   
+  
   
   
   
@@ -1026,6 +1028,7 @@
 
   
   
+  
   
   
   
@@ -1082,6 +1085,7 @@
 
   
   
+  
   
   
   
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/17] graphics: use enums instead of int

[Peter Krempa]

On Thu, May 05, 2016 at 18:20:20 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/domain_conf.c  |  2 ++
>  src/conf/domain_conf.h  | 14 +++---
>  src/libxl/libxl_conf.c  |  8 
>  src/qemu/qemu_command.c | 14 +-
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b7b099c..b825477 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1552,7 +1552,7 @@ typedef enum {
>  typedef struct _virDomainGraphicsListenDef virDomainGraphicsListenDef;
>  typedef virDomainGraphicsListenDef *virDomainGraphicsListenDefPtr;
>  struct _virDomainGraphicsListenDef {
> -int type;   /* enum virDomainGraphicsListenType */
> +virDomainGraphicsListenType type;

Same as below. Assigned in virDomainGraphicsListenDefParseXML.

>  char *address;
>  char *network;
>  bool fromConfig;/* true if the @address is config file originated */
> @@ -1564,7 +1564,7 @@ struct _virDomainGraphicsDef {
>   * Value 0 means port wasn't specified in XML at all.
>   * Positive value is actual port number given in XML.
>   */
> -int type;
> +virDomainGraphicsType type;

In virDomainGraphicsDefParseXML this struct member is directly assigned
without a temp variable so virDomainGraphicsTypeFromString won't be
able to return error as enum types are unsigned .


>  union {
>  struct {
>  int port;

ACK with a temp variable added.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/12] domaincapstest: Add tests for QEMU 2.6

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:14 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---
>  .../domaincaps-qemu_2.6.0-1.xml| 63 
>  .../domaincaps-qemu_2.6.0-2.xml| 63 
>  .../domaincaps-qemu_2.6.0-3.xml| 67 
> ++
>  .../domaincaps-qemu_2.6.0-4.xml| 67 
> ++
>  .../domaincaps-qemu_2.6.0-5.xml| 61 
>  tests/domaincapstest.c | 20 +++
>  6 files changed, 341 insertions(+)
>  create mode 100644 tests/domaincapsschemadata/domaincaps-qemu_2.6.0-1.xml
>  create mode 100644 tests/domaincapsschemadata/domaincaps-qemu_2.6.0-2.xml
>  create mode 100644 tests/domaincapsschemadata/domaincaps-qemu_2.6.0-3.xml
>  create mode 100644 tests/domaincapsschemadata/domaincaps-qemu_2.6.0-4.xml
>  create mode 100644 tests/domaincapsschemadata/domaincaps-qemu_2.6.0-5.xml

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/12] qemucapabilitiestest: Add tests for aarch64 and ppc64le

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:13 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 

Please explain the difference between caps_2.6.0-1.aarch64.replies
caps_2.6.0-2.aarch64.replies. At least the difference how they were
obtained.

> ---
>  .../caps_2.6.0-1.aarch64.replies   | 4940 
>  .../qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml  |  238 +
>  .../caps_2.6.0-1.ppc64le.replies   | 6163 
> 
>  .../qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml  |  604 ++
>  .../caps_2.6.0-2.aarch64.replies   | 4940 
>  .../qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml  |  238 +
>  tests/qemucapabilitiestest.c   |3 +
>  7 files changed, 17126 insertions(+)
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/12] domaincapstest: Use default machine type

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:12 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 

A little bit of explanation would be preferred.

> ---
>  tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml |  2 +-
>  tests/domaincapstest.c  | 17 
> ++---
>  2 files changed, 15 insertions(+), 4 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/12] qemucapabilitiestest: Rename *.caps to *.xml

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:11 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/12] qemucapabilitiestest: Test all capabilities

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:10 +0200, Jiri Denemark wrote:
> Enhance the test to cover all capabilities we probe for rather than
> testing the flags only.
> 
> Signed-off-by: Jiri Denemark 
> ---

[...]

> diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
> index 80f3cd8..1f2d5d7 100644
> --- a/tests/qemucapabilitiestest.c
> +++ b/tests/qemucapabilitiestest.c

[...]

> @@ -134,17 +137,19 @@ testQemuCaps(const void *opaque)
>  if (!(mon = testQemuFeedMonitor(replies, data->xmlopt)))
>  goto cleanup;
>  
> -if (!(capsExpected = qemuTestParseCapabilities(capsFile)))
> -goto cleanup;
> -
> -if (!(capsActual = virQEMUCapsNew()))
> -goto cleanup;
> -
> -if (virQEMUCapsInitQMPMonitor(capsActual,
> +if (!(capsActual = virQEMUCapsNew()) ||
> +virQEMUCapsInitQMPMonitor(capsActual,
>qemuMonitorTestGetMonitor(mon)) < 0)
>  goto cleanup;
>  
> -if (testQemuCapsCompare(capsExpected, capsActual) < 0)
> +if ((capsExpected = qemuTestParseCapabilities(capsFile)) &&
> +testQemuCapsFlagsCompare(capsExpected, capsActual) < 0)

So here you compare the flags only ...

> +goto cleanup;
> +
> +if (!(actual = virQEMUCapsFormatCache(capsActual, 0, 0)))
> +goto cleanup;
> +
> +if (virtTestCompareToFile(actual, capsFile) < 0)

and here you compare them again with the rest of the file. 

>  goto cleanup;
>  
>  ret = 0;

I think the first comparison can be dropped. This would require to fix
the flags first and then in a second run fix all the rest. The second
check should do the trick by itself.

ACK with the first check dropped


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/12] qemucapabilitiestest: Reorder flags in caps files

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:09 +0200, Jiri Denemark wrote:
> The flags should follow the order in which they are defined in
> virQEMUCaps enum.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  tests/qemucapabilitiesdata/caps_1.5.3-1.x86_64.caps  | 2 +-
>  tests/qemucapabilitiesdata/caps_1.6.0-1.x86_64.caps  | 2 +-
>  tests/qemucapabilitiesdata/caps_1.6.50-1.x86_64.caps | 2 +-
>  tests/qemucapabilitiesdata/caps_2.1.1-1.x86_64.caps  | 4 ++--
>  tests/qemucapabilitiesdata/caps_2.4.0-1.x86_64.caps  | 6 +++---
>  tests/qemucapabilitiesdata/caps_2.5.0-1.x86_64.caps  | 6 +++---
>  tests/qemucapabilitiesdata/caps_2.6.0-1.x86_64.caps  | 6 +++---
>  7 files changed, 14 insertions(+), 14 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/12] qemucapabilitiestest: Reindent *.caps files

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:08 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/12] qemucapabilitiestest: Uses consistent names

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:07 +0200, Jiri Denemark wrote:
> In other tests we use "expected" and "actual" to refer to the expected
> outcome of the tested API and the result we got, respectively.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  tests/qemucapabilitiestest.c | 31 ---
>  1 file changed, 16 insertions(+), 15 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/12] qemu: Export caps cache APIs for tests

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:06 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_capabilities.c | 4 ++--
>  src/qemu/qemu_capspriv.h | 9 +
>  2 files changed, 11 insertions(+), 2 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/12] qemu: Separate formatting from saving into caps cache

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:05 +0200, Jiri Denemark wrote:
> We will need to use the formatter directly for testing QEMU capabilities
> code.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_capabilities.c | 32 ++--
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index c2ba69d..862aed0 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c

[...]

> @@ -3094,10 +3095,21 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const 
> char *filename)
>  virBufferAdjustIndent(, -2);
>  virBufferAddLit(, "\n");
>  
> -if (virBufferCheckError() < 0)
> -goto cleanup;
> +if (virBufferCheckError() == 0)
> +ret = virBufferContentAndReset();
>  
> -xml = virBufferContentAndReset();
> +return ret;
> +}
> +

Two newlines to separate functions.

> +static int
> +virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename)
> +{
> +char *xml = NULL;
> +int ret = -1;
> +
> +xml = virQEMUCapsFormatCache(qemuCaps,
> + virGetSelfLastChanged(),
> + LIBVIR_VERSION_NUMBER);
>  
>  if (virFileWriteStr(filename, xml, 0600) < 0) {
>  virReportSystemError(errno,

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/12] qemucapabilitiestest: Prepare for testing non-x86_64 archs

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:04 +0200, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/12] tests: Refactor domaincapstest

[Peter Krempa]

On Thu, May 05, 2016 at 18:42:03 +0200, Jiri Denemark wrote:
> The test was just a big mess passing callbacks and their data through
> data for another callback.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  tests/domaincapstest.c | 176 
> ++---
>  1 file changed, 93 insertions(+), 83 deletions(-)

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virCgroupValidateMachineGroup: Reflect change in CGroup struct naming

[Pavel Hrdina]

On Thu, May 05, 2016 at 05:56:55PM +0200, Michal Privoznik wrote:
> Fron c3bd0019c0e on instead of creating the following path for
> cgroups:
> 
>   /sys/fs/cgroupX/$name.libvirt-$driver
> 
> we generate rather more verbose one:
> 
>   /sys/fs/cgroupX/$driver-$id-$name.libvirt-$driver
> 
> where $name is optional and included iff contains allowed chars.
> See original commit for more reasoning. Now, problem with the
> original commit is that we are unable to start any LXC domain
> after it. Because when starting LXC container, the CGroup layout
> is created by our lxc_controller process and then detected and
> validated by libvirtd. The validation is done by trying to match
> detected layout against all the possible patterns for cgroup
> paths that we've ever had. And the commit in question forgot to
> update this part of the code.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> I am really surprised how long this bug went unnoticed. Is
> anybody using LXC? When pushed, it should be backported to
> 1.3.2+.
> 
>  src/util/vircgroup.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index da5ccff..add6c5f 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -261,12 +261,17 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
>  char *scopename_new = NULL;
>  char *machinename = virSystemdMakeMachineName(drivername, id,
>name, privileged);
> +char *partmachinename = NULL;
>  
>  if (virAsprintf(, "%s.libvirt-%s",
> -name, drivername) < 0)
> +name, drivername) < 0 ||
> +(machinename &&
> + virAsprintf(, "%s.libvirt-%s",
> + machinename, drivername) < 0))
>  goto cleanup;
>  
> -if (virCgroupPartitionEscape() < 0)
> +if (virCgroupPartitionEscape() < 0 ||
> +(machinename && virCgroupPartitionEscape() < 0))
>  goto cleanup;

Instead of those composite conditions I would rather make something like this:

if (machinename) {
create partmachinename;
run virCgroupPartitionEscape();
}

It's the same but for me more readable and cleaner code.

ACK with or without the change.

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list