Re: [libvirt] [PATCH v2 1/3] storage: Adjust qemu-img switches check

[Peter Krempa]

On Mon, Jun 06, 2016 at 13:24:50 -0400, John Ferlan wrote:
> Since we support QEMU 0.12 and later, checking for support of specific flags
> added prior to that isn't necessary.
> 
> Thus start with the base of having the "-o options" available for the
> qemu-img create option and then determine whether we have the compat
> option for qcow2 files (which would be necessary up through qemu 2.0
> where the default changes to compat 0.11).
> 
> Adjust test to no long check for NONE and FLAG options as well was removing
> results of tests that would use that option.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c  | 91 
> +++---
>  tests/storagevolxml2argvdata/qcow2-flag.argv   |  2 -
>  .../qcow2-nobacking-convert-flag.argv  |  2 -
>  .../qcow2-nobacking-convert-none.argv  |  2 -
>  .../qcow2-nobacking-flag.argv  |  1 -
>  .../qcow2-nobacking-none.argv  |  1 -
>  tests/storagevolxml2argvdata/qcow2-none.argv   |  1 -
>  tests/storagevolxml2argvtest.c | 22 +-
>  8 files changed, 29 insertions(+), 93 deletions(-)
>  delete mode 100644 tests/storagevolxml2argvdata/qcow2-flag.argv
>  delete mode 100644 
> tests/storagevolxml2argvdata/qcow2-nobacking-convert-flag.argv
>  delete mode 100644 
> tests/storagevolxml2argvdata/qcow2-nobacking-convert-none.argv
>  delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-flag.argv
>  delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-none.argv
>  delete mode 100644 tests/storagevolxml2argvdata/qcow2-none.argv
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 8f03a6e..ff3f8c1 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -869,10 +869,15 @@ virStoragePloopResize(virStorageVolDefPtr vol,
>  return ret;
>  }
>  
> +/* Flag values shared w/ storagevolxml2argvtest.c.
> + *
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11)
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT
> + *was made necessary due to 2.0 change to change the default
> + *qcow2 file format from 0.10 to 1.1.
> + */
>  enum {
> -QEMU_IMG_BACKING_FORMAT_NONE = 0,
> -QEMU_IMG_BACKING_FORMAT_FLAG,
> -QEMU_IMG_BACKING_FORMAT_OPTIONS,
> +QEMU_IMG_BACKING_FORMAT_OPTIONS = 0,
>  QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,

So if these are declared as flags they either should be binary or-able
or be used as indexes into a virBitmap as we do for qemu caps.

This use doesn't make sense as you can effectively return just one
capability.

>  };
>  
> @@ -904,46 +909,18 @@ virStorageBackendQemuImgSupportsCompat(const char 
> *qemuimg)
>  static int
>  virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
>  {
> -char *help = NULL;
> -char *start;
> -char *end;
> -char *tmp;
> -int ret = -1;
> -int exitstatus;
> -virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL);
> -
> -virCommandAddEnvString(cmd, "LC_ALL=C");
> -virCommandSetOutputBuffer(cmd, &help);
> -virCommandClearCaps(cmd);
> -
> -/* qemuimg doesn't return zero exit status on -h,
> - * therefore we need to provide pointer for storing
> - * exit status, although we don't parse it any later */
> -if (virCommandRun(cmd, &exitstatus) < 0)
> -goto cleanup;
> -
> -if ((start = strstr(help, " create ")) == NULL ||
> -(end = strstr(start, "\n")) == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("unable to parse qemu-img output '%s'"),
> -   help);
> -goto cleanup;
> -}
> -if (((tmp = strstr(start, "-F fmt")) && tmp < end) ||
> -((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) {
> -ret = QEMU_IMG_BACKING_FORMAT_FLAG;
> -} else if ((tmp = strstr(start, "[-o options]")) && tmp < end) {
> -if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
> -else
> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
> -} else {
> -ret = QEMU_IMG_BACKING_FORMAT_NONE;
> -}
> +/* As of QEMU 0.11 the [-o options] support was added via qemu
> + * commit id '9ea2ea71', so we start with that base and figure
> + * out what else we have */
> +int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;

If this flag is always set (and 0) it doesn't make sense to keep it.

> +
> +/* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
> + * understands. Since we still support QEMU 0.12 and newer, we need
> + * to be able to handle the previous format as can be set via a
> + * compat=0.10 option. */
> +if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> +ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
>  
> - cleanup:
> -virCommandFree(cmd);
> -VIR_FREE(help);
>  return ret;
>  }
>  

--
libvir-l

Re: [libvirt] [PATCH v2] systemd: directly notify systemd instead of using sd_notify

[Peter Krempa]

On Mon, Jun 06, 2016 at 16:21:30 +0100, Daniel Berrange wrote:
> The sd_notify method is used to tell systemd when libvirtd
> has finished starting up. All it does is send a datagram
> containing the string parameter to systemd on a UNIX socket
> named in the NOTIFY_SOCKET environment variable. Rather than
> pulling in the systemd libraries for this, just code the
> notification directly in libvirt as this is a stable ABI
> from systemd's POV which explicitly allows independant
> implementations:
> 
> See "Reimplementable Independently" column in the
> "$NOTIFY_SOCKET Daemon Notifications" row:
> 
> https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  configure.ac  |  2 --
>  libvirt.spec.in   | 12 ---
>  m4/virt-systemd-daemon.m4 | 34 --
>  src/Makefile.am   |  4 ++--
>  src/util/virsystemd.c | 53 
> ++-
>  5 files changed, 50 insertions(+), 55 deletions(-)
>  delete mode 100644 m4/virt-systemd-daemon.m4

[...]

I've read into the quirks in 'man 7 unix' and they are fun. Nothing
worth bikeshedding about in case of linux though.

ACK

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

Re: [libvirt] [PATCH 2/3] vz: fix crash when parsing unexpected disk configuration

[Nikolay Shirokovskiy]

On 06.06.2016 20:59, Maxim Nestratov wrote:
> As it turned out PrlVmDev_GetStackIndex can return negative values
> without reporting an error, which is incorrect but nevertheless.
> After that we feed this negative index to virIndexToDiskName,
> which in turn returns NULL and we set it to virDomainDiskDef.dst.
> Using virDiskNameToBusDeviceIndex with a virDomainDiskDef structure
> which has NULL dst field crashes.
> Fix this by returning an error in prlsdkGetDiskId in such cases.
> 
> Signed-off-by: Maxim Nestratov 
> ---
>  src/vz/vz_sdk.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 7eb78ca..947db3a 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -543,6 +543,9 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, 
> char **dst)
>  return -1;
>  }
>  
> +if (NULL == *dst)
> +return -1;
> +
>  return 0;
>  }
>  
> 

ACK

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

Re: [libvirt] [PATCH] Updated to deal with specifying user IDs to that do not map to usernames

[Peter Krempa]

On Mon, Jun 06, 2016 at 14:25:23 -0500, Roy Keene wrote:
> Patch to libvirt master to avoid failing when a user ID is specified, 
> e.g. for , that does not map to a user name.
> 
> This is useful if you want to run each VM as a separate user and not 
> bother creating an /etc/passwd entry for each UID.

For this use case you shall prefix the name with a +. Please refer to
the documentation on seclabels.

https://libvirt.org/formatdomain.html#seclabel

> 
> It compiles but is as yet untested.
> 
> ---
>   src/util/virutil.c | 69 
> +++---
>   1 file changed, 50 insertions(+), 19 deletions(-)

NACK to this patch

Peter

> 
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index d80d994..ae95237 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -790,26 +790,57 @@ virGetUserEnt(uid_t uid, char **name, gid_t 
> *group, char **dir)
>   if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
>   goto cleanup;
>   }
> -if (rc != 0) {
> -virReportSystemError(rc,
> - _("Failed to find user record for uid '%u'"),
> - (unsigned int) uid);
> -goto cleanup;
> -} else if (pw == NULL) {
> -virReportError(VIR_ERR_SYSTEM_ERROR,
> -   _("Failed to find user record for uid '%u'"),
> -   (unsigned int) uid);
> -goto cleanup;
> -}
> 
> -if (name && VIR_STRDUP(*name, pw->pw_name) < 0)
> -goto cleanup;
> -if (group)
> -*group = pw->pw_gid;
> -if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) {
> -if (name)
> -VIR_FREE(*name);
> -goto cleanup;
> +if (rc != 0 || pw == NULL) {
> +/*
> + * If the user does not exist or its data is not present, return
> + * a created username.
> + */
> + VIR_FREE(strbuf);
> +
> + strbuflen = 128;
> +
> + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) {
> + return(-1);
> + }
> +
> + /*
> +  * Fake user home directory: /
> +  */
> + if (dir) {
> + if (VIR_STRDUP(*dir, "/") < 0) {
> + goto cleanup;
> + }
> + }

Erm no. We should not do this. Not at this level.

> +
> + /*
> +  * Fake user GID: Same as UID
> +  */
> + if (group) {
> + *group = (gid_t) uid;
> + }
> +
> + /*
> +  * Fake user name: Same as UID (in string)
> +  */
> + snprintf(strbuf, strbuflen, "%llu", (unsigned long long) uid);
> +
> + if (name && VIR_STRDUP(*name, strbuf) < 0) {
> + if (dir) {
> + VIR_FREE(*dir);
> + }
> + goto cleanup;
> + }
> +} else {
> +if (name && VIR_STRDUP(*name, pw->pw_name) < 0)
> +goto cleanup;
> +if (group)
> +*group = pw->pw_gid;
> +if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) {
> +if (name)
> +VIR_FREE(*name);
> +goto cleanup;
> +}
>   }
> 
>   ret = 0;
> -- 
> 2.7.4
> 



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

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

Re: [libvirt] [PATCH] apparmor: Don't scrub environment of virtlogd process

[Jim Fehlig]

On 06/05/2016 05:06 AM, Guido Günther wrote:
> otherwise we drop variables like XDG_RUNTIME_DIR with qemu:///session
> and libvirtd faild to find virtlogd's socket.
> ---
>  examples/apparmor/usr.sbin.libvirtd | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/examples/apparmor/usr.sbin.libvirtd 
> b/examples/apparmor/usr.sbin.libvirtd
> index 5d606e6..23f70f5 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -45,6 +45,7 @@
>/bin/* PUx,
>/sbin/* PUx,
>/usr/bin/* PUx,
> +  /usr/sbin/virtlogd pix,
>/usr/sbin/* PUx,
>/lib/udev/scsi_id PUx,
>/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,

I took a quick peek and it seems XDC_RUNTIME_DIR is the only environment
variable accessed by virtlogd. So I think this is fine, but soft ACK since I'm
not the apparmor expert here.

BTW, I didn't test this myself but recently received a bug report against
gnome-boxes for the issue and asked those affected to test your patch. I
received positive feedback from at least one user

https://bugzilla.opensuse.org/show_bug.cgi?id=980441

Regards,
Jim

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

[libvirt] [PATCH v2 5/6] 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 f455d3d..b8519ba 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -189,7 +189,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",
@@ -209,17 +209,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,
 };
 
@@ -235,8 +235,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,
@@ -246,9 +246,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,
@@ -257,7 +257,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,
@@ -1941,8 +1941,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)
 {
@@ -1971,8 +1971,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, &local_err);
 } else if (featurestr[0] == '-') {
+feat2prop(featurestr);
 add_flagname_to_bitmaps(featurestr + 1, minus_features, 
&local_err);
 } else if ((val = strchr(featurestr, '='))) {
 *val = 0; val++;
@@ -3180,11 +3182,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], &cpu->env.features[w], bitnr);
 
 for (i = 1; names[i]; i++) {
-feat2prop(names[i]);
 object_property_add_alias(obj, names[i], obj, names[0],
   &error_

[libvirt] [PATCH v2 2/6] 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 aaa239a..bd815f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2165,9 +2165,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;
 }
 }
@@ -2175,6 +2172,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;
@@ -2979,12 +2985,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(&local_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(&local_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 v2 4/6] 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 
---
Changes v1 -> v2:
* Remove @runnable field, non-empty @unavailable-features is
  enough to report CPU model as not runnable.
* Documentation updates:
  * Changed to "(since 2.7)";
  * Add more details about the exact meaning of
unavailable-features, and what it would mean to see
read-only QOM properties in the list, and that
implementations can return "type" if there's
no extra information available;
---
 qapi-schema.json | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8483bdf..43478e9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3005,11 +3005,32 @@
 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
+# @unavailable-features: #optional List of properties that prevent
+#the CPU model from running in the current
+#host. (since 2.7)
+#
+# @unavailable-features is a list of QOM property names that
+# represent CPU model attributes that prevent the CPU from running.
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU model run in the current host. If
+# absolutely no extra information will be returned to explain why
+# the CPU model is not runnable, implementations may simply
+# return "type" as the property name.
+# If the property is read-write, it means that it MAY be possible
+# to run the CPU model in the current host if that property is
+# changed. Management software can use it as hints to suggest or
+# choose an alternative for the user, or just to generate meaningful
+# error messages explaining why the CPU model can't be used.
+# If @unavailable-features is an empty list, the CPU model is
+# runnable using the current host and machine-type.
+# If @unavailable-features is not present, runnability
+# information for the CPU is not available.
 #
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str',
+'*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 v2 6/6] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "unavailable-features" field on the x86 implementation
of query-cpu-definitions.

Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost 
---
Changes v1 -> v2:
* Updated to the new schema: no @runnable field, and
  always report @unavailable-features as present
---
 target-i386/cpu.c | 41 +
 1 file changed, 41 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b8519ba..65d1ffc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2054,6 +2054,45 @@ static void x86_cpu_report_filtered_features(X86CPU *cpu)
 }
 }
 
+/* Check for missing features that may prevent the CPU class from
+ * running using the current machine and accelerator.
+ */
+static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
+ strList **missing_feats)
+{
+X86CPU *xc;
+FeatureWord w;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("kvm");;
+*missing_feats = new;
+return;
+}
+
+xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
+if (x86_cpu_filter_features(xc)) {
+for (w = 0; w < FEATURE_WORDS; w++) {
+FeatureWordInfo *fi = &feature_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));
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2146,6 +2185,8 @@ 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);
+x86_cpu_class_check_missing_features(cc, &info->unavailable_features);
+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 v2 1/6] 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 5dde658..e724004 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -63,6 +63,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 a62d731..aaa239a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -678,6 +678,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;
@@ -1531,6 +1539,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void 
*data)
 cpu_x86_fill_model_id(host_cpudef.model_id);
 
 xcc->cpu_def = &host_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.
@@ -2022,23 +2033,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 = &builtin_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, &s);
+g_slist_free(list);
 
 (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2050,26 +2100,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 **cpu_list = user_data;
+ 

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

[Eduardo Habkost]

This series extends query-cpu-definitions to include an extra
field: "unavailable-features". The new field can be used to find
out reasons that prevent the CPU model from running in the
current host.

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

Changes v1 -> v2:
* Fixed documentation to say "(since 2.7)"
* Removed @runnable field, improved documentation

Example command output:

  { "return": [
  {
"unavailable-features": [ "kvm" ],
 "name": "host"
  },
  {
"unavailable-features": [],
"name": "qemu64"
  },
  {
"unavailable-features": [],
"name": "qemu32"
  },
  {
"unavailable-features": ["npt", "fxsr-opt", "vme"],
"name": "phenom"
  },
  {
"unavailable-features": ["vme"],
"name": "pentium3"
  },
  {
"unavailable-features": ["vme"],
"name": "pentium2"
  },
  {
"unavailable-features": ["vme"],
"name": "pentium"
  },
  {
"unavailable-features": ["vme"],
"name": "n270"
  },
  {
"unavailable-features": ["vme"],
"name": "kvm64"
  },
  {
"unavailable-features": ["vme"],
"name": "kvm32"
  },
  {
"unavailable-features": ["vme"],
"name": "coreduo"
  },
  {
"unavailable-features": ["vme"],
"name": "core2duo"
  },
  {
"unavailable-features": ["vme"],
"name": "athlon"
  },
  {
"unavailable-features": ["vme"],
"name": "Westmere"
  },
  {
"unavailable-features": ["xsavec", "3dnowprefetch", "rdseed", "rtm", 
"invpcid", "erms", "avx2", "hle", "rdrand", "f16c", "avx", "tsc-deadline", 
"x2apic", "pcid", "fma", "vme"],
"name": "Skylake-Client"
  },
  {
"unavailable-features": ["avx", "tsc-deadline", "x2apic", "vme"],
"name": "SandyBridge"
  },
  {
"unavailable-features": ["vme"],
"name": "Penryn"
  },
  {
"unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", 
"misalignsse", "f16c", "avx", "fma", "vme"],
"name": "Opteron_G5"
  },
  {
"unavailable-features": ["fma4", "xop", "3dnowprefetch", "misalignsse", 
"avx", "vme"],
"name": "Opteron_G4"
  },
  {
"unavailable-features": ["misalignsse", "vme"],
"name": "Opteron_G3"
  },
  {
"unavailable-features": ["vme"],
"name": "Opteron_G2"
  },
  {
"unavailable-features": ["vme"],
"name": "Opteron_G1"
  },
  {
"unavailable-features": ["vme"],
"name": "Nehalem"
  },
  {
"unavailable-features": ["erms", "rdrand", "f16c", "avx", 
"tsc-deadline", "x2apic", "vme"],
"name": "IvyBridge"
  },
  {
"unavailable-features": ["rtm", "invpcid", "erms", "avx2", "hle", 
"rdrand", "f16c", "avx", "tsc-deadline", "x2apic", "pcid", "fma", "vme"],
"name": "Haswell"
  },
  {
"unavailable-features": ["invpcid", "erms", "avx2", "rdrand", "f16c", 
"avx", "tsc-deadline", "x2apic", "pcid", "fma", "vme"],
"name": "Haswell-noTSX"
  },
  {
"unavailable-features": ["vme"],
"name": "Conroe"
  },
  {
"unavailable-features": ["3dnowprefetch", "rdseed", "rtm", "invpcid", 
"erms", "avx2", "hle", "rdrand", "f16c", "avx", "tsc-deadline", "x2apic", 
"pcid", "fma", "vme"],
"name": "Broadwell"
  },
  {
"unavailable-features": ["3dnowprefetch", "rdseed", "invpcid", "erms", 
"avx2", "rdrand", "f16c", "avx", "tsc-deadline", "x2apic", "pcid", "fma", 
"vme"],
"name": "Broadwell-noTSX"
  },
  {
"unavailable-features": ["vme"],
"name": "486"
  }
  ]}

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

Eduardo Habkost (6):
  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  |  23 -
 target-i386/cpu-qom.h |   4 +
 target-i386/cpu.c | 262 +++---
 3 files changed, 209 insertions(+), 80 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH v2 3/6] 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 bd815f3..f455d3d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2018,6 +2018,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 = &cpu->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)
@@ -2147,40 +2181,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 = &cpu->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] Updated to deal with specifying user IDs to that do not map to usernames

[Roy Keene]

Patch to libvirt master to avoid failing when a user ID is specified, 
e.g. for , that does not map to a user name.


This is useful if you want to run each VM as a separate user and not 
bother creating an /etc/passwd entry for each UID.


It compiles but is as yet untested.

---
 src/util/virutil.c | 69 
+++---

 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index d80d994..ae95237 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -790,26 +790,57 @@ virGetUserEnt(uid_t uid, char **name, gid_t 
*group, char **dir)

 if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
 goto cleanup;
 }
-if (rc != 0) {
-virReportSystemError(rc,
- _("Failed to find user record for uid '%u'"),
- (unsigned int) uid);
-goto cleanup;
-} else if (pw == NULL) {
-virReportError(VIR_ERR_SYSTEM_ERROR,
-   _("Failed to find user record for uid '%u'"),
-   (unsigned int) uid);
-goto cleanup;
-}

-if (name && VIR_STRDUP(*name, pw->pw_name) < 0)
-goto cleanup;
-if (group)
-*group = pw->pw_gid;
-if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) {
-if (name)
-VIR_FREE(*name);
-goto cleanup;
+if (rc != 0 || pw == NULL) {
+/*
+ * If the user does not exist or its data is not present, return
+ * a created username.
+ */
+ VIR_FREE(strbuf);
+
+ strbuflen = 128;
+
+ if (VIR_ALLOC_N(strbuf, strbuflen) < 0) {
+ return(-1);
+ }
+
+ /*
+  * Fake user home directory: /
+  */
+ if (dir) {
+ if (VIR_STRDUP(*dir, "/") < 0) {
+ goto cleanup;
+ }
+ }
+
+ /*
+  * Fake user GID: Same as UID
+  */
+ if (group) {
+ *group = (gid_t) uid;
+ }
+
+ /*
+  * Fake user name: Same as UID (in string)
+  */
+ snprintf(strbuf, strbuflen, "%llu", (unsigned long long) uid);
+
+ if (name && VIR_STRDUP(*name, strbuf) < 0) {
+ if (dir) {
+ VIR_FREE(*dir);
+ }
+ goto cleanup;
+ }
+} else {
+if (name && VIR_STRDUP(*name, pw->pw_name) < 0)
+goto cleanup;
+if (group)
+*group = pw->pw_gid;
+if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) {
+if (name)
+VIR_FREE(*name);
+goto cleanup;
+}
 }

 ret = 0;
--
2.7.4



smime.p7s
Description: S/MIME Cryptographic Signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] systemd: directly notify systemd instead of using sd_notify

[Cole Robinson]

On 06/06/2016 11:21 AM, Daniel P. Berrange wrote:
> The sd_notify method is used to tell systemd when libvirtd
> has finished starting up. All it does is send a datagram
> containing the string parameter to systemd on a UNIX socket
> named in the NOTIFY_SOCKET environment variable. Rather than
> pulling in the systemd libraries for this, just code the
> notification directly in libvirt as this is a stable ABI
> from systemd's POV which explicitly allows independant
> implementations:
> 
> See "Reimplementable Independently" column in the
> "$NOTIFY_SOCKET Daemon Notifications" row:
> 
> https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> 
> Signed-off-by: Daniel P. Berrange 

FWIW I tested that things build fine with this patch on f24 and rawhide

- Cole

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

[libvirt] [PATCH 2/3] vz: fix crash when parsing unexpected disk configuration

[Maxim Nestratov]

As it turned out PrlVmDev_GetStackIndex can return negative values
without reporting an error, which is incorrect but nevertheless.
After that we feed this negative index to virIndexToDiskName,
which in turn returns NULL and we set it to virDomainDiskDef.dst.
Using virDiskNameToBusDeviceIndex with a virDomainDiskDef structure
which has NULL dst field crashes.
Fix this by returning an error in prlsdkGetDiskId in such cases.

Signed-off-by: Maxim Nestratov 
---
 src/vz/vz_sdk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7eb78ca..947db3a 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -543,6 +543,9 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char 
**dst)
 return -1;
 }
 
+if (NULL == *dst)
+return -1;
+
 return 0;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH 0/3] vz: util: some crash fixes

[Maxim Nestratov]

Maxim Nestratov (3):
  util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects
  vz: fix crash when parsing unexpected disk configuration
  util: fix potential crash in virDiskNameParse

 src/util/virclosecallbacks.c | 2 ++
 src/util/virutil.c   | 3 +++
 src/vz/vz_sdk.c  | 3 +++
 3 files changed, 8 insertions(+)

-- 
1.8.3.1

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

[libvirt] [PATCH 1/3] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects

[Maxim Nestratov]

There is a possibility that qemu driver frees by unreferencing its
closeCallbacks pointer as it has the only reference to the object,
while in fact not all users of CloseCallbacks called thier
virCloseCallbacksUnset.

Backtrace is the following:
Thread #1:
0  in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
1  in virCondWait (c=, m=)
at util/virthread.c:154
2  in virThreadPoolFree (pool=0x7f0810110b50)
at util/virthreadpool.c:266
3  in qemuStateCleanup () at qemu/qemu_driver.c:1116
4  in virStateCleanup () at libvirt.c:808
5  in main (argc=, argv=)
at libvirtd.c:1660

Thread #2:
0  in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at 
util/virobject.c:169
1  in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=) at util/virobject.c:365
2  in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317
3  in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, 
vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 
) at util/virclosecallbacks.c:163
4  in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, 
vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368
5  in qemuProcessStop (driver=driver@entry=0x7f081018be50, 
vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, 
asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at 
qemu/qemu_process.c:5854
6  in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at 
qemu/qemu_driver.c:4585
7  qemuProcessEventHandler (data=, opaque=0x7f081018be50) at 
qemu/qemu_driver.c:4629
8  in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at 
util/virthreadpool.c:145
9  in virThreadHelper (data=) at util/virthread.c:206
10 in start_thread () from /lib64/libpthread.so.0

Let's reference CloseCallbacks object in virCloseCallbacksSet and
unreference in virCloseCallbacksUnset.

Signed-off-by: Maxim Nestratov 
---
 src/util/virclosecallbacks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
index 82d4071..2fab56b 100644
--- a/src/util/virclosecallbacks.c
+++ b/src/util/virclosecallbacks.c
@@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks,
 virObjectRef(vm);
 }
 
+virObjectRef(closeCallbacks);
 ret = 0;
  cleanup:
 virObjectUnlock(closeCallbacks);
@@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks,
 goto cleanup;
 
 virObjectUnref(vm);
+virObjectUnref(closeCallbacks);
 ret = 0;
  cleanup:
 virObjectUnlock(closeCallbacks);
-- 
1.8.3.1

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

[libvirt] [PATCH 3/3] util: fix potential crash in virDiskNameParse

[Maxim Nestratov]

As far as virDiskNameToIndex calls virDiskNameParse and doesn't
check parameters and in most cases is used with disk->dst as a
parameter and disk->dst is by virIndexToDiskName, which can
return NULL, virDiskNameParse can crash in such cases.
Let's be paranoic and sustain even such incorrect usage.

Signed-off-by: Maxim Nestratov 
---
 src/util/virutil.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index d80d994..05e136d 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -556,6 +556,9 @@ int virDiskNameParse(const char *name, int *disk, int 
*partition)
 static char const* const drive_prefix[] = {"fd", "hd", "vd", "sd", "xvd", 
"ubd"};
 size_t i;
 
+if (!name)
+return -1;
+
 for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
 if (STRPREFIX(name, drive_prefix[i])) {
 ptr = name + strlen(drive_prefix[i]);
-- 
1.8.3.1

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

[libvirt] [PATCH REPOST 2/4] storage: Use virRandomBytes for virStorageGenerateQcowPassphrase

[John Ferlan]

Use the common API

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c   | 10 +++---
 src/util/virstorageencryption.c | 42 +++--
 src/util/virstorageencryption.h |  4 ++--
 3 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 8f03a6e..fd432c8 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -597,7 +597,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn,
 virStorageEncryptionSecretPtr enc_secret = NULL;
 virSecretPtr secret = NULL;
 char *xml;
-unsigned char value[VIR_STORAGE_QCOW_PASSPHRASE_SIZE];
+unsigned char *value = NULL;
 int ret = -1;
 
 if (conn->secretDriver == NULL ||
@@ -641,10 +641,13 @@ virStorageGenerateQcowEncryption(virConnectPtr conn,
 }
 VIR_FREE(xml);
 
-if (virStorageGenerateQcowPassphrase(value) < 0)
+if (!(value =
+  virStorageGenerateQcowPassphrase(VIR_STORAGE_QCOW_PASSPHRASE_SIZE)))
 goto cleanup;
 
-if (conn->secretDriver->secretSetValue(secret, value, sizeof(value), 0) < 
0)
+if (conn->secretDriver->secretSetValue(secret, value,
+   VIR_STORAGE_QCOW_PASSPHRASE_SIZE,
+   0) < 0)
 goto cleanup;
 
 enc_secret->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE;
@@ -666,6 +669,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn,
 virBufferFreeAndReset(&buf);
 virSecretDefFree(def);
 VIR_FREE(enc_secret);
+VIR_FREE(value);
 return ret;
 }
 
diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c
index 8105158..00d1ff7 100644
--- a/src/util/virstorageencryption.c
+++ b/src/util/virstorageencryption.c
@@ -1,7 +1,7 @@
 /*
  * virstorageencryption.c: volume encryption information
  *
- * Copyright (C) 2009-2014 Red Hat, Inc.
+ * Copyright (C) 2009-2014, 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -34,6 +34,7 @@
 #include "virerror.h"
 #include "viruuid.h"
 #include "virfile.h"
+#include "virrandom.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
@@ -284,36 +285,23 @@ virStorageEncryptionFormat(virBufferPtr buf,
 return 0;
 }
 
-int
-virStorageGenerateQcowPassphrase(unsigned char *dest)
+unsigned char *
+virStorageGenerateQcowPassphrase(size_t nbytes)
 {
-int fd;
-size_t i;
+int ret;
+uint8_t *value;
+
+if (VIR_ALLOC_N(value, nbytes) < 0)
+return NULL;
 
 /* A qcow passphrase is up to 16 bytes, with any data following a NUL
ignored.  Prohibit control and non-ASCII characters to avoid possible
unpleasant surprises with the qemu monitor input mechanism. */
-fd = open("/dev/urandom", O_RDONLY);
-if (fd < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Cannot open /dev/urandom"));
-return -1;
-}
-i = 0;
-while (i < VIR_STORAGE_QCOW_PASSPHRASE_SIZE) {
-ssize_t r;
-
-while ((r = read(fd, dest + i, 1)) == -1 && errno == EINTR)
-;
-if (r <= 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Cannot read from /dev/urandom"));
-VIR_FORCE_CLOSE(fd);
-return -1;
-}
-if (dest[i] >= 0x20 && dest[i] <= 0x7E)
-i++; /* Got an acceptable character */
+if ((ret = virRandomBytes(value, nbytes, 0x20, 0x7E)) < 0) {
+virReportSystemError(ret, "%s", _("failed to generate passphrase"));
+VIR_FREE(value);
+return NULL;
 }
-VIR_FORCE_CLOSE(fd);
-return 0;
+
+return value;
 }
diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h
index 04641b1..bdfaa15 100644
--- a/src/util/virstorageencryption.h
+++ b/src/util/virstorageencryption.h
@@ -1,7 +1,7 @@
 /*
  * virstorageencryption.h: volume encryption information
  *
- * Copyright (C) 2009-2011, 2014 Red Hat, Inc.
+ * Copyright (C) 2009-2011, 2014, 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -76,6 +76,6 @@ enum {
   VIR_STORAGE_QCOW_PASSPHRASE_SIZE = 16
 };
 
-int virStorageGenerateQcowPassphrase(unsigned char *dest);
+unsigned char *virStorageGenerateQcowPassphrase(size_t nbytes);
 
 #endif /* __VIR_STORAGE_ENCRYPTION_H__ */
-- 
2.5.5

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

[libvirt] [PATCH REPOST 1/4] util: Add range parameters to virRandomBytes

[John Ferlan]

Add a minval and maxval range for acceptible values from /dev/urandom

Signed-off-by: John Ferlan 
---
 src/util/vircrypto.c |  2 +-
 src/util/virrandom.c | 13 ++---
 src/util/virrandom.h |  3 ++-
 src/util/viruuid.c   |  2 +-
 tests/qemuxml2argvmock.c |  2 +-
 tests/vircryptotest.c|  4 ++--
 tests/virrandommock.c| 10 +++---
 tests/virrandomtest.c| 32 
 8 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 4f288f0..4125230 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -301,7 +301,7 @@ virCryptoGenerateRandom(size_t nbytes)
 /* If we don't have gnutls_rnd(), we will generate a less cryptographically
  * strong master buf from /dev/urandom.
  */
-if ((ret = virRandomBytes(buf, nbytes)) < 0) {
+if ((ret = virRandomBytes(buf, nbytes, 0x00, 0xff)) < 0) {
 virReportSystemError(ret, "%s", _("failed to generate byte stream"));
 VIR_FREE(buf);
 return NULL;
diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 62a0e31..256819a 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -164,13 +164,17 @@ uint32_t virRandomInt(uint32_t max)
  * virRandomBytes
  * @buf: Pointer to location to store bytes
  * @buflen: Number of bytes to store
+ * @minval: Minimum value acceptable
+ * @maxval: Minimum value acceptable
  *
  * Generate a stream of random bytes from /dev/urandom
  * into @buf of size @buflen
  */
 int
 virRandomBytes(unsigned char *buf,
-   size_t buflen)
+   size_t buflen,
+   uint8_t minval,
+   uint8_t maxval)
 {
 int fd;
 
@@ -187,8 +191,11 @@ virRandomBytes(unsigned char *buf,
 return n < 0 ? errno : ENODATA;
 }
 
-buf += n;
-buflen -= n;
+/* Compare to acceptable range */
+if (*buf >= minval && *buf <= maxval) {
+buf += n;
+buflen -= n;
+}
 }
 
 VIR_FORCE_CLOSE(fd);
diff --git a/src/util/virrandom.h b/src/util/virrandom.h
index f457d2d..6b3fb0f 100644
--- a/src/util/virrandom.h
+++ b/src/util/virrandom.h
@@ -27,7 +27,8 @@
 uint64_t virRandomBits(int nbits);
 double virRandom(void);
 uint32_t virRandomInt(uint32_t max);
-int virRandomBytes(unsigned char *buf, size_t buflen)
+int virRandomBytes(unsigned char *buf, size_t buflen,
+   uint8_t minval, uint8_t maxval)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 int virRandomGenerateWWN(char **wwn, const char *virt_type);
 
diff --git a/src/util/viruuid.c b/src/util/viruuid.c
index 3cbaae0..f494adf 100644
--- a/src/util/viruuid.c
+++ b/src/util/viruuid.c
@@ -76,7 +76,7 @@ virUUIDGenerate(unsigned char *uuid)
 if (uuid == NULL)
 return -1;
 
-if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN))) {
+if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN, 0x00, 0xff))) {
 char ebuf[1024];
 VIR_WARN("Falling back to pseudorandom UUID,"
  " failed to generate random bytes: %s",
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index c6a1f98..76511af 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -157,7 +157,7 @@ virCryptoGenerateRandom(size_t nbytes)
 if (VIR_ALLOC_N(buf, nbytes) < 0)
 return NULL;
 
-ignore_value(virRandomBytes(buf, nbytes));
+ignore_value(virRandomBytes(buf, nbytes, 0x00, 0xff));
 
 return buf;
 }
diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c
index 72265d9..0dd2cf4 100644
--- a/tests/vircryptotest.c
+++ b/tests/vircryptotest.c
@@ -87,8 +87,8 @@ testCryptoEncrypt(const void *opaque)
 VIR_ALLOC_N(iv, ivlen) < 0)
 goto cleanup;
 
-if (virRandomBytes(enckey, enckeylen) < 0 ||
-virRandomBytes(iv, ivlen) < 0)
+if (virRandomBytes(enckey, enckeylen, 0x00, 0xff) < 0 ||
+virRandomBytes(iv, ivlen, 0x00, 0xff) < 0)
 goto cleanup;
 
 if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen,
diff --git a/tests/virrandommock.c b/tests/virrandommock.c
index 6df5e20..d988bab 100644
--- a/tests/virrandommock.c
+++ b/tests/virrandommock.c
@@ -28,12 +28,16 @@
 
 int
 virRandomBytes(unsigned char *buf,
-   size_t buflen)
+   size_t buflen,
+   uint8_t minval,
+   uint8_t maxval)
 {
 size_t i;
 
-for (i = 0; i < buflen; i++)
-buf[i] = i;
+for (i = 0; i < buflen; i++) {
+if (i >= minval && i <= maxval)
+buf[i] = i;
+}
 
 return 0;
 }
diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c
index 367bdc7..be148d2 100644
--- a/tests/virrandomtest.c
+++ b/tests/virrandomtest.c
@@ -29,27 +29,31 @@
 
 # define VIR_FROM_THIS VIR_FROM_NONE
 
+struct testRandomData {
+uint8_t minval;
+uint8_t maxval;
+};
+
 static int
-testRandomBytes(const void *unused ATTRIBUTE_UNUSED)
+testRandomBytes(const void *opaque)
 {
 int

[libvirt] [PATCH REPOST 4/4] util: Adjust virCryptoEncryptData code to use macros

[John Ferlan]

Will make it easier to add new key lengths

Signed-off-by: John Ferlan 
---
 src/util/vircrypto.c | 50 +++---
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 27a3d1d..f50ac6a 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -229,36 +229,40 @@ virCryptoEncryptData(virCryptoCipher algorithm,
  uint8_t **ciphertext,
  size_t *ciphertextlen)
 {
+/*
+ * Encrypt the data buffer using an encryption key and
+ * initialization vector via the gnutls_cipher_encrypt API
+ * for the specific cipher algorithm.
+ */
+# define DO_CRYPT(ekl, ivl, alg, nam)   \
+do {\
+if (enckeylen != ekl) { \
+virReportError(VIR_ERR_INVALID_ARG, \
+   _("'%s' encryption invalid keylen=%d"),  \
+   nam, ekl);   \
+return -1;  \
+}   \
+if (ivlen != ivl) { \
+virReportError(VIR_ERR_INVALID_ARG, \
+   _("'%s' initialization vector invalid len=%d"),  \
+   nam, ivl);   \
+return -1;  \
+}   \
+return virCryptoEncryptDataAESgnutls(alg, enckey, enckeylen,\
+ iv, ivlen, data, datalen,  \
+ ciphertext, ciphertextlen);\
+} while (0);
+
 switch (algorithm) {
 case VIR_CRYPTO_CIPHER_AES256CBC:
-if (enckeylen != 32) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("AES256CBC encryption invalid keylen=%zu"),
-   enckeylen);
-return -1;
-}
-
-if (ivlen != 16) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("AES256CBC initialization vector invalid 
len=%zu"),
-   ivlen);
-return -1;
-}
-
-/*
- * Encrypt the data buffer using an encryption key and
- * initialization vector via the gnutls_cipher_encrypt API
- * for GNUTLS_CIPHER_AES_256_CBC.
- */
-return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC,
- enckey, enckeylen, iv, ivlen,
- data, datalen,
- ciphertext, ciphertextlen);
+DO_CRYPT(32, 16, GNUTLS_CIPHER_AES_256_CBC, "AES256CBC");
 
 case VIR_CRYPTO_CIPHER_NONE:
 case VIR_CRYPTO_CIPHER_LAST:
 break;
 }
+# undef DO_CRYPT
+
 
 virReportError(VIR_ERR_INVALID_ARG,
_("algorithm=%d is not supported"), algorithm);
-- 
2.5.5

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

[libvirt] [PATCH REPOST 0/4] Couple of random and crypto adjustments

[John Ferlan]

Originally posted :

http://www.redhat.com/archives/libvir-list/2016-May/msg01650.html

Updated to current head and reposted.

John Ferlan (4):
  util: Add range parameters to virRandomBytes
  storage: Use virRandomBytes for virStorageGenerateQcowPassphrase
  util: Alter virCryptoEncryptData for non GNUTLS builds
  util: Adjust virCryptoEncryptData code to use macros

 src/storage/storage_backend.c   | 10 +++--
 src/util/vircrypto.c| 88 +
 src/util/virrandom.c| 13 --
 src/util/virrandom.h|  3 +-
 src/util/virstorageencryption.c | 42 +++-
 src/util/virstorageencryption.h |  4 +-
 src/util/viruuid.c  |  2 +-
 tests/qemuxml2argvmock.c|  2 +-
 tests/vircryptotest.c   |  4 +-
 tests/virrandommock.c   | 10 +++--
 tests/virrandomtest.c   | 32 +++
 11 files changed, 124 insertions(+), 86 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH REPOST 3/4] util: Alter virCryptoEncryptData for non GNUTLS builds

[John Ferlan]

Rather than intermixing the ATTRIBUTE_UNUSED - use HAVE_GNUTLS_CIPHER_ENCRYPT
for the whole function instead.

Signed-off-by: John Ferlan 
---
 src/util/vircrypto.c | 36 +---
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 4125230..27a3d1d 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -200,7 +200,6 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t 
gnutls_enc_alg,
 memset(&iv_buf, 0, sizeof(gnutls_datum_t));
 return -1;
 }
-#endif
 
 
 /* virCryptoEncryptData:
@@ -221,14 +220,14 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t 
gnutls_enc_alg,
  */
 int
 virCryptoEncryptData(virCryptoCipher algorithm,
- uint8_t *enckey ATTRIBUTE_UNUSED,
+ uint8_t *enckey,
  size_t enckeylen,
- uint8_t *iv ATTRIBUTE_UNUSED,
+ uint8_t *iv,
  size_t ivlen,
- uint8_t *data ATTRIBUTE_UNUSED,
- size_t datalen ATTRIBUTE_UNUSED,
- uint8_t **ciphertext ATTRIBUTE_UNUSED,
- size_t *ciphertextlen ATTRIBUTE_UNUSED)
+ uint8_t *data,
+ size_t datalen,
+ uint8_t **ciphertext,
+ size_t *ciphertextlen)
 {
 switch (algorithm) {
 case VIR_CRYPTO_CIPHER_AES256CBC:
@@ -246,7 +245,6 @@ virCryptoEncryptData(virCryptoCipher algorithm,
 return -1;
 }
 
-#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
 /*
  * Encrypt the data buffer using an encryption key and
  * initialization vector via the gnutls_cipher_encrypt API
@@ -256,9 +254,6 @@ virCryptoEncryptData(virCryptoCipher algorithm,
  enckey, enckeylen, iv, ivlen,
  data, datalen,
  ciphertext, ciphertextlen);
-#else
-break;
-#endif
 
 case VIR_CRYPTO_CIPHER_NONE:
 case VIR_CRYPTO_CIPHER_LAST:
@@ -270,6 +265,25 @@ virCryptoEncryptData(virCryptoCipher algorithm,
 return -1;
 }
 
+#else
+
+int
+virCryptoEncryptData(virCryptoCipher algorithm,
+ uint8_t *enckey ATTRIBUTE_UNUSED,
+ size_t enckeylen ATTRIBUTE_UNUSED,
+ uint8_t *iv ATTRIBUTE_UNUSED,
+ size_t ivlen ATTRIBUTE_UNUSED,
+ uint8_t *data ATTRIBUTE_UNUSED,
+ size_t datalen ATTRIBUTE_UNUSED,
+ uint8_t **ciphertext ATTRIBUTE_UNUSED,
+ size_t *ciphertextlen ATTRIBUTE_UNUSED)
+{
+virReportError(VIR_ERR_INVALID_ARG,
+   _("algorithm=%d is not supported"), algorithm);
+return -1;
+}
+#endif
+
 /* virCryptoGenerateRandom:
  * @nbytes: Size in bytes of random byte stream to generate
  *
-- 
2.5.5

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

[libvirt] [PATCH v2 0/3] Adjust qemu-img switches and checks

[John Ferlan]

v1:http://www.redhat.com/archives/libvir-list/2016-June/msg00081.html
(patches 4, 6, and 7)

Patch 4 is now patch 1
Patch 6 was reviewed, but this patch alters it not need backingType
Patch 7 reworked to accommodate patch 1 changes

John Ferlan (3):
  storage: Adjust qemu-img switches check
  storage: Create helper to set backing for CreateQemuImg code
  storage: Create helper to set options for CreateQemuImg code

 src/storage/storage_backend.c  | 223 ++---
 tests/storagevolxml2argvdata/qcow2-flag.argv   |   2 -
 .../qcow2-nobacking-convert-flag.argv  |   2 -
 .../qcow2-nobacking-convert-none.argv  |   2 -
 .../qcow2-nobacking-flag.argv  |   1 -
 .../qcow2-nobacking-none.argv  |   1 -
 tests/storagevolxml2argvdata/qcow2-none.argv   |   1 -
 tests/storagevolxml2argvtest.c |  22 +-
 8 files changed, 107 insertions(+), 147 deletions(-)
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-flag.argv
 delete mode 100644 
tests/storagevolxml2argvdata/qcow2-nobacking-convert-flag.argv
 delete mode 100644 
tests/storagevolxml2argvdata/qcow2-nobacking-convert-none.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-flag.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-none.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-none.argv

-- 
2.5.5

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

[libvirt] [PATCH v2 1/3] storage: Adjust qemu-img switches check

[John Ferlan]

Since we support QEMU 0.12 and later, checking for support of specific flags
added prior to that isn't necessary.

Thus start with the base of having the "-o options" available for the
qemu-img create option and then determine whether we have the compat
option for qcow2 files (which would be necessary up through qemu 2.0
where the default changes to compat 0.11).

Adjust test to no long check for NONE and FLAG options as well was removing
results of tests that would use that option.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c  | 91 +++---
 tests/storagevolxml2argvdata/qcow2-flag.argv   |  2 -
 .../qcow2-nobacking-convert-flag.argv  |  2 -
 .../qcow2-nobacking-convert-none.argv  |  2 -
 .../qcow2-nobacking-flag.argv  |  1 -
 .../qcow2-nobacking-none.argv  |  1 -
 tests/storagevolxml2argvdata/qcow2-none.argv   |  1 -
 tests/storagevolxml2argvtest.c | 22 +-
 8 files changed, 29 insertions(+), 93 deletions(-)
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-flag.argv
 delete mode 100644 
tests/storagevolxml2argvdata/qcow2-nobacking-convert-flag.argv
 delete mode 100644 
tests/storagevolxml2argvdata/qcow2-nobacking-convert-none.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-flag.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-nobacking-none.argv
 delete mode 100644 tests/storagevolxml2argvdata/qcow2-none.argv

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 8f03a6e..ff3f8c1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -869,10 +869,15 @@ virStoragePloopResize(virStorageVolDefPtr vol,
 return ret;
 }
 
+/* Flag values shared w/ storagevolxml2argvtest.c.
+ *
+ * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11)
+ * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT
+ *was made necessary due to 2.0 change to change the default
+ *qcow2 file format from 0.10 to 1.1.
+ */
 enum {
-QEMU_IMG_BACKING_FORMAT_NONE = 0,
-QEMU_IMG_BACKING_FORMAT_FLAG,
-QEMU_IMG_BACKING_FORMAT_OPTIONS,
+QEMU_IMG_BACKING_FORMAT_OPTIONS = 0,
 QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
 };
 
@@ -904,46 +909,18 @@ virStorageBackendQemuImgSupportsCompat(const char 
*qemuimg)
 static int
 virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
 {
-char *help = NULL;
-char *start;
-char *end;
-char *tmp;
-int ret = -1;
-int exitstatus;
-virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL);
-
-virCommandAddEnvString(cmd, "LC_ALL=C");
-virCommandSetOutputBuffer(cmd, &help);
-virCommandClearCaps(cmd);
-
-/* qemuimg doesn't return zero exit status on -h,
- * therefore we need to provide pointer for storing
- * exit status, although we don't parse it any later */
-if (virCommandRun(cmd, &exitstatus) < 0)
-goto cleanup;
-
-if ((start = strstr(help, " create ")) == NULL ||
-(end = strstr(start, "\n")) == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unable to parse qemu-img output '%s'"),
-   help);
-goto cleanup;
-}
-if (((tmp = strstr(start, "-F fmt")) && tmp < end) ||
-((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) {
-ret = QEMU_IMG_BACKING_FORMAT_FLAG;
-} else if ((tmp = strstr(start, "[-o options]")) && tmp < end) {
-if (virStorageBackendQemuImgSupportsCompat(qemuimg))
-ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
-else
-ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
-} else {
-ret = QEMU_IMG_BACKING_FORMAT_NONE;
-}
+/* As of QEMU 0.11 the [-o options] support was added via qemu
+ * commit id '9ea2ea71', so we start with that base and figure
+ * out what else we have */
+int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
+
+/* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
+ * understands. Since we still support QEMU 0.12 and newer, we need
+ * to be able to handle the previous format as can be set via a
+ * compat=0.10 option. */
+if (virStorageBackendQemuImgSupportsCompat(qemuimg))
+ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
 
- cleanup:
-virCommandFree(cmd);
-VIR_FREE(help);
 return ret;
 }
 
@@ -1218,29 +1195,17 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
conn,
 if (info.backingPath)
 virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
 
-if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) {
-if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
-imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
-info.compat = "0.10";
+if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
+imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
+info.compat = "0.10";
 
-if (virS

[libvirt] [PATCH v2 3/3] storage: Create helper to set options for CreateQemuImg code

[John Ferlan]

Create a helper virStorageBackendCreateQemuImgSetOptions to set either
the qemu-img -o options or the previous mechanism using -F

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c | 30 +-
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 41c7bda..62791b1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1119,6 +1119,26 @@ 
virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
 }
 
 
+static int
+virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
+ int imgformat,
+ struct _virStorageBackendQemuImgInfo 
info)
+{
+char *opts = NULL;
+
+if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
+imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
+info.compat = "0.10";
+
+if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0)
+return -1;
+if (opts)
+virCommandAddArgList(cmd, "-o", opts, NULL);
+VIR_FREE(opts);
+
+return 0;
+}
+
 
 /* Create a qemu-img virCommand from the supplied binary path,
  * volume definitions and imgformat
@@ -1134,7 +1154,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
conn,
 {
 virCommandPtr cmd = NULL;
 const char *type;
-char *opts = NULL;
 struct _virStorageBackendQemuImgInfo info = {
 .format = vol->target.format,
 .path = vol->target.path,
@@ -1207,17 +1226,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
conn,
 if (info.backingPath)
 virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
 
-if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
-imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
-info.compat = "0.10";
-
-if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) {
+if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, info) < 0) {
 virCommandFree(cmd);
 return NULL;
 }
-if (opts)
-virCommandAddArgList(cmd, "-o", opts, NULL);
-VIR_FREE(opts);
 
 if (info.inputPath)
 virCommandAddArg(cmd, info.inputPath);
-- 
2.5.5

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

[libvirt] [PATCH v2 2/3] storage: Create helper to set backing for CreateQemuImg code

[John Ferlan]

Create a helper virStorageBackendCreateQemuImgSetBacking to perform the
backing store set

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c | 116 +++---
 1 file changed, 64 insertions(+), 52 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index ff3f8c1..41c7bda 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1060,6 +1060,65 @@ 
virStorageBackendCreateQemuImgSetInput(virStorageVolDefPtr inputvol,
 }
 
 
+static int
+virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
+ virStorageVolDefPtr vol,
+ virStorageVolDefPtr inputvol,
+ struct _virStorageBackendQemuImgInfo 
*info)
+{
+int accessRetCode = -1;
+char *absolutePath = NULL;
+
+info->backingFormat = vol->target.backingStore->format;
+info->backingPath = vol->target.backingStore->path;
+
+if (info->preallocate) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("metadata preallocation conflicts with backing"
+ " store"));
+return -1;
+}
+
+/* XXX: Not strictly required: qemu-img has an option a different
+ * backing store, not really sure what use it serves though, and it
+ * may cause issues with lvm. Untested essentially.
+ */
+if (inputvol && inputvol->target.backingStore &&
+STRNEQ_NULLABLE(inputvol->target.backingStore->path,
+info->backingPath)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("a different backing store cannot be specified."));
+return -1;
+}
+
+if (!virStorageFileFormatTypeToString(info->backingFormat)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unknown storage vol backing store type %d"),
+   info->backingFormat);
+return -1;
+}
+
+/* Convert relative backing store paths to absolute paths for access
+ * validation.
+ */
+if ('/' != *(info->backingPath) &&
+virAsprintf(&absolutePath, "%s/%s", pool->def->target.path,
+info->backingPath) < 0)
+return -1;
+accessRetCode = access(absolutePath ? absolutePath :
+   info->backingPath, R_OK);
+VIR_FREE(absolutePath);
+if (accessRetCode != 0) {
+virReportSystemError(errno,
+ _("inaccessible backing store volume %s"),
+ info->backingPath);
+return -1;
+}
+
+return 0;
+}
+
+
 
 /* Create a qemu-img virCommand from the supplied binary path,
  * volume definitions and imgformat
@@ -1075,7 +1134,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
conn,
 {
 virCommandPtr cmd = NULL;
 const char *type;
-const char *backingType = NULL;
 char *opts = NULL;
 struct _virStorageBackendQemuImgInfo info = {
 .format = vol->target.format,
@@ -1120,54 +1178,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
conn,
 virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0)
 return NULL;
 
-if (vol->target.backingStore) {
-int accessRetCode = -1;
-char *absolutePath = NULL;
-
-info.backingFormat = vol->target.backingStore->format;
-info.backingPath = vol->target.backingStore->path;
-
-if (info.preallocate) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("metadata preallocation conflicts with backing"
- " store"));
-return NULL;
-}
-
-/* XXX: Not strictly required: qemu-img has an option a different
- * backing store, not really sure what use it serves though, and it
- * may cause issues with lvm. Untested essentially.
- */
-if (inputvol && inputvol->target.backingStore &&
-STRNEQ_NULLABLE(inputvol->target.backingStore->path, 
info.backingPath)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("a different backing store cannot be 
specified."));
-return NULL;
-}
-
-if (!(backingType = 
virStorageFileFormatTypeToString(info.backingFormat))) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unknown storage vol backing store type %d"),
-   info.backingFormat);
-return NULL;
-}
-
-/* Convert relative backing store paths to absolute paths for access
- * validation.
- */
-if ('/' != *(info.backingPath) &&
-virAsprintf(&absolutePath, "%s/%s", pool->def->target.path,
-info.backingPath) < 0)
-return NULL;
-accessRetCode = access(absolutePath ? absolutePath : info

Re: [libvirt] [PATCH 0/7] Cleanup with storageencryption/qemu-img

[John Ferlan]

On 06/03/2016 06:42 AM, John Ferlan wrote:
> The first 4 patches were already posted:
> 
>   http://www.redhat.com/archives/libvir-list/2016-May/msg01984.htm
> 
>   http://www.redhat.com/archives/libvir-list/2016-May/msg02147.html
> 
> There was only one small change in a comment to any of those patches.
> 
> The last 3 patches are new - just trying to make it easier to use the
> qemu-img code for creating a luks volume.
> 
> John Ferlan (7):
>   util: Clean up code formatting in virstorageencryption
>   storage: Split out setting default secret for encryption
>   storage: Split out a helper for encryption checks
>   storage: Adjust qemu-img switches check
>   storage: Create helper to set input for CreateQemuImg code
>   storage: Create helper to set backing for CreateQemuImg code
>   storage: Create helper to set options for CreateQemuImg code
> 
>  src/storage/storage_backend.c| 367 
> ++-
>  src/storage/storage_backend_fs.c |  79 +
>  src/util/virstorageencryption.c  |  58 +++
>  3 files changed, 282 insertions(+), 222 deletions(-)
> 

I have pushed patches 1, 2, 3, and an adjusted 5.

I'll respin 4 with test removals and 7 with appropriate adjustments.

Tks -

John

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

Re: [libvirt] [PATCH 6/7] storage: Create helper to set backing for CreateQemuImg code

[John Ferlan]

On 06/06/2016 09:49 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:42:11 -0400, John Ferlan wrote:
>> Create a helper virStorageBackendCreateQemuImgSetBacking to perform the
>> backing store set
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/storage/storage_backend.c | 118 
>> --
>>  1 file changed, 67 insertions(+), 51 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index 4a3c41d..624790f 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -940,6 +940,7 @@ struct _virStorageBackendQemuImgInfo {
>>  bool nocow;
>>  
>>  const char *backingPath;
>> +const char *backingType;
> 
> backingFormatStr.
> 
>>  int backingFormat;
>>  
>>  const char *inputPath;
> 
> ACK with the adjustment
> 

Given that patch 4 will change to remove the else condition, this patch
won't be necessary

John

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

Re: [libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Daniel P. Berrange]

On Mon, Jun 06, 2016 at 04:46:50PM +0200, Peter Krempa wrote:
> On Mon, Jun 06, 2016 at 15:16:50 +0100, Daniel Berrange wrote:
> > The sd_notify method is used to tell systemd when libvirtd
> > has finished starting up. All it does is send a datagram
> > containing the string parameter to systemd on a UNIX socket
> > named in the NOTIFY_SOCKET environment variable. Rather than
> > pulling in the systemd libraries for this, just code the
> > notification directly in libvirt as this is a stable ABI
> > from systemd's POV which explicitly allows independant
> > implementations:
> > 
> > See "Reimplementable Independently" column in the
> > "$NOTIFY_SOCKET Daemon Notifications" row:
> > 
> > https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  configure.ac  |  2 --
> >  libvirt.spec.in   | 12 ---
> >  m4/virt-systemd-daemon.m4 | 34 ---
> >  src/Makefile.am   |  4 ++--
> >  src/util/virsystemd.c | 52 
> > ++-
> >  5 files changed, 49 insertions(+), 55 deletions(-)
> >  delete mode 100644 m4/virt-systemd-daemon.m4
> 
> [...]
> 
> > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> > index 4883f94..71b8f33 100644
> > --- a/src/util/virsystemd.c
> > +++ b/src/util/virsystemd.c
> 
> [...]
> 
> > @@ -480,9 +482,49 @@ int virSystemdTerminateMachine(const char *name)
> >  void
> >  virSystemdNotifyStartup(void)
> >  {
> > -#ifdef WITH_SYSTEMD_DAEMON
> > -sd_notify(0, "READY=1");
> > -#endif
> > +#ifdef HAVE_SYS_UN_H
> > +const char *path;
> > +const char *msg = "READY=1";
> > +int fd;
> > +struct sockaddr_un un = {
> > +.sun_family = AF_UNIX,
> > +};
> > +struct iovec iov = {
> > +.iov_base = (char *)msg,
> > +.iov_len = strlen(msg),
> > +};
> > +struct msghdr mh = {
> > +.msg_name = &un,
> > +.msg_namelen = sizeof(un),
> > +.msg_iov = &iov,
> > +.msg_iovlen = 1,
> > +};
> > +
> > +if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
> > +VIR_DEBUG("Skipping systemd notify, not requested");
> > +return;
> > +}
> > +
> > +if (strlen(path) > sizeof(un.sun_path)) {
> 
> Off-by-one by not considering the trailing \0

UNIX socket addresses are *not* NUL-terminated strings. The full
108 bytes in the 'sun_path' field are considered to be the path.
So even if you do have a NUL in your string, everything following
it is also considered part of the address - its why its important
to ensure the sun_path is set to all-zeros :-)

> > +VIR_WARN("Systemd notify socket path '%s' too long", path);
> > +return;
> > +}
> 
> ACK

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

[libvirt] [PATCH v2] systemd: directly notify systemd instead of using sd_notify

[Daniel P. Berrange]

The sd_notify method is used to tell systemd when libvirtd
has finished starting up. All it does is send a datagram
containing the string parameter to systemd on a UNIX socket
named in the NOTIFY_SOCKET environment variable. Rather than
pulling in the systemd libraries for this, just code the
notification directly in libvirt as this is a stable ABI
from systemd's POV which explicitly allows independant
implementations:

See "Reimplementable Independently" column in the
"$NOTIFY_SOCKET Daemon Notifications" row:

https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/

Signed-off-by: Daniel P. Berrange 
---
 configure.ac  |  2 --
 libvirt.spec.in   | 12 ---
 m4/virt-systemd-daemon.m4 | 34 --
 src/Makefile.am   |  4 ++--
 src/util/virsystemd.c | 53 ++-
 5 files changed, 50 insertions(+), 55 deletions(-)
 delete mode 100644 m4/virt-systemd-daemon.m4

diff --git a/configure.ac b/configure.ac
index 74c33b3..a63b912 100644
--- a/configure.ac
+++ b/configure.ac
@@ -256,7 +256,6 @@ LIBVIRT_CHECK_SANLOCK
 LIBVIRT_CHECK_SASL
 LIBVIRT_CHECK_SELINUX
 LIBVIRT_CHECK_SSH2
-LIBVIRT_CHECK_SYSTEMD_DAEMON
 LIBVIRT_CHECK_UDEV
 LIBVIRT_CHECK_WIRESHARK
 LIBVIRT_CHECK_NSS
@@ -2787,7 +2786,6 @@ LIBVIRT_RESULT_SANLOCK
 LIBVIRT_RESULT_SASL
 LIBVIRT_RESULT_SELINUX
 LIBVIRT_RESULT_SSH2
-LIBVIRT_RESULT_SYSTEMD_DAEMON
 LIBVIRT_RESULT_UDEV
 LIBVIRT_RESULT_WIRESHARK
 LIBVIRT_RESULT_NSS
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 8b88eef..b93a53c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -79,7 +79,6 @@
 %define with_firewalld 0%{!?_without_firewalld:0}
 %define with_libssh2   0%{!?_without_libssh2:0}
 %define with_wireshark 0%{!?_without_wireshark:0}
-%define with_systemd_daemon 0%{!?_without_systemd_daemon:0}
 %define with_pm_utils  1
 
 # Finally set the OS / architecture specific special cases
@@ -133,7 +132,6 @@
 # Fedora has systemd, libvirt still used sysvinit there.
 %if 0%{?fedora} || 0%{?rhel} >= 7
 %define with_systemd 1
-%define with_systemd_daemon 1
 %define with_pm_utils 0
 %endif
 
@@ -268,9 +266,6 @@ BuildRequires: python
 %if %{with_systemd}
 BuildRequires: systemd-units
 %endif
-%if %{with_systemd_daemon}
-BuildRequires: systemd-devel
-%endif
 %if %{with_xen} || %{with_libxl}
 BuildRequires: xen-devel
 %endif
@@ -1061,12 +1056,6 @@ rm -rf .git
 %define arg_wireshark --without-wireshark-dissector
 %endif
 
-%if %{with_systemd_daemon}
-%define arg_systemd_daemon --with-systemd-daemon
-%else
-%define arg_systemd_daemon --without-systemd-daemon
-%endif
-
 %if %{with_pm_utils}
 %define arg_pm_utils --with-pm-utils
 %else
@@ -1157,7 +1146,6 @@ rm -f po/stamp-po
--with-driver-modules \
%{?arg_firewalld} \
%{?arg_wireshark} \
-   %{?arg_systemd_daemon} \
%{?arg_pm_utils} \
--with-nss-plugin \
%{arg_packager} \
diff --git a/m4/virt-systemd-daemon.m4 b/m4/virt-systemd-daemon.m4
deleted file mode 100644
index 8516e41..000
--- a/m4/virt-systemd-daemon.m4
+++ /dev/null
@@ -1,34 +0,0 @@
-dnl The libsystemd-daemon.so library
-dnl
-dnl Copyright (C) 2012-2013 Red Hat, Inc.
-dnl
-dnl This library is free software; you can redistribute it and/or
-dnl modify it under the terms of the GNU Lesser General Public
-dnl License as published by the Free Software Foundation; either
-dnl version 2.1 of the License, or (at your option) any later version.
-dnl
-dnl This library is distributed in the hope that it will be useful,
-dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
-dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-dnl Lesser General Public License for more details.
-dnl
-dnl You should have received a copy of the GNU Lesser General Public
-dnl License along with this library.  If not, see
-dnl .
-dnl
-
-AC_DEFUN([LIBVIRT_CHECK_SYSTEMD_DAEMON],[
-  LIBVIRT_CHECK_PKG([SYSTEMD_DAEMON], [libsystemd-daemon], [0.27.1])
-
-old_CFLAGS="$CFLAGS"
-old_LIBS="$LIBS"
-CFLAGS="$CFLAGS $SYSTEMD_DAEMON_CFLAGS"
-LIBS="$LIBS $SYSTEMD_DAEMON_LIBS"
-AC_CHECK_FUNCS([sd_notify])
-CFLAGS="$old_CFLAGS"
-LIBS="$old_LIBS"
-])
-
-AC_DEFUN([LIBVIRT_RESULT_SYSTEMD_DAEMON],[
-  LIBVIRT_RESULT_LIB([SYSTEMD_DAEMON])
-])
diff --git a/src/Makefile.am b/src/Makefile.am
index 12b66c2..11c79df 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1106,12 +1106,12 @@ libvirt_util_la_SOURCES =   
\
 libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \
$(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \
$(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS)  \
-   $(SYSTEMD_DAEMON_CFLAGS) $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \
+   $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \
-I$(srcdir)/

[libvirt] [PATCH 4/9] rpc: allow priority string to be passed to TLS context

[Daniel P. Berrange]

Extend the virNetTLSContextNew* constructors to allow
the TLS priority string to be passed in, overriding the
compile time default.

Signed-off-by: Daniel P. Berrange 
---
 daemon/libvirtd.c|  2 ++
 src/remote/remote_driver.c   |  1 +
 src/rpc/virnettlscontext.c   | 27 ---
 src/rpc/virnettlscontext.h   |  4 
 tests/virnettlscontexttest.c |  2 ++
 tests/virnettlssessiontest.c |  2 ++
 6 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 5617e42..b844af4 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -585,6 +585,7 @@ daemonSetupNetworking(virNetServerPtr srv,
config->cert_file,
config->key_file,
(const char 
*const*)config->tls_allowed_dn_list,
+   NULL,

config->tls_no_sanity_certificate ? false : true,

config->tls_no_verify_certificate ? false : true)))
 goto cleanup;
@@ -592,6 +593,7 @@ daemonSetupNetworking(virNetServerPtr srv,
 if (!(ctxt = virNetTLSContextNewServerPath(NULL,
!privileged,
(const char 
*const*)config->tls_allowed_dn_list,
+   NULL,

config->tls_no_sanity_certificate ? false : true,

config->tls_no_verify_certificate ? false : true)))
 goto cleanup;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index e3cf5fb..219cf47 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -845,6 +845,7 @@ doRemoteOpen(virConnectPtr conn,
 #ifdef WITH_GNUTLS
 priv->tls = virNetTLSContextNewClientPath(pkipath,
   geteuid() != 0 ? true : 
false,
+  NULL,
   sanity, verify);
 if (!priv->tls)
 goto failed;
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 975b5b8..bc15890 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -65,6 +65,7 @@ struct _virNetTLSContext {
 bool isServer;
 bool requireValidCert;
 const char *const*x509dnWhitelist;
+char *priority;
 };
 
 struct _virNetTLSSession {
@@ -703,6 +704,7 @@ static virNetTLSContextPtr virNetTLSContextNew(const char 
*cacert,
const char *cert,
const char *key,
const char 
*const*x509dnWhitelist,
+   const char *priority,
bool sanityCheckCert,
bool requireValidCert,
bool isServer)
@@ -716,6 +718,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char 
*cacert,
 if (!(ctxt = virObjectLockableNew(virNetTLSContextClass)))
 return NULL;
 
+if (VIR_STRDUP(ctxt->priority, priority) < 0)
+goto error;
+
 err = gnutls_certificate_allocate_credentials(&ctxt->x509cred);
 if (err) {
 virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -903,6 +908,7 @@ static int virNetTLSContextLocateCredentials(const char 
*pkipath,
 static virNetTLSContextPtr virNetTLSContextNewPath(const char *pkipath,
bool tryUserPkiPath,
const char 
*const*x509dnWhitelist,
+   const char *priority,
bool sanityCheckCert,
bool requireValidCert,
bool isServer)
@@ -915,7 +921,7 @@ static virNetTLSContextPtr virNetTLSContextNewPath(const 
char *pkipath,
 return NULL;
 
 ctxt = virNetTLSContextNew(cacert, cacrl, cert, key,
-   x509dnWhitelist, sanityCheckCert,
+   x509dnWhitelist, priority, sanityCheckCert,
requireValidCert, isServer);
 
 VIR_FREE(cacert);
@@ -929,19 +935,21 @@ static virNetTLSContextPtr virNetTLSContextNewPath(const 
char *pkipath,
 virNetTLSContextPtr virNetTLSContextNewServerPath(const char *pkipath,
   bool tryUserPkiPath,
   

[libvirt] [PATCH 9/9] Use @SYSTEM priority for TLS on Fedora >= 21

[Daniel P. Berrange]

In Fedora >= 21, there is a new crypto priority framework
that sets TLS policies globally for all apps. To activate
this with GNUTLS we must request "@SYSTEM" instead of
the traditional "NORMAL" string. The '@' causes gnutls todo
a lookup in its config file for the 'SYSTEM' keyword entry.

Signed-off-by: Daniel P. Berrange 
---
 libvirt.spec.in | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 8b88eef..2d138b0 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -208,6 +208,12 @@
 %define enable_werror --disable-werror
 %endif
 
+%if 0%{?fedora} >= 21
+%define tls_priority "@SYSTEM"
+%else
+%define tls_priority "NORMAL"
+%endif
+
 
 Summary: Library providing a simple virtualization API
 Name: libvirt
@@ -1164,6 +1170,7 @@ rm -f po/stamp-po
%{arg_packager_version} \
--with-qemu-user=%{qemu_user} \
--with-qemu-group=%{qemu_group} \
+  --with-tls-priority=%{tls_priority} \
%{?arg_loader_nvram} \
%{?enable_werror} \
--enable-expensive-tests \
-- 
2.5.5

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

[libvirt] [PATCH 6/9] remote: allow TLS protocol/cipher priority override in URI

[Daniel P. Berrange]

Add support for a "tls_priority" URI parameter in remote
driver URIs. eg

 qemu+tls://localhost/session?tls_priority=NORMAL:-VERS-SSL3.0

Signed-off-by: Daniel P. Berrange 
---
 docs/remote.html.in| 13 +
 src/remote/remote_driver.c |  5 -
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/remote.html.in b/docs/remote.html.in
index 638fdae..9b132f1 100644
--- a/docs/remote.html.in
+++ b/docs/remote.html.in
@@ -230,6 +230,19 @@ Note that parameter values must be
   
   
 
+  tls_priority
+
+ tls 
+
+  A vaid GNUTLS priority string
+
+  
+  
+
+ Example: tls_priority=NORMAL:-VERS-SSL3.0 
+  
+  
+
   command
 
  ssh, ext 
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 219cf47..5f02169 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -652,6 +652,7 @@ doRemoteOpen(virConnectPtr conn,
 #ifndef WIN32
 char *daemonPath = NULL;
 #endif
+char *tls_priority = NULL;
 
 /* We handle *ALL* URIs here. The caller has rejected any
  * URIs we don't care about */
@@ -774,6 +775,7 @@ doRemoteOpen(virConnectPtr conn,
 EXTRACT_URI_ARG_STR("pkipath", pkipath);
 EXTRACT_URI_ARG_STR("known_hosts", knownHosts);
 EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
+EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
 
 EXTRACT_URI_ARG_BOOL("no_sanity", sanity);
 EXTRACT_URI_ARG_BOOL("no_verify", verify);
@@ -845,12 +847,13 @@ doRemoteOpen(virConnectPtr conn,
 #ifdef WITH_GNUTLS
 priv->tls = virNetTLSContextNewClientPath(pkipath,
   geteuid() != 0 ? true : 
false,
-  NULL,
+  tls_priority,
   sanity, verify);
 if (!priv->tls)
 goto failed;
 priv->is_secure = 1;
 #else
+(void)tls_priority;
 (void)sanity;
 (void)verify;
 virReportError(VIR_ERR_INVALID_ARG, "%s",
-- 
2.5.5

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

[libvirt] [PATCH 1/9] tls: remove support for gnutls 1.x.x, require 2.2.0

[Daniel P. Berrange]

We need to use the gnutls_priority_set_direct method which
was not introduced until 2.1.7, so bump version to 2.2.0
which is the first stable release with it included. This
release dates from Dec 2007 so it is reasonable to ditch
support for the 1.x.x series for gnutls releases entirely.

Signed-off-by: Daniel P. Berrange 
---
 configure.ac   |  2 +-
 src/Makefile.am|  1 -
 src/gnutls_1_0_compat.h| 43 ---
 src/rpc/virnettlscontext.c | 11 ---
 tests/virnettlshelpers.h   |  1 -
 5 files changed, 1 insertion(+), 57 deletions(-)
 delete mode 100644 src/gnutls_1_0_compat.h

diff --git a/configure.ac b/configure.ac
index 74c33b3..42eaa82 100644
--- a/configure.ac
+++ b/configure.ac
@@ -117,7 +117,7 @@ fi
 
 dnl Required minimum versions of all libs we depend on
 LIBXML_REQUIRED="2.6.0"
-GNUTLS_REQUIRED="1.0.25"
+GNUTLS_REQUIRED="2.2.0"
 POLKIT_REQUIRED="0.6"
 PARTED_REQUIRED="1.8.0"
 DEVMAPPER_REQUIRED=1.0.0
diff --git a/src/Makefile.am b/src/Makefile.am
index 12b66c2..2ad400d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -436,7 +436,6 @@ remote/qemu_client_bodies.h: $(srcdir)/rpc/gendispatch.pl \
  > $(srcdir)/remote/qemu_client_bodies.h
 
 REMOTE_DRIVER_SOURCES =\
-   gnutls_1_0_compat.h \
remote/remote_driver.c remote/remote_driver.h   \
$(REMOTE_DRIVER_GENERATED)
 
diff --git a/src/gnutls_1_0_compat.h b/src/gnutls_1_0_compat.h
deleted file mode 100644
index b006e2b..000
--- a/src/gnutls_1_0_compat.h
+++ /dev/null
@@ -1,43 +0,0 @@
-/*
- * gnutls_1_0_compat.h: GnuTLS 1.0 compatibility
- *
- * Copyright (C) 2007, 2013 Red Hat, Inc.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library.  If not, see
- * .
- *
- * Author: Richard W.M. Jones 
- */
-
-#ifndef LIBVIRT_GNUTLS_1_0_COMPAT_H__
-# define LIBVIRT_GNUTLS_1_0_COMPAT_H__
-
-# include 
-
-/* enable backward compatibility macros for gnutls 1.x.y */
-# if LIBGNUTLS_VERSION_MAJOR < 2
-#  define GNUTLS_1_0_COMPAT
-# endif
-
-# ifdef GNUTLS_1_0_COMPAT
-#  define gnutls_session_t gnutls_session
-#  define gnutls_x509_crt_tgnutls_x509_crt
-#  define gnutls_dh_params_t   gnutls_dh_params
-#  define gnutls_transport_ptr_t   gnutls_transport_ptr
-#  define gnutls_datum_t   gnutls_datum
-#  define gnutls_certificate_credentials_t gnutls_certificate_credentials
-#  define gnutls_cipher_algorithm_tgnutls_cipher_algorithm
-# endif
-
-#endif /* LIBVIRT_GNUTLS_1_0_COMPAT_H__ */
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 6e78623..fa9ca41 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -29,7 +29,6 @@
 # include 
 #endif
 #include 
-#include "gnutls_1_0_compat.h"
 
 #include "virnettlscontext.h"
 #include "virstring.h"
@@ -170,7 +169,6 @@ static int virNetTLSContextCheckCertTimes(gnutls_x509_crt_t 
cert,
 }
 
 
-#ifndef GNUTLS_1_0_COMPAT
 /*
  * The gnutls_x509_crt_get_basic_constraints function isn't
  * available in GNUTLS 1.0.x branches. This isn't critical
@@ -219,7 +217,6 @@ static int 
virNetTLSContextCheckCertBasicConstraints(gnutls_x509_crt_t cert,
 
 return 0;
 }
-#endif
 
 
 static int virNetTLSContextCheckCertKeyUsage(gnutls_x509_crt_t cert,
@@ -438,11 +435,9 @@ static int virNetTLSContextCheckCert(gnutls_x509_crt_t 
cert,
isServer, isCA) < 0)
 return -1;
 
-#ifndef GNUTLS_1_0_COMPAT
 if (virNetTLSContextCheckCertBasicConstraints(cert, certFile,
   isServer, isCA) < 0)
 return -1;
-#endif
 
 if (virNetTLSContextCheckCertKeyUsage(cert, certFile,
   isCA) < 0)
@@ -489,10 +484,8 @@ static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t 
cert,
 if (status & GNUTLS_CERT_REVOKED)
 reason = _("The certificate has been revoked.");
 
-#ifndef GNUTLS_1_0_COMPAT
 if (status & GNUTLS_CERT_INSECURE_ALGORITHM)
 reason = _("The certificate uses an insecure algorithm");
-#endif
 
 virReportError(VIR_ERR_SYSTEM_ERROR,
_("Our own certificate %s failed validation against %s: 
%s"),
@@ -1022,10 +1015,8 @@ stati

[libvirt] [PATCH 5/9] libvirtd: add config option for TLS priority

[Daniel P. Berrange]

Add a "tls_priority" config option to /etc/libvirt/libvirtd.conf
to allow the administrator to override the built-in default
setting. This only affects the server side configuration.

Signed-off-by: Daniel P. Berrange 
---
 daemon/libvirtd-config.c| 2 ++
 daemon/libvirtd-config.h| 1 +
 daemon/libvirtd.aug | 1 +
 daemon/libvirtd.c   | 4 ++--
 daemon/libvirtd.conf| 9 -
 daemon/test_libvirtd.aug.in | 1 +
 6 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 45280e9..940bd4b 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -367,6 +367,7 @@ daemonConfigFree(struct daemonConfig *data)
 tmp++;
 }
 VIR_FREE(data->sasl_allowed_username_list);
+VIR_FREE(data->tls_priority);
 
 VIR_FREE(data->key_file);
 VIR_FREE(data->ca_file);
@@ -442,6 +443,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
   &data->sasl_allowed_username_list, filename) 
< 0)
 goto error;
 
+GET_CONF_STR(conf, filename, tls_priority);
 
 GET_CONF_UINT(conf, filename, min_workers);
 GET_CONF_UINT(conf, filename, max_workers);
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index 672e9ad..b9098a8 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -56,6 +56,7 @@ struct daemonConfig {
 int tls_no_sanity_certificate;
 char **tls_allowed_dn_list;
 char **sasl_allowed_username_list;
+char *tls_priority;
 
 char *key_file;
 char *cert_file;
diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index 7a81723..2b8df66 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -53,6 +53,7 @@ module Libvirtd =
| str_array_entry "tls_allowed_dn_list"
| str_array_entry "sasl_allowed_username_list"
| str_array_entry "access_drivers"
+   | str_entry "tls_priority"
 
let processing_entry = int_entry "min_workers"
 | int_entry "max_workers"
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index b844af4..a1e2015 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -585,7 +585,7 @@ daemonSetupNetworking(virNetServerPtr srv,
config->cert_file,
config->key_file,
(const char 
*const*)config->tls_allowed_dn_list,
-   NULL,
+   config->tls_priority,

config->tls_no_sanity_certificate ? false : true,

config->tls_no_verify_certificate ? false : true)))
 goto cleanup;
@@ -593,7 +593,7 @@ daemonSetupNetworking(virNetServerPtr srv,
 if (!(ctxt = virNetTLSContextNewServerPath(NULL,
!privileged,
(const char 
*const*)config->tls_allowed_dn_list,
-   NULL,
+   
config->tls_priority,

config->tls_no_sanity_certificate ? false : true,

config->tls_no_verify_certificate ? false : true)))
 goto cleanup;
diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index 1c1fa7f..3b957e5 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -242,7 +242,7 @@
 #tls_allowed_dn_list = ["DN1", "DN2"]
 
 
-# A whitelist of allowed SASL usernames. The format for usernames
+# A whitelist of allowed SASL usernames. The format for username
 # depends on the SASL authentication mechanism. Kerberos usernames
 # look like username@REALM
 #
@@ -259,6 +259,13 @@
 #sasl_allowed_username_list = ["j...@example.com", "f...@example.com" ]
 
 
+# Override the compile time default TLS priority string. The
+# default is usually "NORMAL" unless overridden at build time.
+# Only set this is it is desired for libvirt to deviate from
+# the global default settings.
+#
+#tls_priority="NORMAL"
+
 
 #
 #
diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in
index 7a03603..1fb182c 100644
--- a/daemon/test_libvirtd.aug.in
+++ b/daemon/test_libvirtd.aug.in
@@ -35,6 +35,7 @@ module Test_libvirtd =
  { "1" = "j...@example.com" }
  { "2" = "f...@example.com" }
 }
+{ "tls_priority" = "NORMAL" }
 { "max_clients" = "5000" }
 { "max_queued_clients" = "1000" }
 { "max_anonymous_clie

Re: [libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Daniel P. Berrange]

On Mon, Jun 06, 2016 at 04:59:24PM +0200, Peter Krempa wrote:
> On Mon, Jun 06, 2016 at 15:50:46 +0100, Daniel Berrange wrote:
> > On Mon, Jun 06, 2016 at 04:46:50PM +0200, Peter Krempa wrote:
> > > On Mon, Jun 06, 2016 at 15:16:50 +0100, Daniel Berrange wrote:
> > > > The sd_notify method is used to tell systemd when libvirtd
> > > > has finished starting up. All it does is send a datagram
> > > > containing the string parameter to systemd on a UNIX socket
> > > > named in the NOTIFY_SOCKET environment variable. Rather than
> > > > pulling in the systemd libraries for this, just code the
> > > > notification directly in libvirt as this is a stable ABI
> > > > from systemd's POV which explicitly allows independant
> > > > implementations:
> > > > 
> > > > See "Reimplementable Independently" column in the
> > > > "$NOTIFY_SOCKET Daemon Notifications" row:
> > > > 
> > > > https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> > > > 
> > > > Signed-off-by: Daniel P. Berrange 
> > > > ---
> > > >  configure.ac  |  2 --
> > > >  libvirt.spec.in   | 12 ---
> > > >  m4/virt-systemd-daemon.m4 | 34 ---
> > > >  src/Makefile.am   |  4 ++--
> > > >  src/util/virsystemd.c | 52 
> > > > ++-
> > > >  5 files changed, 49 insertions(+), 55 deletions(-)
> > > >  delete mode 100644 m4/virt-systemd-daemon.m4
> > > 
> > > [...]
> > > 
> > > > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> > > > index 4883f94..71b8f33 100644
> > > > --- a/src/util/virsystemd.c
> > > > +++ b/src/util/virsystemd.c
> > > 
> > > [...]
> > > 
> > > > @@ -480,9 +482,49 @@ int virSystemdTerminateMachine(const char *name)
> > > >  void
> > > >  virSystemdNotifyStartup(void)
> > > >  {
> > > > -#ifdef WITH_SYSTEMD_DAEMON
> > > > -sd_notify(0, "READY=1");
> > > > -#endif
> > > > +#ifdef HAVE_SYS_UN_H
> > > > +const char *path;
> > > > +const char *msg = "READY=1";
> > > > +int fd;
> > > > +struct sockaddr_un un = {
> > > > +.sun_family = AF_UNIX,
> > > > +};
> > > > +struct iovec iov = {
> > > > +.iov_base = (char *)msg,
> > > > +.iov_len = strlen(msg),
> > > > +};
> > > > +struct msghdr mh = {
> > > > +.msg_name = &un,
> > > > +.msg_namelen = sizeof(un),
> > > > +.msg_iov = &iov,
> > > > +.msg_iovlen = 1,
> > > > +};
> > > > +
> > > > +if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
> > > > +VIR_DEBUG("Skipping systemd notify, not requested");
> > > > +return;
> > > > +}
> > > > +
> > > > +if (strlen(path) > sizeof(un.sun_path)) {
> > > 
> > > Off-by-one by not considering the trailing \0
> > 
> > UNIX socket addresses are *not* NUL-terminated strings. The full
> > 108 bytes in the 'sun_path' field are considered to be the path.
> > So even if you do have a NUL in your string, everything following
> > it is also considered part of the address - its why its important
> > to ensure the sun_path is set to all-zeros :-)
> 
> Ah. I learned something at least.
> 
> But then there's still a problem as virStrcpy doesn't know about this
> unix socket quirk and it subtracts '1' from the third argument before
> comparing it with the length of the second argument and won't copy
> anything in the corner case if the path has exactly 108 bytes.

Ok, I'll repost using memcpy() to make it clearer that we're really
not trying to create a NUL-terminated string.


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

[libvirt] [PATCH 2/9] rpc: set gnutls log function at global init time

[Daniel P. Berrange]

Currently we set the gnutls log function when creating a
TLS context, however, the setting is in fact global, not
per context. So we should be setting it when we first call
gnutls_global_init() instead.

Signed-off-by: Daniel P. Berrange 
---
 src/rpc/virnettlscontext.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index fa9ca41..425f7ff 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -708,7 +708,6 @@ static virNetTLSContextPtr virNetTLSContextNew(const char 
*cacert,
bool isServer)
 {
 virNetTLSContextPtr ctxt;
-const char *gnutlsdebug;
 int err;
 
 if (virNetTLSContextInitialize() < 0)
@@ -717,16 +716,6 @@ static virNetTLSContextPtr virNetTLSContextNew(const char 
*cacert,
 if (!(ctxt = virObjectLockableNew(virNetTLSContextClass)))
 return NULL;
 
-if ((gnutlsdebug = virGetEnvAllowSUID("LIBVIRT_GNUTLS_DEBUG")) != NULL) {
-int val;
-if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0)
-val = 10;
-gnutls_global_set_log_level(val);
-gnutls_global_set_log_function(virNetTLSLog);
-VIR_DEBUG("Enabled GNUTLS debug");
-}
-
-
 err = gnutls_certificate_allocate_credentials(&ctxt->x509cred);
 if (err) {
 virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -1440,5 +1429,15 @@ void virNetTLSSessionDispose(void *obj)
  */
 void virNetTLSInit(void)
 {
+const char *gnutlsdebug;
+if ((gnutlsdebug = virGetEnvAllowSUID("LIBVIRT_GNUTLS_DEBUG")) != NULL) {
+int val;
+if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0)
+val = 10;
+gnutls_global_set_log_level(val);
+gnutls_global_set_log_function(virNetTLSLog);
+VIR_DEBUG("Enabled GNUTLS debug");
+}
+
 gnutls_global_init();
 }
-- 
2.5.5

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

[libvirt] [PATCH 3/9] configure: allow setting default TLS priority string

[Daniel P. Berrange]

Currently libvirt calls gnutls_set_default_priority()
which on old systems resolves to "NORMAL" while new
systems it resolves to "@SYSTEM". Either way, this
is a global default that is identical across all apps.

We want to allow distros to flexibility to define a
custom default string for libvirt priority, so add
a --tls-priority=STRING  flag to configure to enable
this to be set.

It is expected that distros would use this when creating
RPM/Deb/etc packages, according to their preferred crypto
handling policies.

Signed-off-by: Daniel P. Berrange 
---
 configure.ac   | 10 ++
 src/rpc/virnettlscontext.c |  6 +++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 42eaa82..c4fc8be 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1277,6 +1277,16 @@ AC_SUBST([GNUTLS_CFLAGS])
 AC_SUBST([GNUTLS_LIBS])
 
 
+AC_ARG_WITH([tls-priority],
+  [AS_HELP_STRING([--with-tls-priority],
+[set the default TLS session priority string @<:@default=NORMAL@:>@])],
+  [],
+  [with_tls_priority=NORMAL])
+
+AC_DEFINE_UNQUOTED([TLS_PRIORITY], ["$with_tls_priority"],
+  [TLS default priority string])
+
+
 dnl PolicyKit library
 POLKIT_CFLAGS=
 POLKIT_LIBS=
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 425f7ff..975b5b8 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1204,10 +1204,10 @@ virNetTLSSessionPtr 
virNetTLSSessionNew(virNetTLSContextPtr ctxt,
 /* avoid calling all the priority functions, since the defaults
  * are adequate.
  */
-if ((err = gnutls_set_default_priority(sess->session)) != 0) {
+if ((err = gnutls_priority_set_direct(sess->session, TLS_PRIORITY, NULL)) 
!= 0) {
 virReportError(VIR_ERR_SYSTEM_ERROR,
-   _("Failed to set TLS session priority %s"),
-   gnutls_strerror(err));
+   _("Failed to set TLS session priority to %s: %s"),
+   TLS_PRIORITY, gnutls_strerror(err));
 goto error;
 }
 
-- 
2.5.5

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

[libvirt] [PATCH 0/9] Make TLS priority choice configurable

[Daniel P. Berrange]

Historically libvirt has used gnutls_set_default_priority()
to tell GNUTLS to use its standard protocol/cipher config
settings. Since Fedora >= 21, this has caused gnutls to
lookup the conf in /etc/crypto-policies/back-end/gnutls.conf,
while previously it was hardcoded at gnutls build time.

Using the global config is good, but sometimes there might
be a need to have libvirt use a different config than
everything else on the host. eg the global config must
need to be weakened for back-compat usage in non-libvirt
apps. We should allow libvirt to maintain a strong config
despite this.

Ideally gnutls would let us express a preference for multiple
config file settings, and would pick the first one it found.
That would let us request "@LIBVIRT,SYSTEM" to say use the
"LIBVIRT" priority if set, otherwise use the "SYSTEM" priority.
This is proposed in upstream GNUTLS

  http://lists.gnutls.org/pipermail/gnutls-devel/2016-June/008007.html

and if accepted will be the best way to configure things. Until
that feature is accepted though, we should allow a local override
in libvirtd.conf (servers) and libvirt.conf (clients). This series
of patches does that.

NB, we also need to do similar for the QEMU VNC TLS configuration
but that's going to be a followup series.

Daniel P. Berrange (9):
  tls: remove support for gnutls 1.x.x, require 2.2.0
  rpc: set gnutls log function at global init time
  configure: allow setting default TLS priority string
  rpc: allow priority string to be passed to TLS context
  libvirtd: add config option for TLS priority
  remote: allow TLS protocol/cipher priority override in URI
  Pass config file object through to driver open methods
  remote: allow TLS priority to be customized
  Use @SYSTEM priority for TLS on Fedora >= 21

 configure.ac | 12 -
 daemon/libvirtd-config.c |  2 ++
 daemon/libvirtd-config.h |  1 +
 daemon/libvirtd.aug  |  1 +
 daemon/libvirtd.c|  2 ++
 daemon/libvirtd.conf |  9 ++-
 daemon/test_libvirtd.aug.in  |  1 +
 docs/remote.html.in  | 13 ++
 libvirt.spec.in  |  7 ++
 src/Makefile.am  |  1 -
 src/bhyve/bhyve_driver.c |  1 +
 src/driver-hypervisor.h  |  1 +
 src/esx/esx_driver.c |  1 +
 src/gnutls_1_0_compat.h  | 43 
 src/hyperv/hyperv_driver.c   |  4 ++-
 src/libvirt.c|  2 +-
 src/libxl/libxl_driver.c |  1 +
 src/lxc/lxc_driver.c |  1 +
 src/openvz/openvz_driver.c   |  1 +
 src/phyp/phyp_driver.c   |  4 ++-
 src/qemu/qemu_driver.c   |  1 +
 src/remote/remote_driver.c   | 20 ++-
 src/rpc/virnettlscontext.c   | 59 ++--
 src/rpc/virnettlscontext.h   |  4 +++
 src/test/test_driver.c   |  1 +
 src/uml/uml_driver.c |  1 +
 src/vbox/vbox_common.c   |  1 +
 src/vbox/vbox_driver.c   |  1 +
 src/vmware/vmware_driver.c   |  1 +
 src/vz/vz_driver.c   |  1 +
 src/xen/xen_driver.c |  4 ++-
 tests/virnettlscontexttest.c |  2 ++
 tests/virnettlshelpers.h |  1 -
 tests/virnettlssessiontest.c |  2 ++
 34 files changed, 126 insertions(+), 81 deletions(-)
 delete mode 100644 src/gnutls_1_0_compat.h

-- 
2.5.5

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

[libvirt] [PATCH 7/9] Pass config file object through to driver open methods

[Daniel P. Berrange]

The virConnectOpenInternal method opens the libvirt client
config file and uses it to resolve things like URI aliases.

There may be driver specific things that are useful to
store in the config file too, so rather than have them
re-parse the same file, pass the virConfPtr down to the
drivers.

Signed-off-by: Daniel P. Berrange 
---
 src/bhyve/bhyve_driver.c   | 1 +
 src/driver-hypervisor.h| 1 +
 src/esx/esx_driver.c   | 1 +
 src/hyperv/hyperv_driver.c | 4 +++-
 src/libvirt.c  | 2 +-
 src/libxl/libxl_driver.c   | 1 +
 src/lxc/lxc_driver.c   | 1 +
 src/openvz/openvz_driver.c | 1 +
 src/phyp/phyp_driver.c | 4 +++-
 src/qemu/qemu_driver.c | 1 +
 src/remote/remote_driver.c | 1 +
 src/test/test_driver.c | 1 +
 src/uml/uml_driver.c   | 1 +
 src/vbox/vbox_common.c | 1 +
 src/vbox/vbox_driver.c | 1 +
 src/vmware/vmware_driver.c | 1 +
 src/vz/vz_driver.c | 1 +
 src/xen/xen_driver.c   | 4 +++-
 18 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index c4051a1..1885bdf 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -178,6 +178,7 @@ bhyveDomObjFromDomain(virDomainPtr domain)
 static virDrvOpenStatus
 bhyveConnectOpen(virConnectPtr conn,
  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ virConfPtr conf ATTRIBUTE_UNUSED,
  unsigned int flags)
 {
  virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index d11ff7f..5ab5775 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -28,6 +28,7 @@
 typedef virDrvOpenStatus
 (*virDrvConnectOpen)(virConnectPtr conn,
  virConnectAuthPtr auth,
+ virConfPtr conf,
  unsigned int flags);
 
 typedef int
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 031c666..c5f5ed8 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -841,6 +841,7 @@ esxConnectToVCenter(esxPrivate *priv,
  */
 static virDrvOpenStatus
 esxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
+   virConfPtr conf ATTRIBUTE_UNUSED,
unsigned int flags)
 {
 virDrvOpenStatus result = VIR_DRV_OPEN_ERROR;
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index b95c549..9c7faf0 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -58,7 +58,9 @@ hypervFreePrivate(hypervPrivate **priv)
 
 
 static virDrvOpenStatus
-hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int 
flags)
+hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
+  virConfPtr conf ATTRIBUTE_UNUSED,
+  unsigned int flags)
 {
 virDrvOpenStatus result = VIR_DRV_OPEN_ERROR;
 char *plus;
diff --git a/src/libvirt.c b/src/libvirt.c
index 6d9dba8..a5e0e41 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1084,7 +1084,7 @@ virConnectOpenInternal(const char *name,
 ret->secretDriver = virConnectDriverTab[i]->secretDriver;
 ret->storageDriver = virConnectDriverTab[i]->storageDriver;
 
-res = virConnectDriverTab[i]->hypervisorDriver->connectOpen(ret, auth, 
flags);
+res = virConnectDriverTab[i]->hypervisorDriver->connectOpen(ret, auth, 
conf, flags);
 VIR_DEBUG("driver %zu %s returned %s",
   i, virConnectDriverTab[i]->hypervisorDriver->name,
   res == VIR_DRV_OPEN_SUCCESS ? "SUCCESS" :
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 95dfc01..82a05e0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -763,6 +763,7 @@ libxlStateReload(void)
 static virDrvOpenStatus
 libxlConnectOpen(virConnectPtr conn,
  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ virConfPtr conf ATTRIBUTE_UNUSED,
  unsigned int flags)
 {
 virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index f0948ea..ad866f9 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -155,6 +155,7 @@ lxcDomObjFromDomain(virDomainPtr domain)
 
 static virDrvOpenStatus lxcConnectOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+   virConfPtr conf ATTRIBUTE_UNUSED,
unsigned int flags)
 {
 virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index a7474ff..489a607 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1423,6 +1423,7 @@ openvzDomainSetVcpus(virDomainPtr dom, unsigned int 
nvcpus)
 
 static virDrvOpenStatus openvzConnectOpen(virConnectPtr conn,
   virConnectAuthPtr auth 
ATTRIBUTE_UNUSED,
+   

[libvirt] [PATCH 8/9] remote: allow TLS priority to be customized

[Daniel P. Berrange]

Support reading the TLS priority from the client configuration
file via the "tls_priority" config option, eg

 $ cat $HOME/.config/libvirt/libvirt.conf
 tls_priority="NORMAL:-VERS-SSL3.0"

Signed-off-by: Daniel P. Berrange 
---
 src/remote/remote_driver.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b42d1d1..367f46e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -638,6 +638,7 @@ static int
 doRemoteOpen(virConnectPtr conn,
  struct private_data *priv,
  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
+ virConfPtr conf,
  unsigned int flags)
 {
 char *transport_str = NULL;
@@ -844,6 +845,18 @@ doRemoteOpen(virConnectPtr conn,
 /* Connect to the remote service. */
 switch (transport) {
 case trans_tls:
+if (conf && !tls_priority) {
+virConfValuePtr val = virConfGetValue(conf, "tls_priority");
+if (val) {
+if (val->type != VIR_CONF_STRING) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("Config file 'tls_priority' must be a 
string"));
+goto failed;
+}
+tls_priority = val->str;
+}
+}
+
 #ifdef WITH_GNUTLS
 priv->tls = virNetTLSContextNewClientPath(pkipath,
   geteuid() != 0 ? true : 
false,
@@ -1179,7 +1192,7 @@ remoteAllocPrivateData(void)
 static virDrvOpenStatus
 remoteConnectOpen(virConnectPtr conn,
   virConnectAuthPtr auth,
-  virConfPtr conf ATTRIBUTE_UNUSED,
+  virConfPtr conf,
   unsigned int flags)
 {
 struct private_data *priv;
@@ -1238,7 +1251,7 @@ remoteConnectOpen(virConnectPtr conn,
 #endif
 }
 
-ret = doRemoteOpen(conn, priv, auth, rflags);
+ret = doRemoteOpen(conn, priv, auth, conf, rflags);
 if (ret != VIR_DRV_OPEN_SUCCESS) {
 conn->privateData = NULL;
 remoteDriverUnlock(priv);
-- 
2.5.5

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

Re: [libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Peter Krempa]

On Mon, Jun 06, 2016 at 15:50:46 +0100, Daniel Berrange wrote:
> On Mon, Jun 06, 2016 at 04:46:50PM +0200, Peter Krempa wrote:
> > On Mon, Jun 06, 2016 at 15:16:50 +0100, Daniel Berrange wrote:
> > > The sd_notify method is used to tell systemd when libvirtd
> > > has finished starting up. All it does is send a datagram
> > > containing the string parameter to systemd on a UNIX socket
> > > named in the NOTIFY_SOCKET environment variable. Rather than
> > > pulling in the systemd libraries for this, just code the
> > > notification directly in libvirt as this is a stable ABI
> > > from systemd's POV which explicitly allows independant
> > > implementations:
> > > 
> > > See "Reimplementable Independently" column in the
> > > "$NOTIFY_SOCKET Daemon Notifications" row:
> > > 
> > > https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  configure.ac  |  2 --
> > >  libvirt.spec.in   | 12 ---
> > >  m4/virt-systemd-daemon.m4 | 34 ---
> > >  src/Makefile.am   |  4 ++--
> > >  src/util/virsystemd.c | 52 
> > > ++-
> > >  5 files changed, 49 insertions(+), 55 deletions(-)
> > >  delete mode 100644 m4/virt-systemd-daemon.m4
> > 
> > [...]
> > 
> > > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> > > index 4883f94..71b8f33 100644
> > > --- a/src/util/virsystemd.c
> > > +++ b/src/util/virsystemd.c
> > 
> > [...]
> > 
> > > @@ -480,9 +482,49 @@ int virSystemdTerminateMachine(const char *name)
> > >  void
> > >  virSystemdNotifyStartup(void)
> > >  {
> > > -#ifdef WITH_SYSTEMD_DAEMON
> > > -sd_notify(0, "READY=1");
> > > -#endif
> > > +#ifdef HAVE_SYS_UN_H
> > > +const char *path;
> > > +const char *msg = "READY=1";
> > > +int fd;
> > > +struct sockaddr_un un = {
> > > +.sun_family = AF_UNIX,
> > > +};
> > > +struct iovec iov = {
> > > +.iov_base = (char *)msg,
> > > +.iov_len = strlen(msg),
> > > +};
> > > +struct msghdr mh = {
> > > +.msg_name = &un,
> > > +.msg_namelen = sizeof(un),
> > > +.msg_iov = &iov,
> > > +.msg_iovlen = 1,
> > > +};
> > > +
> > > +if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
> > > +VIR_DEBUG("Skipping systemd notify, not requested");
> > > +return;
> > > +}
> > > +
> > > +if (strlen(path) > sizeof(un.sun_path)) {
> > 
> > Off-by-one by not considering the trailing \0
> 
> UNIX socket addresses are *not* NUL-terminated strings. The full
> 108 bytes in the 'sun_path' field are considered to be the path.
> So even if you do have a NUL in your string, everything following
> it is also considered part of the address - its why its important
> to ensure the sun_path is set to all-zeros :-)

Ah. I learned something at least.

But then there's still a problem as virStrcpy doesn't know about this
unix socket quirk and it subtracts '1' from the third argument before
comparing it with the length of the second argument and won't copy
anything in the corner case if the path has exactly 108 bytes.

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

Re: [libvirt] [PATCH 4/7] storage: Adjust qemu-img switches check

[Peter Krempa]

On Mon, Jun 06, 2016 at 09:46:22 -0400, John Ferlan wrote:
> On 06/06/2016 09:37 AM, Peter Krempa wrote:
> > On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
> >> Since we support QEMU 0.12 and later, checking for support of specific 
> >> flags
> >> added prior to that isn't necessary.
> >>
> >> Thus start with the base of having the "-o options" available for the
> >> qemu-img create option and then determine whether we have the compat
> >> option for qcow2 files (which would be necessary up through qemu 2.0
> >> where the default changes to compat 0.11).
> >>
> >> NOTE: Keeping old tests around since it's still possible to create in
> >> the old format.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/storage/storage_backend.c | 66 
> >> ---
> >>  1 file changed, 24 insertions(+), 42 deletions(-)

[...]

> > So ... if the above is never set, why aren't you dropping this entire
> > case?
> > 
> 
> It wasn't clear if tests/storagevolxml2argvtest.c should then be
> removed, adjusted, or otherwise. It replicated the flags I put the X_ in
> front of as:
> 
> enum {
> FMT_NONE = 0,
> FMT_FLAG,
> FMT_OPTIONS,
> FMT_COMPAT,
> };
> 
> 
> Whether we care or not to support/test all those old options was unclear
> to me. I would think at some point in time qemu-img would drop those in

I don't think it makes any sense to test options that we won't ever pass
to qemu-img so we can drop it.

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

Re: [libvirt] [PATCH 1/5] Move struct elem out of virNetDevGetFeatures

[Peter Krempa]

On Mon, Jun 06, 2016 at 16:37:15 +0200, Ján Tomko wrote:
> On Mon, Jun 06, 2016 at 04:25:02PM +0200, Peter Krempa wrote:
> > On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
> > > Rename struct elem to ethtool_to_virnetdev_feature and move it
> > > out of the function to allow reusing it.
> > > ---
> > >  src/util/virnetdev.c | 13 +++--
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> > > index 7db4497..354e6f7 100644
> > > --- a/src/util/virnetdev.c
> > > +++ b/src/util/virnetdev.c
> > > @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void 
> > > *cmd)
> > >  return ret;
> > >  }
> > >  
> > > +struct ethtool_to_virnetdev_feature {
> > 
> > This name is very unorthodox.
> > 
> > ACK to this patch if you choose a more compliant name.
> 
> How about virNetDevEthtoolFeatureCmd?

That sounds good

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

Re: [libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Peter Krempa]

On Mon, Jun 06, 2016 at 15:16:50 +0100, Daniel Berrange wrote:
> The sd_notify method is used to tell systemd when libvirtd
> has finished starting up. All it does is send a datagram
> containing the string parameter to systemd on a UNIX socket
> named in the NOTIFY_SOCKET environment variable. Rather than
> pulling in the systemd libraries for this, just code the
> notification directly in libvirt as this is a stable ABI
> from systemd's POV which explicitly allows independant
> implementations:
> 
> See "Reimplementable Independently" column in the
> "$NOTIFY_SOCKET Daemon Notifications" row:
> 
> https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  configure.ac  |  2 --
>  libvirt.spec.in   | 12 ---
>  m4/virt-systemd-daemon.m4 | 34 ---
>  src/Makefile.am   |  4 ++--
>  src/util/virsystemd.c | 52 
> ++-
>  5 files changed, 49 insertions(+), 55 deletions(-)
>  delete mode 100644 m4/virt-systemd-daemon.m4

[...]

> diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> index 4883f94..71b8f33 100644
> --- a/src/util/virsystemd.c
> +++ b/src/util/virsystemd.c

[...]

> @@ -480,9 +482,49 @@ int virSystemdTerminateMachine(const char *name)
>  void
>  virSystemdNotifyStartup(void)
>  {
> -#ifdef WITH_SYSTEMD_DAEMON
> -sd_notify(0, "READY=1");
> -#endif
> +#ifdef HAVE_SYS_UN_H
> +const char *path;
> +const char *msg = "READY=1";
> +int fd;
> +struct sockaddr_un un = {
> +.sun_family = AF_UNIX,
> +};
> +struct iovec iov = {
> +.iov_base = (char *)msg,
> +.iov_len = strlen(msg),
> +};
> +struct msghdr mh = {
> +.msg_name = &un,
> +.msg_namelen = sizeof(un),
> +.msg_iov = &iov,
> +.msg_iovlen = 1,
> +};
> +
> +if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
> +VIR_DEBUG("Skipping systemd notify, not requested");
> +return;
> +}
> +
> +if (strlen(path) > sizeof(un.sun_path)) {

Off-by-one by not considering the trailing \0

> +VIR_WARN("Systemd notify socket path '%s' too long", path);
> +return;
> +}

ACK

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

Re: [libvirt] [PATCH 1/5] Move struct elem out of virNetDevGetFeatures

[Ján Tomko]

On Mon, Jun 06, 2016 at 04:25:02PM +0200, Peter Krempa wrote:
> On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
> > Rename struct elem to ethtool_to_virnetdev_feature and move it
> > out of the function to allow reusing it.
> > ---
> >  src/util/virnetdev.c | 13 +++--
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> > index 7db4497..354e6f7 100644
> > --- a/src/util/virnetdev.c
> > +++ b/src/util/virnetdev.c
> > @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void 
> > *cmd)
> >  return ret;
> >  }
> >  
> > +struct ethtool_to_virnetdev_feature {
> 
> This name is very unorthodox.
> 
> ACK to this patch if you choose a more compliant name.

How about virNetDevEthtoolFeatureCmd?

Jan

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

Re: [libvirt] [PATCH 1/5] Move struct elem out of virNetDevGetFeatures

[Peter Krempa]

On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
> Rename struct elem to ethtool_to_virnetdev_feature and move it
> out of the function to allow reusing it.
> ---
>  src/util/virnetdev.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 7db4497..354e6f7 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void 
> *cmd)
>  return ret;
>  }
>  
> +struct ethtool_to_virnetdev_feature {

This name is very unorthodox.

ACK to this patch if you choose a more compliant name.

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

[libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Daniel P. Berrange]

The sd_notify method is used to tell systemd when libvirtd
has finished starting up. All it does is send a datagram
containing the string parameter to systemd on a UNIX socket
named in the NOTIFY_SOCKET environment variable. Rather than
pulling in the systemd libraries for this, just code the
notification directly in libvirt as this is a stable ABI
from systemd's POV which explicitly allows independant
implementations:

See "Reimplementable Independently" column in the
"$NOTIFY_SOCKET Daemon Notifications" row:

https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/

Signed-off-by: Daniel P. Berrange 
---
 configure.ac  |  2 --
 libvirt.spec.in   | 12 ---
 m4/virt-systemd-daemon.m4 | 34 ---
 src/Makefile.am   |  4 ++--
 src/util/virsystemd.c | 52 ++-
 5 files changed, 49 insertions(+), 55 deletions(-)
 delete mode 100644 m4/virt-systemd-daemon.m4

diff --git a/configure.ac b/configure.ac
index 74c33b3..a63b912 100644
--- a/configure.ac
+++ b/configure.ac
@@ -256,7 +256,6 @@ LIBVIRT_CHECK_SANLOCK
 LIBVIRT_CHECK_SASL
 LIBVIRT_CHECK_SELINUX
 LIBVIRT_CHECK_SSH2
-LIBVIRT_CHECK_SYSTEMD_DAEMON
 LIBVIRT_CHECK_UDEV
 LIBVIRT_CHECK_WIRESHARK
 LIBVIRT_CHECK_NSS
@@ -2787,7 +2786,6 @@ LIBVIRT_RESULT_SANLOCK
 LIBVIRT_RESULT_SASL
 LIBVIRT_RESULT_SELINUX
 LIBVIRT_RESULT_SSH2
-LIBVIRT_RESULT_SYSTEMD_DAEMON
 LIBVIRT_RESULT_UDEV
 LIBVIRT_RESULT_WIRESHARK
 LIBVIRT_RESULT_NSS
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 8b88eef..b93a53c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -79,7 +79,6 @@
 %define with_firewalld 0%{!?_without_firewalld:0}
 %define with_libssh2   0%{!?_without_libssh2:0}
 %define with_wireshark 0%{!?_without_wireshark:0}
-%define with_systemd_daemon 0%{!?_without_systemd_daemon:0}
 %define with_pm_utils  1
 
 # Finally set the OS / architecture specific special cases
@@ -133,7 +132,6 @@
 # Fedora has systemd, libvirt still used sysvinit there.
 %if 0%{?fedora} || 0%{?rhel} >= 7
 %define with_systemd 1
-%define with_systemd_daemon 1
 %define with_pm_utils 0
 %endif
 
@@ -268,9 +266,6 @@ BuildRequires: python
 %if %{with_systemd}
 BuildRequires: systemd-units
 %endif
-%if %{with_systemd_daemon}
-BuildRequires: systemd-devel
-%endif
 %if %{with_xen} || %{with_libxl}
 BuildRequires: xen-devel
 %endif
@@ -1061,12 +1056,6 @@ rm -rf .git
 %define arg_wireshark --without-wireshark-dissector
 %endif
 
-%if %{with_systemd_daemon}
-%define arg_systemd_daemon --with-systemd-daemon
-%else
-%define arg_systemd_daemon --without-systemd-daemon
-%endif
-
 %if %{with_pm_utils}
 %define arg_pm_utils --with-pm-utils
 %else
@@ -1157,7 +1146,6 @@ rm -f po/stamp-po
--with-driver-modules \
%{?arg_firewalld} \
%{?arg_wireshark} \
-   %{?arg_systemd_daemon} \
%{?arg_pm_utils} \
--with-nss-plugin \
%{arg_packager} \
diff --git a/m4/virt-systemd-daemon.m4 b/m4/virt-systemd-daemon.m4
deleted file mode 100644
index 8516e41..000
--- a/m4/virt-systemd-daemon.m4
+++ /dev/null
@@ -1,34 +0,0 @@
-dnl The libsystemd-daemon.so library
-dnl
-dnl Copyright (C) 2012-2013 Red Hat, Inc.
-dnl
-dnl This library is free software; you can redistribute it and/or
-dnl modify it under the terms of the GNU Lesser General Public
-dnl License as published by the Free Software Foundation; either
-dnl version 2.1 of the License, or (at your option) any later version.
-dnl
-dnl This library is distributed in the hope that it will be useful,
-dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
-dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-dnl Lesser General Public License for more details.
-dnl
-dnl You should have received a copy of the GNU Lesser General Public
-dnl License along with this library.  If not, see
-dnl .
-dnl
-
-AC_DEFUN([LIBVIRT_CHECK_SYSTEMD_DAEMON],[
-  LIBVIRT_CHECK_PKG([SYSTEMD_DAEMON], [libsystemd-daemon], [0.27.1])
-
-old_CFLAGS="$CFLAGS"
-old_LIBS="$LIBS"
-CFLAGS="$CFLAGS $SYSTEMD_DAEMON_CFLAGS"
-LIBS="$LIBS $SYSTEMD_DAEMON_LIBS"
-AC_CHECK_FUNCS([sd_notify])
-CFLAGS="$old_CFLAGS"
-LIBS="$old_LIBS"
-])
-
-AC_DEFUN([LIBVIRT_RESULT_SYSTEMD_DAEMON],[
-  LIBVIRT_RESULT_LIB([SYSTEMD_DAEMON])
-])
diff --git a/src/Makefile.am b/src/Makefile.am
index 12b66c2..11c79df 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1106,12 +1106,12 @@ libvirt_util_la_SOURCES =   
\
 libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \
$(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \
$(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS)  \
-   $(SYSTEMD_DAEMON_CFLAGS) $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \
+   $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \
-I$(srcdir)

Re: [libvirt] [PATCH] docs: Document our event loop

[Peter Krempa]

On Sun, Jun 05, 2016 at 12:19:47 +0200, Michal Privoznik wrote:
> I was asked the other day what's event loop and how libvirt uses
> it. Well, I haven't found any good sources on the Internet so I
> thought of writing the documentation on my own.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> NB, when I will push this, the images will go to our
> libvirt-media repo among with the original SVGs that PNGs were
> rendered from. I'm sending them here for easier review.
> 
>  docs/internals.html.in   |   1 +
>  docs/internals/event_loop_simple.png | Bin 0 -> 16043 bytes
>  docs/internals/event_loop_worker.png | Bin 0 -> 29696 bytes
>  docs/internals/eventloop.html.in | 106 
> +++
>  docs/sitemap.html.in |   4 ++
>  5 files changed, 111 insertions(+)
>  create mode 100644 docs/internals/event_loop_simple.png
>  create mode 100644 docs/internals/event_loop_worker.png
>  create mode 100644 docs/internals/eventloop.html.in

While the docs are just a very brief introduction to how it works in
libvirt it's still better than nothing. People that want to use it will
still need to dig through the sources though.

ACK

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

Re: [libvirt] [PATCH 6/7] storage: Create helper to set backing for CreateQemuImg code

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:11 -0400, John Ferlan wrote:
> Create a helper virStorageBackendCreateQemuImgSetBacking to perform the
> backing store set
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 118 
> --
>  1 file changed, 67 insertions(+), 51 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 4a3c41d..624790f 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -940,6 +940,7 @@ struct _virStorageBackendQemuImgInfo {
>  bool nocow;
>  
>  const char *backingPath;
> +const char *backingType;

backingFormatStr.

>  int backingFormat;
>  
>  const char *inputPath;

ACK with the adjustment

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

Re: [libvirt] [PATCH 7/7] storage: Create helper to set options for CreateQemuImg code

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:12 -0400, John Ferlan wrote:
> Create a helper virStorageBackendCreateQemuImgSetOptions to set either
> the qemu-img -o options or the previous mechanism using -F
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 58 
> ++-
>  1 file changed, 35 insertions(+), 23 deletions(-)

This will need changes after updating 4/7 so I'll review the updated
version.

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

Re: [libvirt] [PATCH 4/7] storage: Adjust qemu-img switches check

[John Ferlan]

On 06/06/2016 09:37 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
>> Since we support QEMU 0.12 and later, checking for support of specific flags
>> added prior to that isn't necessary.
>>
>> Thus start with the base of having the "-o options" available for the
>> qemu-img create option and then determine whether we have the compat
>> option for qcow2 files (which would be necessary up through qemu 2.0
>> where the default changes to compat 0.11).
>>
>> NOTE: Keeping old tests around since it's still possible to create in
>> the old format.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/storage/storage_backend.c | 66 
>> ---
>>  1 file changed, 24 insertions(+), 42 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index 2076155..4c40e43 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol,
>>  return ret;
>>  }
>>  
>> +/* Flag values shared w/ storagevolxml2argvtest.c.
>> + * Since it's still possible to provide the old format args, just
>> + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c)
>> + * to indicate the are older.
> 
> We use that approach as we can't delete the already existing flags since
> we wouldn't be able to parse the status XML. For qemu-img we don't store
> the flags anywhere so the old ones can be deleted.
> 
>> + *
>> + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11)
>> + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT
>> + *was made necessary due to 2.0 change to change the default
>> + *qcow2 file format from 0.10 to 1.1.
>> + */
>>  enum {
>> -QEMU_IMG_BACKING_FORMAT_NONE = 0,
>> -QEMU_IMG_BACKING_FORMAT_FLAG,
>> +X_QEMU_IMG_BACKING_FORMAT_NONE = 0,
> 
> So this can be dropped entirely after the hunk below.
> 
>> +X_QEMU_IMG_BACKING_FORMAT_FLAG,
> 
> This will be never set either after the hunk below.
> 
>>  QEMU_IMG_BACKING_FORMAT_OPTIONS,
>>  QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
>>  };
>> @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char 
>> *qemuimg)
>>  static int
>>  virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
>>  {
>> -char *help = NULL;
>> -char *start;
>> -char *end;
>> -char *tmp;
>> -int ret = -1;
>> -int exitstatus;
>> -virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL);
>> -
>> -virCommandAddEnvString(cmd, "LC_ALL=C");
>> -virCommandSetOutputBuffer(cmd, &help);
>> -virCommandClearCaps(cmd);
>> -
>> -/* qemuimg doesn't return zero exit status on -h,
>> - * therefore we need to provide pointer for storing
>> - * exit status, although we don't parse it any later */
>> -if (virCommandRun(cmd, &exitstatus) < 0)
>> -goto cleanup;
>> +/* As of QEMU 0.11 the [-o options] support was added via qemu
>> + * commit id '9ea2ea71', so we start with that base and figure
>> + * out what else we have */
>> +int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
> 
> So this flag can basically be completely removed as it should be always
> present in qemu-img 0.12
> 
>> +
>> +/* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
>> + * understands. Since we still support QEMU 0.12 and newer, we need
>> + * to be able to handle the previous format as can be set via a
>> + * compat=0.10 option. */
>> +if (virStorageBackendQemuImgSupportsCompat(qemuimg))
>> +ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
> 
> Yup, we need this.
> 
>>  
>> -if ((start = strstr(help, " create ")) == NULL ||
>> -(end = strstr(start, "\n")) == NULL) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("unable to parse qemu-img output '%s'"),
>> -   help);
>> -goto cleanup;
>> -}
>> -if (((tmp = strstr(start, "-F fmt")) && tmp < end) ||
>> -((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) {
>> -ret = QEMU_IMG_BACKING_FORMAT_FLAG;
>> -} else if ((tmp = strstr(start, "[-o options]")) && tmp < end) {
>> -if (virStorageBackendQemuImgSupportsCompat(qemuimg))
>> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
>> -else
>> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
>> -} else {
>> -ret = QEMU_IMG_BACKING_FORMAT_NONE;
>> -}
>> -
>> - cleanup:
>> -virCommandFree(cmd);
>> -VIR_FREE(help);
>>  return ret;
>>  }
>>  
>> @@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
>> conn,
>>  VIR_FREE(opts);
>>  } else {
>>  if (info.backingPath) {
>> -if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG)
>> +if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG)
> 
> 
> So ... if the above is never set, why aren't you dropping this entire
> case?
> 

It wasn't clear i

Re: [libvirt] [PATCH 5/7] storage: Create helper to set input for CreateQemuImg code

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:10 -0400, John Ferlan wrote:
> Create helper virStorageBackendCreateQemuImgSetInput to set the input
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 50 
> +++
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 4c40e43..4a3c41d 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -943,6 +943,7 @@ struct _virStorageBackendQemuImgInfo {
>  int backingFormat;
>  
>  const char *inputPath;
> +const char *inputType;

This is misleading. That's the string version of 'inputFormat'. I'd call
it inputFormatStr if you wan't to store it here.

>  int inputFormat;
>  };
>  

ACK with the tweak

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

Re: [libvirt] [PATCH 4/7] storage: Adjust qemu-img switches check

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:09 -0400, John Ferlan wrote:
> Since we support QEMU 0.12 and later, checking for support of specific flags
> added prior to that isn't necessary.
> 
> Thus start with the base of having the "-o options" available for the
> qemu-img create option and then determine whether we have the compat
> option for qcow2 files (which would be necessary up through qemu 2.0
> where the default changes to compat 0.11).
> 
> NOTE: Keeping old tests around since it's still possible to create in
> the old format.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 66 
> ---
>  1 file changed, 24 insertions(+), 42 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 2076155..4c40e43 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -869,9 +869,19 @@ virStoragePloopResize(virStorageVolDefPtr vol,
>  return ret;
>  }
>  
> +/* Flag values shared w/ storagevolxml2argvtest.c.
> + * Since it's still possible to provide the old format args, just
> + * keep them; however, prefix with an "X_" (similar to qemu_capabilities.c)
> + * to indicate the are older.

We use that approach as we can't delete the already existing flags since
we wouldn't be able to parse the status XML. For qemu-img we don't store
the flags anywhere so the old ones can be deleted.

> + *
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS (added in qemu 0.11)
> + * QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT
> + *was made necessary due to 2.0 change to change the default
> + *qcow2 file format from 0.10 to 1.1.
> + */
>  enum {
> -QEMU_IMG_BACKING_FORMAT_NONE = 0,
> -QEMU_IMG_BACKING_FORMAT_FLAG,
> +X_QEMU_IMG_BACKING_FORMAT_NONE = 0,

So this can be dropped entirely after the hunk below.

> +X_QEMU_IMG_BACKING_FORMAT_FLAG,

This will be never set either after the hunk below.

>  QEMU_IMG_BACKING_FORMAT_OPTIONS,
>  QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
>  };
> @@ -904,46 +914,18 @@ virStorageBackendQemuImgSupportsCompat(const char 
> *qemuimg)
>  static int
>  virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
>  {
> -char *help = NULL;
> -char *start;
> -char *end;
> -char *tmp;
> -int ret = -1;
> -int exitstatus;
> -virCommandPtr cmd = virCommandNewArgList(qemuimg, "-h", NULL);
> -
> -virCommandAddEnvString(cmd, "LC_ALL=C");
> -virCommandSetOutputBuffer(cmd, &help);
> -virCommandClearCaps(cmd);
> -
> -/* qemuimg doesn't return zero exit status on -h,
> - * therefore we need to provide pointer for storing
> - * exit status, although we don't parse it any later */
> -if (virCommandRun(cmd, &exitstatus) < 0)
> -goto cleanup;
> +/* As of QEMU 0.11 the [-o options] support was added via qemu
> + * commit id '9ea2ea71', so we start with that base and figure
> + * out what else we have */
> +int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;

So this flag can basically be completely removed as it should be always
present in qemu-img 0.12

> +
> +/* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
> + * understands. Since we still support QEMU 0.12 and newer, we need
> + * to be able to handle the previous format as can be set via a
> + * compat=0.10 option. */
> +if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> +ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;

Yup, we need this.

>  
> -if ((start = strstr(help, " create ")) == NULL ||
> -(end = strstr(start, "\n")) == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("unable to parse qemu-img output '%s'"),
> -   help);
> -goto cleanup;
> -}
> -if (((tmp = strstr(start, "-F fmt")) && tmp < end) ||
> -((tmp = strstr(start, "-F backing_fmt")) && tmp < end)) {
> -ret = QEMU_IMG_BACKING_FORMAT_FLAG;
> -} else if ((tmp = strstr(start, "[-o options]")) && tmp < end) {
> -if (virStorageBackendQemuImgSupportsCompat(qemuimg))
> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
> -else
> -ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
> -} else {
> -ret = QEMU_IMG_BACKING_FORMAT_NONE;
> -}
> -
> - cleanup:
> -virCommandFree(cmd);
> -VIR_FREE(help);
>  return ret;
>  }
>  
> @@ -1219,7 +1201,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr 
> conn,
>  VIR_FREE(opts);
>  } else {
>  if (info.backingPath) {
> -if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG)
> +if (imgformat == X_QEMU_IMG_BACKING_FORMAT_FLAG)


So ... if the above is never set, why aren't you dropping this entire
case?

>  virCommandAddArgList(cmd, "-F", backingType, NULL);
>  else
>  VIR_DEBUG("Unable to set backing store format for %s with 
> %s",

--
libvir-list mailing list
libvir-list@

Re: [libvirt] [PATCH v6 2/2] qemu: Add support to QXL's max_outputs parameter

[Pavel Hrdina]

On Sun, Jun 05, 2016 at 02:36:04AM +0200, Martin Kletzander wrote:
> Historically, we added heads=1 to videos, but for example for qxl, we
> did not reflect that on the command line.  Implementing that now could
> mean that if user were to migrate from older to newer libvirt, the
> command-line for qemu would differ.  In order for that not to happen a
> migration cookie flag is introduced.

Remove the migration cookie from commit message.

> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283207
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_command.c|  8 
>  src/qemu/qemu_migration.c  |  1 -
>  .../qemuxml2argv-video-qxl-heads.args  | 28 +
>  .../qemuxml2argv-video-qxl-heads.xml   | 47 
> ++
>  .../qemuxml2argv-video-qxl-noheads.args| 24 +++
>  .../qemuxml2argv-video-qxl-noheads.xml | 39 ++
>  tests/qemuxml2argvtest.c   | 16 
>  .../qemuxml2xmlout-video-qxl-heads.xml | 47 
> ++
>  .../qemuxml2xmlout-video-qxl-noheads.xml   | 39 ++
>  tests/qemuxml2xmltest.c|  3 ++
>  10 files changed, 251 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-heads.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-video-qxl-noheads.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 368bd871f7e3..60b0049f3d22 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4199,6 +4199,14 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
>  /* QEMU accepts mebibytes for vgamem_mb. */
>  virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
>  }
> +
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
> +virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS)) {
> +if (video->heads)
> +virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
> +} else {
> +video->heads = 0;
> +}

Same as for the first patch, qxl and qxl-vga are different devices.  Currently
we have qxl-vga only as primary video device and qxl only as secondary.  You
should pair the capabilities with the correct video device like we do for
vram64.

>  } else if (video->vram &&
>  ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
>virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index c5b2963d65b6..bed5f0b581b9 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3131,7 +3131,6 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>  if (nmigrate_disks) {
>  if (has_drive_mirror) {
>  size_t i, j;
> -

Unrelated white space change.

>  /* Check user requested only known disk targets. */
>  for (i = 0; i < nmigrate_disks; i++) {
>  for (j = 0; j < vm->def->ndisks; j++) {
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.args
> new file mode 100644

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-heads.xml
> new file mode 100644

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.args
> new file mode 100644

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-noheads.xml
> new file mode 100644

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index db42b7bd71be..716755b39827 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1972,6 +1972,22 @@ mymain(void)
>  DO_TEST("ppc64-usb-controller-legacy",
>  QEMU_CAPS_PIIX3_USB_UHCI);
> 
> +DO_TEST("video-qxl-heads",
> +QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> +QEMU_CAPS_VGA_QXL,
> +QEMU_CAPS_DEVICE_QXL_VGA,
> +QEMU_CAPS_DEVICE_QXL,
> +QEMU_CAPS_QXL_MAX_OUTPUTS,
> +QEMU_CAPS_QXL_VGA_MAX_OUTPUTS);
> +
> +DO_TEST("video-qxl-noheads",
> +QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> +QEMU_CAPS_VGA_QXL,
> +QEMU_CAPS_DEVICE_QXL_VGA,
> +QEMU_CAPS_DEVICE_QXL,
> +QEMU_CAPS_QXL_MAX_OUTPUTS,
> +   

Re: [libvirt] [PATCH 3/7] storage: Split out a helper for encryption checks

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:08 -0400, John Ferlan wrote:
> Split out a helper from virStorageBackendCreateQemuImgCmdFromVol
> to check the encryption - soon a new encryption sheriff will be
> patroling and that'll mean all sorts of new checks.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 79 
> ---
>  1 file changed, 51 insertions(+), 28 deletions(-)

ACK

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

Re: [libvirt] [PATCH 2/7] storage: Split out setting default secret for encryption

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:07 -0400, John Ferlan wrote:
> Split the qcow setting of encryption secrets into a helper
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_fs.c | 79 
> +---
>  1 file changed, 49 insertions(+), 30 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH v6 1/2] qemu: Check for qxl's max_outputs parameter

[Pavel Hrdina]

On Sun, Jun 05, 2016 at 02:36:03AM +0200, Martin Kletzander wrote:
> There is max_outputs parameter for both qxl and qxl-vga and there is no
> easy way of saying that we want the capability enabled only if both are
> supported.  So let's have two of them and only use them together.

I would rephrase the commit message.  The qxl and qxl-vga are different devices
and even though they are both usually present.

> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_capabilities.c | 5 +
>  src/qemu/qemu_capabilities.h | 4 
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 2 ++
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 2 ++
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 2 ++
>  5 files changed, 15 insertions(+)
> 

ACK

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

Re: [libvirt] [PATCH 1/7] util: Clean up code formatting in virstorageencryption

[Peter Krempa]

On Fri, Jun 03, 2016 at 06:42:06 -0400, John Ferlan wrote:
> Bring style more in line with more recent code.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/util/virstorageencryption.c | 58 
> +++--
>  1 file changed, 27 insertions(+), 31 deletions(-)

ACK

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

Re: [libvirt] [PATCH RFC 32/32] Add nomatch filters when enumerating udev devices

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:09 +0200, Ján Tomko wrote:
> Filter out some subsystems we are not interested in.
> ---
> After the netdev cleanups, this speeds up the driver initialization from 18 
> ms to 13 ms,
> which is percentually a lot, but neligible in absolute times.
> 
> This patch uses a negative filter because I could not find an exhaustive list 
> of possible subsystems.
> A positive filter could be applied to udev_monitor as well.
> 
>  src/node_device/node_device_udev.c | 23 ++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index b46fec6..2e86230 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1167,13 +1167,34 @@ static int udevProcessDeviceListEntry(struct udev 
> *udev,
>  }
>  
> 

I think this deserves a comment explaining the reason and what to add
here.

> +const char *subsystem_blacklist[] = {
> +"acpi", "tty", "vc", "i2c",
> +};
> +
> +static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate)
> +{
> +size_t i;
> +
> +for (i = 0; i < ARRAY_CARDINALITY(subsystem_blacklist); i++) {
> +const char *s = subsystem_blacklist[i];
> +if (udev_enumerate_add_nomatch_subsystem(udev_enumerate, s) < 0) {
> +virReportSystemError(errno, "%s", _("failed to add susbsystem 
> filter"));
> +return -1;

Should this be fatal? On the other hand other stuff will probably fail
too.

> +}
> +}
> +return 0;
> +}
> +
> +
>  static int udevEnumerateDevices(struct udev *udev)
>  {
>  struct udev_enumerate *udev_enumerate = NULL;
>  struct udev_list_entry *list_entry = NULL;
> -int ret = 0;
> +int ret = -1;
>  
>  udev_enumerate = udev_enumerate_new(udev);
> +if (udevEnumerateAddMatches(udev_enumerate) < 0)
> +goto cleanup;
>  
>  ret = udev_enumerate_scan_devices(udev_enumerate);
>  if (ret != 0) {

ACK with the comment added.

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

Re: [libvirt] [PATCH 24/32] Remove PROPERTY_* constants

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:01 +0200, Ján Tomko wrote:
> They are no longer used.
> ---
>  src/node_device/node_device_udev.c | 41 
> --
>  src/node_device/node_device_udev.h |  3 ---
>  2 files changed, 13 insertions(+), 31 deletions(-)

ACK

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

Re: [libvirt] [PATCH 23/32] Only return two values in udevGetUintSysfsAttr

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:00 +0200, Ján Tomko wrote:
> Open code the call to udev_device_get_sysattr_value
> in the one place where it's needed.
> ---
>  src/node_device/node_device_udev.c | 65 
> +++---
>  1 file changed, 18 insertions(+), 47 deletions(-)

ACK

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

Re: [libvirt] [PATCH 27/32] Reformat udevProcessRemoveableMedia

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:04 +0200, Ján Tomko wrote:
> Remove unnecessary ret variable and return early if we have no media
> to save on indentation.
> ---
>  src/node_device/node_device_udev.c | 45 
> +++---
>  1 file changed, 22 insertions(+), 23 deletions(-)

ACK

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

Re: [libvirt] [PATCH 25/32] node_device_udev: switch to using virReportError

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:02 +0200, Ján Tomko wrote:
> Also use the more common "Unable to initialize mutex" string
> and virReportSystemError instead of virStrerror.
> ---
>  src/node_device/node_device_udev.c | 47 
> +++---
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index 54eb319..8307b80 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c

[...]

> @@ -1212,7 +1219,7 @@ static int udevEnumerateDevices(struct udev *udev)
>  
>  ret = udev_enumerate_scan_devices(udev_enumerate);
>  if (0 != ret) {
> -VIR_ERROR(_("udev scan devices returned %d"), ret);
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("udev scan devices returned 
> %d"), ret);

Resulting line is too long.

>  goto out;
>  }
>  
> @@ -1292,14 +1299,15 @@ static void udevEventHandleCallback(int watch 
> ATTRIBUTE_UNUSED,
>  nodeDeviceLock();
>  udev_fd = udev_monitor_get_fd(udev_monitor);
>  if (fd != udev_fd) {
> -VIR_ERROR(_("File descriptor returned by udev %d does not "
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned 
> by udev %d does not "
>  "match node device file descriptor %d"), fd, udev_fd);

Resulting line is too long and formatting is broken.

>  goto out;
>  }
>  
>  device = udev_monitor_receive_device(udev_monitor);
>  if (device == NULL) {
> -VIR_ERROR(_("udev_monitor_receive_device returned NULL"));
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("udev_monitor_receive_device returned NULL"));
>  goto out;
>  }
>  
> @@ -1337,7 +1345,7 @@ udevGetDMIData(virNodeDevCapDataPtr data)
>  if (device == NULL) {
>  device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK);
>  if (device == NULL) {
> -VIR_ERROR(_("Failed to get udev device for syspath '%s' or 
> '%s'"),
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get udev 
> device for syspath '%s' or '%s'"),
>DMI_DEVPATH, DMI_DEVPATH_FALLBACK);

Too long and broken too.

>  goto out;
>  }

ACK with the defects fixed.

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

Re: [libvirt] [PATCH 31/32] node_device_udev: rename labels to cleanup

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:08 +0200, Ján Tomko wrote:
> Instead of the custom out and out_unlock.
> ---
>  src/node_device/node_device_udev.c | 154 
> ++---
>  1 file changed, 77 insertions(+), 77 deletions(-)

ACK

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

Re: [libvirt] [PATCH 26/32] udevProcessStorage: trim all whitespace from model and vendor

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:03 +0200, Ján Tomko wrote:
> Use virTrimSpaces instead of a custom implementation.
> ---
>  src/node_device/node_device_udev.c | 21 +
>  1 file changed, 5 insertions(+), 16 deletions(-)

ACK

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

Re: [libvirt] [PATCH 30/32] node_device_udev: remove unnecessary ret variables

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:07 +0200, Ján Tomko wrote:
> Remove ret variables and labels from functions where there is no cleanup
> to be done.
> ---
>  src/node_device/node_device_udev.c | 108 
> ++---
>  1 file changed, 39 insertions(+), 69 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index aa4c36e..88a333e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c

udevProcessDisk needs the same treatment. ACK with that added.

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

Re: [libvirt] [PATCH 30/32] node_device_udev: remove unnecessary ret variables

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:07 +0200, Ján Tomko wrote:
> Remove ret variables and labels from functions where there is no cleanup
> to be done.
> ---
>  src/node_device/node_device_udev.c | 108 
> ++---
>  1 file changed, 39 insertions(+), 69 deletions(-)

ACK

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

Re: [libvirt] [PATCH 29/32] node_device_udev: remove yoda condition

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:06 +0200, Ján Tomko wrote:
> ---
>  src/node_device/node_device_udev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK

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

Re: [libvirt] [PATCH 28/32] udevSetupSystemDev: return if allocation fails

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:02:05 +0200, Ján Tomko wrote:
> There is no cleanup to be done.
> ---
>  src/node_device/node_device_udev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

ACK

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

Re: [libvirt] [PATCH 22/32] Only return two values in udevGetIntSysfsAttr

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:59 +0200, Ján Tomko wrote:
> Callers only check for an error or a specific integer value.
> ---
>  src/node_device/node_device_udev.c | 28 +++-
>  1 file changed, 11 insertions(+), 17 deletions(-)

ACK

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

Re: [libvirt] [PATCH 21/32] Only return two values in udevGetStringSysfsAttr

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:58 +0200, Ján Tomko wrote:
> The callers only care for an error, and a missing attribute
> is simply NULL.
> ---
>  src/node_device/node_device_udev.c | 78 
> --
>  1 file changed, 24 insertions(+), 54 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index e2aeb32..6e97ac5 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c

ACK

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

Re: [libvirt] [PATCH 20/32] Remove extra allocation in udevGetDeviceSysfsAttr

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:57 +0200, Ján Tomko wrote:
> Most of the code paths free it right after converting it to
> an integer.
> ---
>  src/node_device/node_device_udev.c | 98 
> +++---
>  1 file changed, 29 insertions(+), 69 deletions(-)

I guess it makes sense to keep the wrapper to log the debug message.

ACK

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

Re: [libvirt] [PATCH 1/3] maint: Use libsystemd instead of libsystemd-daemon

[Daniel P. Berrange]

On Fri, May 27, 2016 at 02:55:06PM +0200, Andrea Bolognani wrote:
> The libsystemd-daemon library had been deprecated upstream
> just a few days before we started using it. Talk about bad
> timing :)
> 
> With systemd 230, now in Debian sid and Fedora rawhide, it
> has finally been dropped. We should use libsystemd, its
> replacement, instead.
> ---
>  configure.ac  |  4 ++--
>  libvirt.spec.in   | 14 +++---
>  m4/{virt-systemd-daemon.m4 => virt-libsystemd.m4} | 14 +++---
>  src/Makefile.am   |  4 ++--
>  src/util/virsystemd.c |  4 ++--
>  5 files changed, 20 insertions(+), 20 deletions(-)
>  rename m4/{virt-systemd-daemon.m4 => virt-libsystemd.m4} (73%)

Just unconditionally dropping support for libsystemd-daemon.so
means you require systemd >= 209 for libvirt to build. IMHO
this is too new.

The only thing we use this for is sd_notify() which is a really trivial
method which gets a NUIX socket from NOTIFY_SOCKET env variable, opens
it and just sends across the string. I think we could just implement
this logic trivially ourselvs, and not need to use any library, thus
avoiding the problem with it being moved

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 18/32] Only return two values in udevGetStringProperty

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:55 +0200, Ján Tomko wrote:
> There is no need to differentiate between PROPERTY_FOUND
> and PROPERTY_MISSING - we can just look if the string is non-NULL.
> ---
>  src/node_device/node_device_udev.c | 87 
> --
>  1 file changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index 35d0dc7..876fca1 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -90,9 +90,8 @@ static int udevGetStringProperty(struct udev_device 
> *udev_device,
>  {
>  if (VIR_STRDUP(*value,
> udevGetDeviceProperty(udev_device, property_key)) < 0)
> -return PROPERTY_ERROR;
> -
> -return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND;
> +return -1;

Add a newline here.

> +return 0;
>  }

ACK

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

Re: [libvirt] [PATCH v2 01/10] conf: Rename VIR_DOMAIN_DEF_PARSE_VALIDATE to VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA

[Pavel Hrdina]

On Fri, May 27, 2016 at 02:21:50PM +0200, Peter Krempa wrote:
> Make it obvious that the flag is controlling RNG schema validation.
> ---

ACK

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

Re: [libvirt] [PATCH 19/32] Only return two values in udevGetUintProperty

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:56 +0200, Ján Tomko wrote:
> We only care about the failure, not a missing property.

Well, you made us not care about the missing property in two instances
here :).

> ---
>  src/node_device/node_device_udev.c | 60 
> --
>  1 file changed, 18 insertions(+), 42 deletions(-)

ACK

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

Re: [libvirt] [PATCH v3 4/5] qemu: Remove need for qemuBuildSecretInfoProps

[John Ferlan]

On 06/06/2016 07:31 AM, Peter Krempa wrote:
> On Mon, Jun 06, 2016 at 07:16:29 -0400, John Ferlan wrote:
>>
>>
>> On 06/06/2016 03:26 AM, Peter Krempa wrote:
>>> On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
 Just move the code into qemuBuildObjectSecretCommandLine.

 Signed-off-by: John Ferlan 
 ---
  src/qemu/qemu_command.c | 57 
 +++--
  1 file changed, 12 insertions(+), 45 deletions(-)
>>>
>>> Again. If this is going to be reused in the monitor then it doesn't make
>>> sense to move it to the builder directly. If it's not going to be used
>>> with the monitor then it doesn't make sense to use the json parser and
>>> the object formatter to achieve the same at all since it's rather
>>> untrivial code that can be achieved with a single printf.
>>>
>>
>> Huh?  This patch just moves code from qemuBuildSecretInfoProps into
>> qemuBuildObjectSecretCommandLine as the only caller to the former was
>> the latter.
>>
>> I can drop this though - it's not that important.
> 
> I'm pointing out that if there's no need to use the code to pass secret
> objects via the monitor then we should not use the JSON to commandline
> generator at all.
> 
> Moving of the code directly to the place where we format the command
> line is a very apparent reason that it's not going to be used anywhere
> besides the command line.
> 

That's fine - I'll drop 3 & 4. The qemuBuildSecretInfoProps could have a
future use from hotplug for the monitor.

John

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

Re: [libvirt] [PATCH v2 05/10] qemu: process: Unexport qemuProcessStartValidate

[Pavel Hrdina]

On Fri, May 27, 2016 at 02:21:54PM +0200, Peter Krempa wrote:
> ---
>  src/qemu/qemu_process.c | 2 +-
>  src/qemu/qemu_process.h | 7 ---
>  2 files changed, 1 insertion(+), 8 deletions(-)

ACK

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

Re: [libvirt] [PATCH 17/32] Rewrite disk type checking in udevProcessStorage

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:54 +0200, Ján Tomko wrote:
> Error out on parsing errors and use a local const char pointer
> instead of chained ifs to check whether we found a match.
> ---
>  src/node_device/node_device_udev.c | 32 ++--
>  1 file changed, 18 insertions(+), 14 deletions(-)

You didn't help in making the code more clear but didn't make it worse
either.

ACK

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

Re: [libvirt] [PATCH v3 5/5] secret: Move virStorageSecretType to secret_util and rename

[John Ferlan]

On 06/06/2016 03:27 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:52:53 -0400, John Ferlan wrote:
>> Move the enum into secret_util, rename it to be just virSecretLookupType.
>> This includes quite a bit of collateral damage, but the goal is to remove
>> the "virStorage*" and replace with the virSecretLookupType so that it's
>> easier to to add new lookups that aren't necessarily storage pool related.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  cfg.mk  |  2 +-
>>  src/conf/secret_conf.h  |  2 +-
>>  src/libxl/libxl_conf.c  |  2 +-
>>  src/qemu/qemu_domain.c  |  4 ++--
>>  src/secret/secret_util.c| 18 +-
>>  src/secret/secret_util.h| 22 --
>>  src/storage/storage_backend_iscsi.c |  7 ---
>>  src/storage/storage_backend_rbd.c   |  3 ++-
>>  src/util/virstoragefile.c   | 33 +
>>  src/util/virstoragefile.h   | 17 +++--
>>  tests/qemuargv2xmltest.c|  4 ++--
>>  11 files changed, 62 insertions(+), 52 deletions(-)
>>
>> diff --git a/cfg.mk b/cfg.mk
>> index a7b7266..0529a4e 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -780,7 +780,7 @@ 
>> mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag
>>  sc_prohibit_cross_inclusion:
>>  @for dir in $(cross_dirs); do   \
>>case $$dir in \
>> -util/) safe="util";;\
>> +util/) safe="(util|secret)";;   \
> 
> I don't think this is a good idea. utils are used in many places that
> don't link to the secret driver. While this is now used just to pull in
> one data type, the check is meant to prevent problems with linking the
> file if certain modules are disabled.
> 

Understood, but it was a far less invasive option than chasing the
virstoragefile.h includes to then modify many more places in order to
get the definition.

I would think that definition belongs with secret_util. It's how the
lookup of uuid/usage for virSecretGetSecretString works and has less to
do with something storage specific. If there's another way to make some
adjustment to that particular syntax-check, that's fine, but this is
what I found that worked. Those checks aren't necessarily very readable
to my eyes.

I'm not against keeping it in virstoragefile.h if that's the preference.
Is the name change OK or would you prefer to see
virStorageLookupTypeDef. It's always been a bit confusing when to see
"secretType" in the code when it really was the lookup type not the
"type" from the  object.

Taking things another step further - eventually secrets will be used to
support TLS objects - having a structure with virStorageLookupTypeDef
wouldn't seem to be appropriate.

Untying the existing relationship between the secret driver and the
storage driver (as well as qemu and libxl) without "falling back to"
libvirt API calls to virSecret* is going to require some amount of
cross-pollination. It's a matter of choice.

John


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

Re: [libvirt] [PATCH 16/32] Fix the return value in udevKludgeStorageType

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:53 +0200, Ján Tomko wrote:
> Since the switch to VIR_STRDUP this function returns 1 on success,
> but the caller treats any non-zero value as failure.
> ---
>  src/node_device/node_device_udev.c | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)

ACK

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

Re: [libvirt] [PATCH v3 4/5] qemu: Remove need for qemuBuildSecretInfoProps

[Peter Krempa]

On Mon, Jun 06, 2016 at 07:16:29 -0400, John Ferlan wrote:
> 
> 
> On 06/06/2016 03:26 AM, Peter Krempa wrote:
> > On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
> >> Just move the code into qemuBuildObjectSecretCommandLine.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/qemu/qemu_command.c | 57 
> >> +++--
> >>  1 file changed, 12 insertions(+), 45 deletions(-)
> > 
> > Again. If this is going to be reused in the monitor then it doesn't make
> > sense to move it to the builder directly. If it's not going to be used
> > with the monitor then it doesn't make sense to use the json parser and
> > the object formatter to achieve the same at all since it's rather
> > untrivial code that can be achieved with a single printf.
> > 
> 
> Huh?  This patch just moves code from qemuBuildSecretInfoProps into
> qemuBuildObjectSecretCommandLine as the only caller to the former was
> the latter.
> 
> I can drop this though - it's not that important.

I'm pointing out that if there's no need to use the code to pass secret
objects via the monitor then we should not use the JSON to commandline
generator at all.

Moving of the code directly to the place where we format the command
line is a very apparent reason that it's not going to be used anywhere
besides the command line.

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

Re: [libvirt] [PATCH v3 3/5] qemu: Use virJSONValueObjectCreate for master key

[Peter Krempa]

On Mon, Jun 06, 2016 at 07:14:00 -0400, John Ferlan wrote:
> 
> 
> On 06/06/2016 03:23 AM, Peter Krempa wrote:
> > On Fri, Jun 03, 2016 at 06:52:51 -0400, John Ferlan wrote:
> >> Rather than open coding, follow the secinfo code and use the common
> >> secret object build/generate sequence.
> > 
> > The main reason to do this was to have a single code path that generates
> > properties both for adding the object to qemu via commandline and via
> > monitor. Is this going to be used on the monitor? If not there's no
> > reason to do this.
> > 
> 
> The main reason to do this was to have the commandline code generating
> the master secret perform the same virJSON* call that the code for
> generating the AES secret uses.

Well that is useful only if you are going to use the same code
generating the properties to be used via monitor too. Otherwise the code
will waste quite a few cycles to parse the strings and generate the JSON
structure just to iterate it and format it back to strings.

> 
> Not sure what the reference for via monitor is all about.

The idea behind qemuBuildObjectCommandlineFromJSON or
virQEMUBuildObjectCommandlineFromJSON as of 1/5 was to have a common
place that generates properties for objects (namely memory device
backends at the time I wrote that fucntion) so that we don't need to
implement it twice. Once for the command line and once for the monitor.

As the monitor json property can be converted to the commandline it's
desired to use the JSON format then.

> 
> If using common code isn't desire, then I can drop it.

It is desire if the code will be used both when generating the command
line and when talking on the monitor. The master key is not the case
since we will not be setting the master key using the monitor.

Additionally since we consider that the monitor is secure from prying
eyes we can pass the secrets directly that way so generating the JSON
props is just a waste of compute time.

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

Re: [libvirt] [PATCH 15/32] udevProcessFloppy; remove unnecessary allocation

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:52 +0200, Ján Tomko wrote:
> Use udevHasDeviceProperty instead of udevGetStringProperty.
> We do not need to copy the string since we do not need it.
> 
> Also add braces around the if body, since the change made
> syntax check complain.
> ---
>  src/node_device/node_device_udev.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)

ACK

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

Re: [libvirt] [PATCH v3 4/5] qemu: Remove need for qemuBuildSecretInfoProps

[John Ferlan]

On 06/06/2016 03:26 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:52:52 -0400, John Ferlan wrote:
>> Just move the code into qemuBuildObjectSecretCommandLine.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_command.c | 57 
>> +++--
>>  1 file changed, 12 insertions(+), 45 deletions(-)
> 
> Again. If this is going to be reused in the monitor then it doesn't make
> sense to move it to the builder directly. If it's not going to be used
> with the monitor then it doesn't make sense to use the json parser and
> the object formatter to achieve the same at all since it's rather
> untrivial code that can be achieved with a single printf.
> 

Huh?  This patch just moves code from qemuBuildSecretInfoProps into
qemuBuildObjectSecretCommandLine as the only caller to the former was
the latter.

I can drop this though - it's not that important.

John

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

Re: [libvirt] [PATCH 14/32] Move udevHasDeviceProperty earlier

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:51 +0200, Ján Tomko wrote:
> ---
>  src/node_device/node_device_udev.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)

ACK

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

Re: [libvirt] [PATCH 13/32] Do not VIR_STRDUP the string in udevGetDeviceProperty

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:50 +0200, Ján Tomko wrote:
> Two out of three callers free it right after converting it to a number.
> 
> Also change the comment at the beginning of the function, because
> the comment inside the function told me to.
> ---
>  src/node_device/node_device_udev.c | 72 
> --
>  1 file changed, 23 insertions(+), 49 deletions(-)

ACK

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

Re: [libvirt] [PATCH v3 3/5] qemu: Use virJSONValueObjectCreate for master key

[John Ferlan]

On 06/06/2016 03:23 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:52:51 -0400, John Ferlan wrote:
>> Rather than open coding, follow the secinfo code and use the common
>> secret object build/generate sequence.
> 
> The main reason to do this was to have a single code path that generates
> properties both for adding the object to qemu via commandline and via
> monitor. Is this going to be used on the monitor? If not there's no
> reason to do this.
> 

The main reason to do this was to have the commandline code generating
the master secret perform the same virJSON* call that the code for
generating the AES secret uses.

Not sure what the reference for via monitor is all about.

If using common code isn't desire, then I can drop it.

John

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

Re: [libvirt] [PATCH 11/32] Remove udevStrToLong_ui

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:48 +0200, Ján Tomko wrote:
> Remove the debug message, open code the error in the two udevGetUint
> callers and use a more specific error in SCSI and PCI processing.
> ---
>  src/node_device/node_device_udev.c | 25 ++---
>  1 file changed, 6 insertions(+), 19 deletions(-)

ACK

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

Re: [libvirt] [PATCH 12/32] Remove udevStrToLong_i

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:49 +0200, Ján Tomko wrote:
> Open code the error message.
> ---
>  src/node_device/node_device_udev.c | 25 ++---
>  1 file changed, 6 insertions(+), 19 deletions(-)

ACK

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

Re: [libvirt] [PATCH v3 2/5] storage: Use virSecretGetSecretString

[John Ferlan]

On 06/06/2016 03:32 AM, Peter Krempa wrote:
> On Fri, Jun 03, 2016 at 06:52:50 -0400, John Ferlan wrote:
>> Rather than inline code secret lookup for rbd/iscsi, use the common function.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/Makefile.am |  1 +
>>  src/storage/storage_backend_iscsi.c | 50 
>> +
>>  src/storage/storage_backend_rbd.c   | 48 +++
>>  3 files changed, 10 insertions(+), 89 deletions(-)
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index f3c9a14..019242b 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1615,6 +1615,7 @@ libvirt_driver_storage_impl_la_SOURCES =
>>  libvirt_driver_storage_impl_la_CFLAGS = \
>>  -I$(srcdir)/access \
>>  -I$(srcdir)/conf \
>> +-I$(srcdir)/secret \
> 
> Similarly to my complaint in 5/5 this is breaking the boundaries between
> the storage driver and the secret driver. This is not the first case of
> this though so we just need to thoroughly check that the appropriate
> bits are linked in to the storage driver.
> 
> I'll need to check this.
> 
> Otherwise looks good.
> 
> Peter
> 

Do you mean boundaries that weren't broken before?  I would think 5/5 is
a different case/issue.  Ironcially, RBD/iSCSI would use virSecret*
libvirt API's to get the lookup type and then conn->secretDriver
knowledge to perform the actual lookup.

This is the one that had already been ACK'd and essentially follows
commit id 2844de6f4 done for libvirt_driver_qemu_impl_la and
libvirt_driver_libxl_impl_la. It's adding access to one function and one
enum.

John

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

Re: [libvirt] [PATCH 10/32] Remove udevStrToLong_ull

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:47 +0200, Ján Tomko wrote:
> The wrapper adds an error message or a debug log.
> 
> Since we already log the properties we get from udev as strings,
> there is no much use for the debug logs.
> 
> Open code the error message and delete the function.
> ---
>  src/node_device/node_device_udev.c | 21 +++--
>  1 file changed, 3 insertions(+), 18 deletions(-)

ACK

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

Re: [libvirt] [PATCH 09/32] Rewrite usage of StrToLong_ui in udevProcess{PCI, SCSI}

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:46 +0200, Ján Tomko wrote:
> Use virStrToLong_ui instead of udevStrToLong_ui, reformat the code
> and report a more specific error message.
> ---
>  src/node_device/node_device_udev.c | 80 
> ++
>  1 file changed, 21 insertions(+), 59 deletions(-)

ACK

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

Re: [libvirt] [PATCH 08/32] udevProcessSCSIHost: use STRSKIP

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:45 +0200, Ján Tomko wrote:
> Instead of separating it into STRPEFIX and str + strlen.
> ---
>  src/node_device/node_device_udev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

ACK

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

Re: [libvirt] [PATCH 07/32] udevGetDMIData: remove unused variable

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:44 +0200, Ján Tomko wrote:
> A variable without use is pointless.
> 
> Remove it, since we have no use for it.
> ---
>  src/node_device/node_device_udev.c | 2 --
>  1 file changed, 2 deletions(-)

ACK

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

Re: [libvirt] [PATCH 06/32] Assign node device driver private data earlier

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:43 +0200, Ján Tomko wrote:
> We do not need it to track if priv->udev_monitor is non-NULL.

I don't quite understand what you meant to say with this.

> ---
>  src/node_device/node_device_udev.c | 23 ++-
>  1 file changed, 10 insertions(+), 13 deletions(-)

ACK.

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

Re: [libvirt] [PATCH 05/32] Do not call nodeStateCleanup on early initialization error

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:42 +0200, Ján Tomko wrote:
> If we have not allocated driver yet, there is nothing to cleanup.
> ---
>  src/node_device/node_device_udev.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)

ACK

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

Re: [libvirt] [PATCH 04/32] Reformat nodeStateCleanup

[Peter Krempa]

On Mon, Jun 06, 2016 at 11:01:41 +0200, Ján Tomko wrote:
> Remove the ret variable and return early if there is no driver.
> ---
>  src/node_device/node_device_udev.c | 44 
> +-
>  1 file changed, 20 insertions(+), 24 deletions(-)

ACK

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