Re: [libvirt] Syslog nested job is dangerous message

[Sam Bobroff]

On 17/07/14 17:54, Ján Tomko wrote:
 On 07/17/2014 08:51 AM, Jiri Denemark wrote:
 On Thu, Jul 17, 2014 at 13:29:52 +1000, Sam Bobroff wrote:
 On 17/07/14 12:50, Eric Blake wrote:
 On 07/16/2014 07:52 PM, Sam Bobroff wrote:
 Hello everyone,

 [Can you configure your mailer to wrap long lines?]

 [No problem, done.]

 After performing a migration, the syslog often contains several
 messages like this:

 This thread seems to be the async job owner; entering monitor
 without asking for a nested job is dangerous

 The sign of a bug that we need to fix.  What version of libvirt are
 you using?  We may have already fixed it.

 I've been testing on 1.2.6, but I will re-test on the latest code from git.

 Hmm, it what were you doing when the message appeared in the log? I
 fixed wrong usage of our job tracking APIs long time ago
 (e4e28220b572cf4c71d07740c24150411f0704a6 in 1.0.3) so I guess there
 must be another place we don't do it correctly.

 
 That commit was essentially reverted by my commit 9846402 when fixing
 migration crashes: https://bugzilla.redhat.com/show_bug.cgi?id=1018267
 
 I think the message was harmless during migration as we don't allow other jobs
 (except destroy).
 
 Jan

I've re-tested this on the latest libvirt from git (1.2.7) and I still
see several occurrences of the dangerous message on the destination
machine after performing a live migration with --copy-storage-all.

It seems like qemuDomainObjEnterMonitorInternal() is expecting the
asyncJob parameter to have the value of the currently running async job,
and in the cases that cause the warning message, it's being called via
qemuDomainObjEnterMonitor() which hard codes it to QEMU_ASYNC_NONE.

It looks like these calls should be changed to
qemuDomainObjEnterMonitorAsync() with the right asyncJob parameter,
which I believe is QEMU_ASYNC_JOB_MIGRATION_IN for the cases I'm looking
at (and the old value of QEMU_ASYNC_NONE for all other callers). There
aren't many cases of this, so fixing it this way is fairly simple but it
does involve passing the asyncJob through some intermediate functions.

Another approach would be to change qemuDomainObjEnterMonitorInternal()
so that it can automatically do the same thing: It could see that there
was an async job running, and that it's owned by the current thread, and
then call qemuDomainObjBeginNestedJob() (or directly call
qemuDomainObjBeginJobInternal() since we know that the checks in
qemuDomainObjBeginNestedJob() are going to succeed). It seems like this
might also simplify some other code that is doing pretty much just what
I described above.

The second approach does fix all the cases I'm looking at, and would
also fix other similar cases I haven't yet found, but I'm not sure that
it would be entirely safe or if it's circumventing some sort of safety
checking.

Any comments?
Which approach should I pursue?

Cheers,
Sam.

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

Re: [libvirt] [RFC][scale] new API for querying domains stats

[Richard W.M. Jones]

Did anything come of this discussion, and/or is someone working on this?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

Re: [libvirt] [PATCH v1 1/7] configure: Check for statfs

[Daniel P. Berrange]

On Mon, Jul 21, 2014 at 10:07:48AM -0600, Eric Blake wrote:
 On 07/21/2014 08:54 AM, Daniel P. Berrange wrote:
  On Thu, Jul 17, 2014 at 06:12:42PM +0200, Michal Privoznik wrote:
  The statfs(2) gets filesystem statistics. Currently, we use it only on
  linux, and leave stub to implement on other platforms. But hey, other
  platforms (like FreeBSD) have statfs() too. If we check it in
  configure we can wider platforms supported. Speaking of FreeBSD, the
  headers to include are of course different: sys/param.h and
  sys/mount.h on the FreeBSD and sys/statfs.h on the Linux. The header
  files are checked too.
 
  Signed-off-by: Michal Privoznik mpriv...@redhat.com
  ---
   configure.ac   |  4 ++--
   src/util/virfile.c | 21 ++---
   2 files changed, 16 insertions(+), 9 deletions(-)
 
 
   
   
  -#ifdef __linux__
  +#ifdef HAVE_STATFS
   
   # ifndef NFS_SUPER_MAGIC
   #  define NFS_SUPER_MAGIC 0x6969
  
  I'm fairly sure these constants are entirely Linux specific, so
  although you got it to compile on BSD, I don't think it'll be
  returning sensible results.
 
 Correct. FS Magic numbers are specific to Linux.  Gnulib has a
 'mountlist' module that coreutils and findutils share to try and
 portably get at file system names for non-Linux systems, but right now
 it is GPL, so we'd have to ask gnulib folks if it can be relaxed before
 libvirt could benefit from it.  Sadly, mounting of file systems is still
 an area of widely varying implementation-specific quirks, where there
 are no standard practices between systems.

I think I'd just suggest dropping this patch - it shouldn't hold up the
rest of the huge page series which we only really care about for Linux.


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 v1 4/7] virbitmap: Introduce virBitmapDoesIntersect

[Daniel P. Berrange]

On Mon, Jul 21, 2014 at 10:09:56AM -0600, Eric Blake wrote:
 On 07/17/2014 10:12 AM, Michal Privoznik wrote:
  This internal API just checks if two bitmaps intersect or not.
  
  Signed-off-by: Michal Privoznik mpriv...@redhat.com
  ---
   src/libvirt_private.syms |  1 +
   src/util/virbitmap.c | 20 
   src/util/virbitmap.h |  3 +++
   tests/virbitmaptest.c| 26 ++
   4 files changed, 50 insertions(+)
  
  diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
  index 44403fd..256edd5 100644
  --- a/src/libvirt_private.syms
  +++ b/src/libvirt_private.syms
  @@ -1016,6 +1016,7 @@ virBitmapClearBit;
   virBitmapCopy;
   virBitmapCountBits;
   virBitmapDataToString;
  +virBitmapDoesIntersect;
 
 The naming sounds awkward.  Maybe virBitmapIntersects or
 virBitmapHasIntersection would be better.

Perhaps even   virBitmapOverlaps is a bit easier to say ?

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] [PATCHv2 2/2] qemu: snapshot: Forbid taking/reverting snapshots in PMSUSPENDED state

[Peter Krempa]

On 07/21/14 20:44, Eric Blake wrote:
 On 07/21/2014 08:08 AM, Peter Krempa wrote:
 Qemu doesn't currently support them and behaves strangely. Just forbid
 them.

 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1079162
 ---
  src/qemu/qemu_driver.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 91baa7d..eae23d3 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -13401,9 +13401,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
  case VIR_DOMAIN_SHUTDOWN:
  case VIR_DOMAIN_SHUTOFF:
  case VIR_DOMAIN_CRASHED:
 -case VIR_DOMAIN_PMSUSPENDED:
  break;

 +case VIR_DOMAIN_PMSUSPENDED:
 +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
 +   _(qemu doesn't support taking snapshots of 
 + PMSUSPENDED guests));
 +goto cleanup;
 +
 
 This is a bit harsh; a bit nicer might be to take the snapshot anyways
 but to have the snapshot be in the running state (as if the act of
 taking the snapshot caused a pmwakeup event).  But that's more
 complicated, and changes domain state implicitly, while this approach is
 vocal to the user why we can't do it (and if qemu ever DOES add support,
 we just gate this code by a capability detection bit).

Well that would be a bit more wrong in my eyes than allowing the
snapshot and leaving it in a crippled state until qemu fixes it's part.

We would need to introduce a lot of code that would handle the
transitions and I doubt that it would get much use. Same as I think that
after qemu possibly fixes the problem it will be forgotten for a long time.

 
 ACK.
 

Anyways, I'll push this patch now as I don't think it's worth spending
more time on this.

Peter



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

[libvirt] [PATCH] tests: Remove stale scsihostdata dir

[Michal Privoznik]

In the fbd91d49 commit, new scsihostdata dir is added to EXTRA_DIST in
the tests/Makefile.am. However, the directory itself is not created
anywhere, nor in the commit.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---

Notes:
Pushed under build-breaker rule.

 tests/Makefile.am | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index ecb2f34..3e71069 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -83,7 +83,6 @@ EXTRA_DIST =  \
domainsnapshotxml2xmlin \
domainsnapshotxml2xmlout \
fchostdata \
-   scsihostdata \
interfaceschemadata \
lxcconf2xmldata \
lxcxml2xmldata \
-- 
1.8.5.5

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

[libvirt] [PATCHv1.5 2/8] storage: Add witness for checking storage volume use in security driver

[Peter Krempa]

With my intended use of storage driver assist to chown files on remote
storage we will need a witness that will tell us whether the given
storage volume supports operations needed by the storage driver.
---
 src/storage/storage_driver.c | 30 ++
 src/storage/storage_driver.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index efd79b9..b397c6e 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2357,6 +2357,36 @@ 
virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
backend-storageFileAccess;
 }

+
+/**
+ * virStorageFileSupportsSecurityDriver:
+ *
+ * @src: a storage file structure
+ *
+ * Check if a storage file supports operations needed by the security
+ * driver to perform labelling */
+bool
+virStorageFileSupportsSecurityDriver(virStorageSourcePtr src)
+{
+int actualType = virStorageSourceGetActualType(src);
+virStorageFileBackendPtr backend;
+
+if (!src)
+return false;
+
+if (src-drv) {
+backend = src-drv-backend;
+} else {
+if (!(backend = virStorageFileBackendForTypeInternal(actualType,
+ src-protocol,
+ false)))
+return false;
+}
+
+return !!backend-storageFileChown;
+}
+
+
 void
 virStorageFileDeinit(virStorageSourcePtr src)
 {
diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h
index eefd766..9592dd8 100644
--- a/src/storage/storage_driver.h
+++ b/src/storage/storage_driver.h
@@ -45,6 +45,8 @@ const char 
*virStorageFileGetUniqueIdentifier(virStorageSourcePtr src);
 int virStorageFileAccess(virStorageSourcePtr src, int mode);
 int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid);

+bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src);
+
 int virStorageFileGetMetadata(virStorageSourcePtr src,
   uid_t uid, gid_t gid,
   bool allow_probe)
-- 
2.0.0

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

[libvirt] [PATCHv1.5 0/8] Gluster security driver support and snapshot file pre-creation

[Peter Krempa]

I'm posting a rebased version as John notified me that the original series 
cannot be applied cleanly any more.

Peter Krempa (8):
  storage: Implement storage driver helper to chown disk images
  storage: Add witness for checking storage volume use in security
driver
  security: DAC: Remove superfluous link resolution
  security: DAC: Introduce callback to perform image chown
  security: DAC: Plumb usage of chown callback
  qemu: Implement DAC driver chown callback to co-operate with storage
drv
  storage: Implement virStorageFileCreate for local and gluster files
  qemu: snapshot: Use storage driver to pre-create snapshot file

 src/qemu/qemu_driver.c|  66 
 src/security/security_dac.c   | 109 +++---
 src/security/security_dac.h   |   3 +
 src/security/security_manager.c   |   4 +-
 src/security/security_manager.h   |  19 +-
 src/storage/storage_backend.h |   6 ++
 src/storage/storage_backend_fs.c  |  29 +
 src/storage/storage_backend_gluster.c |  27 +
 src/storage/storage_driver.c  |  58 ++
 src/storage/storage_driver.h  |   3 +
 10 files changed, 277 insertions(+), 47 deletions(-)

-- 
2.0.0

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

[libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

[Peter Krempa]

Use the callback to set disk and storage image labels by modifying the
existing functions and adding wrappers to avoid refactoring a lot of the
code.
---
 src/security/security_dac.c | 89 +++--
 1 file changed, 69 insertions(+), 20 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 1fb0c86..989329f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
 }

 static int
-virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
+   virStorageSourcePtr src,
+   const char *path,
+   uid_t uid,
+   gid_t gid)
 {
+int rc;
+int chown_errno;
+
 VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
- path, (long) uid, (long) gid);
+ NULLSTR(src ? src-path : path), (long) uid, (long) gid);
+
+if (priv  src  priv-chownCallback) {
+rc = priv-chownCallback(src, uid, gid);
+
+/* on -2 returned an error was already reported */
+if (rc == -2)
+return -1;

-if (chown(path, uid, gid)  0) {
+/* on -1 only errno was set */
+chown_errno = errno;
+} else {
 struct stat sb;
-int chown_errno = errno;

-if (stat(path, sb) = 0) {
+if (!path) {
+if (!src || !src-path)
+return 0;
+
+if (!virStorageSourceIsLocalStorage(src))
+return 0;
+
+path = src-path;
+}
+
+rc = chown(path, uid, gid);
+chown_errno = errno;
+
+if (rc  0 
+stat(path, sb) = 0) {
 if (sb.st_uid == uid 
 sb.st_gid == gid) {
 /* It's alright, there's nothing to change anyway. */
 return 0;
 }
 }
+}

+if (rc  0) {
 if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
 VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
  supported by filesystem,
@@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
gid_t gid)
 return 0;
 }

+
 static int
-virSecurityDACRestoreSecurityFileLabel(const char *path)
+virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+{
+return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
+}
+
+
+static int
+virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
+   virStorageSourcePtr src,
+   const char *path)
 {
-VIR_INFO(Restoring DAC user and group on '%s', path);
+VIR_INFO(Restoring DAC user and group on '%s',
+ NULLSTR(src ? src-path : path));

 /* XXX record previous ownership */
-return virSecurityDACSetOwnership(path, 0, 0);
+return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
+}
+
+
+static int
+virSecurityDACRestoreSecurityFileLabel(const char *path)
+{
+return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
 }


@@ -294,10 +343,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
mgr,
 if (!priv-dynamicOwnership)
 return 0;

-/* XXX: Add support for gluster DAC permissions */
-if (!src-path || !virStorageSourceIsLocalStorage(src))
-return 0;
-
 secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
 if (secdef  !secdef-relabel)
 return 0;
@@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
mgr,
 return -1;
 }

-return virSecurityDACSetOwnership(src-path, user, group);
+return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
 }


@@ -349,9 +394,6 @@ 
virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
 if (!priv-dynamicOwnership)
 return 0;

-if (!src-path || !virStorageSourceIsLocalStorage(src))
-return 0;
-
 /* Don't restore labels on readoly/shared disks, because other VMs may
  * still be accessing these. Alternatively we could iterate over all
  * running domains and try to figure out if it is in use, but this would
@@ -373,9 +415,16 @@ 
virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
  * ownership, because that kills access on the destination host which is
  * sub-optimal for the guest VM's I/O attempts :-) */
 if (migrated) {
-int rc = virFileIsSharedFS(src-path);
-if (rc  0)
-return -1;
+int rc = 1;
+
+if (virStorageSourceIsLocalStorage(src)) {
+if (!src-path)
+return 0;
+
+if ((rc = virFileIsSharedFS(src-path))  0)
+return -1;
+}

[libvirt] [PATCHv1.5 3/8] security: DAC: Remove superfluous link resolution

[Peter Krempa]

When restoring security labels in the dac driver the code would resolve
the file path and use the resolved one to be chown-ed. The setting code
doesn't do that. Remove the unnecessary code.
---
 src/security/security_dac.c | 19 +--
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4d2a9d6..cdb2735 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -264,27 +264,10 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
gid_t gid)
 static int
 virSecurityDACRestoreSecurityFileLabel(const char *path)
 {
-struct stat buf;
-int rc = -1;
-char *newpath = NULL;
-
 VIR_INFO(Restoring DAC user and group on '%s', path);

-if (virFileResolveLink(path, newpath)  0) {
-virReportSystemError(errno,
- _(cannot resolve symlink %s), path);
-goto err;
-}
-
-if (stat(newpath, buf) != 0)
-goto err;
-
 /* XXX record previous ownership */
-rc = virSecurityDACSetOwnership(newpath, 0, 0);
-
- err:
-VIR_FREE(newpath);
-return rc;
+return virSecurityDACSetOwnership(path, 0, 0);
 }


-- 
2.0.0

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

[libvirt] [PATCHv1.5 6/8] qemu: Implement DAC driver chown callback to co-operate with storage drv

[Peter Krempa]

Use the storage driver to chown remote images.
---
 src/qemu/qemu_driver.c | 48 +++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a5a9e0f..c0e6485 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -327,6 +327,52 @@ qemuAutostartDomains(virQEMUDriverPtr driver)
 virObjectUnref(cfg);
 }

+
+static int
+qemuSecurityChownCallback(virStorageSourcePtr src,
+  uid_t uid,
+  gid_t gid)
+{
+struct stat sb;
+int save_errno = 0;
+int ret = -1;
+
+if (!virStorageFileSupportsSecurityDriver(src))
+return 0;
+
+if (virStorageSourceIsLocalStorage(src)) {
+/* use direct chmod for local files so that the file doesn't
+ * need to be initialized */
+if (stat(src-path, sb) = 0) {
+if (sb.st_uid == uid 
+sb.st_gid == gid) {
+/* It's alright, there's nothing to change anyway. */
+return 0;
+}
+}
+
+return chown(src-path, uid, gid);
+}
+
+/* storage file init reports errors, return -2 on failure */
+if (virStorageFileInit(src)  0)
+return -2;
+
+if (virStorageFileChown(src, uid, gid)  0) {
+save_errno = errno;
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virStorageFileDeinit(src);
+errno = save_errno;
+
+return ret;
+}
+
+
 static int
 qemuSecurityInit(virQEMUDriverPtr driver)
 {
@@ -375,7 +421,7 @@ qemuSecurityInit(virQEMUDriverPtr driver)
  cfg-securityDefaultConfined,
  cfg-securityRequireConfined,
  cfg-dynamicOwnership,
- NULL)))
+ qemuSecurityChownCallback)))
 goto error;
 if (!stack) {
 if (!(stack = virSecurityManagerNewStack(mgr)))
-- 
2.0.0

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

[libvirt] [PATCHv1.5 8/8] qemu: snapshot: Use storage driver to pre-create snapshot file

[Peter Krempa]

Move the last operation done on local files to the storage driver API.
---
 src/qemu/qemu_driver.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c0e6485..f0cf1c6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12855,7 +12855,6 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 char *source = NULL;
 const char *formatStr = NULL;
 int ret = -1;
-int fd = -1;
 bool need_unlink = false;

 if (snap-snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
@@ -12873,7 +12872,7 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 if (virStorageSourceInitChainElement(newDiskSrc, disk-src, false)  0)
 goto cleanup;

-if (virStorageFileInit(newDiskSrc)  0)
+if (qemuDomainStorageFileInit(driver, vm, newDiskSrc)  0)
 goto cleanup;

 if (qemuGetDriveSourceString(newDiskSrc, NULL, source)  0)
@@ -12889,15 +12888,13 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 }

 /* pre-create the image file so that we can label it before handing it to 
qemu */
-/* XXX we should switch to storage driver based pre-creation of the image 
*/
-if (virStorageSourceIsLocalStorage(newDiskSrc)) {
-if (!reuse  newDiskSrc-type != VIR_STORAGE_TYPE_BLOCK) {
-fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT,
-  need_unlink, NULL);
-if (fd  0)
-goto cleanup;
-VIR_FORCE_CLOSE(fd);
+if (!reuse  newDiskSrc-type != VIR_STORAGE_TYPE_BLOCK) {
+if (virStorageFileCreate(newDiskSrc)  0) {
+virReportSystemError(errno, _(failed to create image file '%s'),
+ source);
+goto cleanup;
 }
+need_unlink = true;
 }

 /* set correct security, cgroup and locking options on the new image */
-- 
2.0.0

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

[libvirt] [PATCHv1.5 7/8] storage: Implement virStorageFileCreate for local and gluster files

[Peter Krempa]

Add backends for this frontend function so that we can use it in the
snapshot creation code.
---
 src/storage/storage_backend_fs.c  | 17 +
 src/storage/storage_backend_gluster.c | 15 +++
 2 files changed, 32 insertions(+)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 2af5ab5..378c553 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1387,6 +1387,22 @@ virStorageFileBackendFileInit(virStorageSourcePtr src)


 static int
+virStorageFileBackendFileCreate(virStorageSourcePtr src)
+{
+int fd = -1;
+
+if ((fd = virFileOpenAs(src-path, O_WRONLY | O_TRUNC | O_CREAT, 0,
+src-drv-uid, src-drv-gid, 0))  0) {
+errno = -fd;
+return -1;
+}
+
+VIR_FORCE_CLOSE(fd);
+return 0;
+}
+
+
+static int
 virStorageFileBackendFileUnlink(virStorageSourcePtr src)
 {
 return unlink(src-path);
@@ -1470,6 +1486,7 @@ virStorageFileBackend virStorageFileBackendFile = {
 .backendInit = virStorageFileBackendFileInit,
 .backendDeinit = virStorageFileBackendFileDeinit,

+.storageFileCreate = virStorageFileBackendFileCreate,
 .storageFileUnlink = virStorageFileBackendFileUnlink,
 .storageFileStat = virStorageFileBackendFileStat,
 .storageFileReadHeader = virStorageFileBackendFileReadHeader,
diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index 052f58d..38d02ac 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -634,6 +634,20 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)


 static int
+virStorageFileBackendGlusterCreate(virStorageSourcePtr src)
+{
+virStorageFileBackendGlusterPrivPtr priv = src-drv-priv;
+glfs_fd_t *fd = NULL;
+
+if (!(fd = glfs_open(priv-vol, src-path, O_CREAT | O_TRUNC | O_WRONLY)))
+return -1;
+
+ignore_value(glfs_close(fd));
+return 0;
+}
+
+
+static int
 virStorageFileBackendGlusterUnlink(virStorageSourcePtr src)
 {
 virStorageFileBackendGlusterPrivPtr priv = src-drv-priv;
@@ -780,6 +794,7 @@ virStorageFileBackend virStorageFileBackendGluster = {
 .backendInit = virStorageFileBackendGlusterInit,
 .backendDeinit = virStorageFileBackendGlusterDeinit,

+.storageFileCreate = virStorageFileBackendGlusterCreate,
 .storageFileUnlink = virStorageFileBackendGlusterUnlink,
 .storageFileStat = virStorageFileBackendGlusterStat,
 .storageFileReadHeader = virStorageFileBackendGlusterReadHeader,
-- 
2.0.0

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

[libvirt] [PATCHv1.5 1/8] storage: Implement storage driver helper to chown disk images

[Peter Krempa]

Gluster storage works on a similar principle to NFS where it takes the
uid and gid of the actual process and uses it to access the storage
volume on the remote server. This introduces a need to chown storage
files on gluster via native API.
---
 src/storage/storage_backend.h |  6 ++
 src/storage/storage_backend_fs.c  | 12 
 src/storage/storage_backend_gluster.c | 12 
 src/storage/storage_driver.c  | 28 
 src/storage/storage_driver.h  |  1 +
 5 files changed, 59 insertions(+)

diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index e48da5b..a93df5d 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -244,6 +244,11 @@ typedef int
 (*virStorageFileBackendAccess)(virStorageSourcePtr src,
int mode);

+typedef int
+(*virStorageFileBackendChown)(virStorageSourcePtr src,
+  uid_t uid,
+  gid_t gid);
+
 virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol);
 virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type,
   int protocol,
@@ -269,6 +274,7 @@ struct _virStorageFileBackend {
 virStorageFileBackendUnlink storageFileUnlink;
 virStorageFileBackendStat   storageFileStat;
 virStorageFileBackendAccess storageFileAccess;
+virStorageFileBackendChown  storageFileChown;
 };

 #endif /* __VIR_STORAGE_BACKEND_H__ */
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 40cc1f4..2af5ab5 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1455,6 +1455,15 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src,
 }


+static int
+virStorageFileBackendFileChown(virStorageSourcePtr src,
+   uid_t uid,
+   gid_t gid)
+{
+return chown(src-path, uid, gid);
+}
+
+
 virStorageFileBackend virStorageFileBackendFile = {
 .type = VIR_STORAGE_TYPE_FILE,

@@ -1465,6 +1474,7 @@ virStorageFileBackend virStorageFileBackendFile = {
 .storageFileStat = virStorageFileBackendFileStat,
 .storageFileReadHeader = virStorageFileBackendFileReadHeader,
 .storageFileAccess = virStorageFileBackendFileAccess,
+.storageFileChown = virStorageFileBackendFileChown,

 .storageFileGetUniqueIdentifier = 
virStorageFileBackendFileGetUniqueIdentifier,
 };
@@ -1479,6 +1489,7 @@ virStorageFileBackend virStorageFileBackendBlock = {
 .storageFileStat = virStorageFileBackendFileStat,
 .storageFileReadHeader = virStorageFileBackendFileReadHeader,
 .storageFileAccess = virStorageFileBackendFileAccess,
+.storageFileChown = virStorageFileBackendFileChown,

 .storageFileGetUniqueIdentifier = 
virStorageFileBackendFileGetUniqueIdentifier,
 };
@@ -1491,6 +1502,7 @@ virStorageFileBackend virStorageFileBackendDir = {
 .backendDeinit = virStorageFileBackendFileDeinit,

 .storageFileAccess = virStorageFileBackendFileAccess,
+.storageFileChown = virStorageFileBackendFileChown,

 .storageFileGetUniqueIdentifier = 
virStorageFileBackendFileGetUniqueIdentifier,
 };
diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index 0f8f0f3..052f58d 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -762,6 +762,17 @@ 
virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src)
 }


+static int
+virStorageFileBackendGlusterChown(virStorageSourcePtr src,
+  uid_t uid,
+  gid_t gid)
+{
+virStorageFileBackendGlusterPrivPtr priv = src-drv-priv;
+
+return glfs_chown(priv-vol, src-path, uid, gid);
+}
+
+
 virStorageFileBackend virStorageFileBackendGluster = {
 .type = VIR_STORAGE_TYPE_NETWORK,
 .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER,
@@ -773,6 +784,7 @@ virStorageFileBackend virStorageFileBackendGluster = {
 .storageFileStat = virStorageFileBackendGlusterStat,
 .storageFileReadHeader = virStorageFileBackendGlusterReadHeader,
 .storageFileAccess = virStorageFileBackendGlusterAccess,
+.storageFileChown = virStorageFileBackendGlusterChown,

 .storageFileGetUniqueIdentifier = 
virStorageFileBackendGlusterGetUniqueIdentifier,

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 3830867..efd79b9 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2613,6 +2613,34 @@ virStorageFileAccess(virStorageSourcePtr src,
 }


+/**
+ * virStorageFileChown: Change owner of a storage file
+ *
+ * @src: storage file to change owner of
+ * @uid: new owner id
+ * @gid: new group id
+ *
+ * Returns 0 on success, -1 on error and sets errno. No libvirt
+ * error is reported. Returns -2 if the operation isn't supported
+ * by libvirt storage 

[libvirt] [PATCHv1.5 4/8] security: DAC: Introduce callback to perform image chown

[Peter Krempa]

To integrate the security driver with the storage driver we need to
pass a callback for a function that will chown storage volumes.

Introduce and document the callback prototype.
---
 src/qemu/qemu_driver.c  |  3 ++-
 src/security/security_dac.c |  9 +
 src/security/security_dac.h |  3 +++
 src/security/security_manager.c |  4 +++-
 src/security/security_manager.h | 19 ++-
 5 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index eae23d3..a5a9e0f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -374,7 +374,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
  cfg-allowDiskFormatProbing,
  cfg-securityDefaultConfined,
  cfg-securityRequireConfined,
- cfg-dynamicOwnership)))
+ cfg-dynamicOwnership,
+ NULL)))
 goto error;
 if (!stack) {
 if (!(stack = virSecurityManagerNewStack(mgr)))
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cdb2735..1fb0c86 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -51,6 +51,7 @@ struct _virSecurityDACData {
 int ngroups;
 bool dynamicOwnership;
 char *baselabel;
+virSecurityManagerDACChownCallback chownCallback;
 };

 typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData;
@@ -87,6 +88,14 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
 priv-dynamicOwnership = dynamicOwnership;
 }

+void
+virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
+   virSecurityManagerDACChownCallback 
chownCallback)
+{
+virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+priv-chownCallback = chownCallback;
+}
+
 /* returns 1 if label isn't found, 0 on success, -1 on error */
 static int
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
diff --git a/src/security/security_dac.h b/src/security/security_dac.h
index dbcf56f..846cefb 100644
--- a/src/security/security_dac.h
+++ b/src/security/security_dac.h
@@ -32,4 +32,7 @@ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr,
 void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
bool dynamic);

+void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
+virSecurityManagerDACChownCallback 
chownCallback);
+
 #endif /* __VIR_SECURITY_DAC */
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 8a45e04..8671620 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -152,7 +152,8 @@ virSecurityManagerNewDAC(const char *virtDriver,
  bool allowDiskFormatProbing,
  bool defaultConfined,
  bool requireConfined,
- bool dynamicOwnership)
+ bool dynamicOwnership,
+ virSecurityManagerDACChownCallback chownCallback)
 {
 virSecurityManagerPtr mgr =
 virSecurityManagerNewDriver(virSecurityDriverDAC,
@@ -170,6 +171,7 @@ virSecurityManagerNewDAC(const char *virtDriver,
 }

 virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership);
+virSecurityDACSetChownCallback(mgr, chownCallback);

 return mgr;
 }
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 97b6a2e..156f882 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -25,6 +25,7 @@

 # include domain_conf.h
 # include vircommand.h
+# include virstoragefile.h

 typedef struct _virSecurityManager virSecurityManager;
 typedef virSecurityManager *virSecurityManagerPtr;
@@ -39,13 +40,29 @@ virSecurityManagerPtr 
virSecurityManagerNewStack(virSecurityManagerPtr primary);
 int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
  virSecurityManagerPtr nested);

+/**
+ * virSecurityManagerDACChownCallback:
+ * @src: Storage file to chown
+ * @uid: target uid
+ * @gid: target gid
+ *
+ * A function callback to chown image files described by the disk source struct
+ * @src. The callback shall return 0 on success, -1 on error and errno set (no
+ * libvirt error reported) OR -2 and a libvirt error reported. */
+typedef int
+(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src,
+  uid_t uid,
+  gid_t gid);
+
+
 virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
uid_t user,
gid_t group,

Re: [libvirt] [Qemu-devel] ipv6 slirp network

[Vasiliy Tolstov]

2014-07-22 0:49 GMT+04:00 Samuel Thibault samuel.thiba...@gnu.org:
 Alternatively, you can track the parameters branch, which I don't
 regenerate.


Thanks. Now all works fine.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCH v2 8/8] rpc: pass listen FD to the daemon being started

[Martin Kletzander]

On Wed, Jul 16, 2014 at 08:30:02PM +0200, Martin Kletzander wrote:

This eliminates the need for active waiting.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/rpc/virnetsocket.c | 58 +-
1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index a94b2bc..c00209c 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c

[...]

@@ -569,28 +572,45 @@ int virNetSocketNewConnectUNIX(const char *path,

[...]

+/*
+ * We cannot do the umask() trick here because that's not
+ * thread-safe.  fchmod(), however, is not guaranteed to work on
+ * some BSD favours, but *should* work on Linux before the socket
+ * is bound.  POSIX says the behaviour of fchmod() called on
+ * socket is unspecified, though.
+ */
+if (fchmod(passfd, 0700)  0) {
+virReportSystemError(errno, %s,
+ _(Failed to change permissions on socket));
+goto error;
}



I've finally found a way out of this.  We can fork() and in the child
do only umask() and bind().  It shouldn't be a problem that fstat()
returns different mode for the socket than stat(), it should work
everywhere and thanks to the fact that we do this pretty rarely and
copy-on-write pages there shouldn't be significant impact.

Is this acceptable?

Martin


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

Re: [libvirt] [PATCH v2 0/8] Speed up waiting for the session daemon

[Daniel P. Berrange]

On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:
 This is complete rework of:
 
 http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html
 
 where Daniel suggested we use systemd-like passing of socket fd in
 combination with the LISTEN_FDS environment variable:
 
 http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html
 
 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369

Obviously that bug is focused on starting of the session daemon,
but the code we're adding here should work with systemd. Have you
tested that this actually allows for systemd activation of the
privileged libvirtd. If we're adding this FD passing, I think we
really ought to make sure we support this, so we don't have to
revisit it later. Should add a libvirtd.socket unit file too
so we have systemd activation by default for libvirtd.

NB, we stil need to enable the daemon by default anyway since
libvirtd needs todo autostart of VMs, but having the socket
activation too avoids some race conditions with startup.


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 2/2] blockjob: properly track blockcopy xml changes on disk

[Eric Blake]

On 07/21/2014 11:38 PM, chen.fan.f...@cn.fujitsu.com wrote:
 On Mon, 2014-07-21 at 22:34 -0600, Eric Blake wrote: 
 We were not directly saving the domain XML to file after starting
 or finishing a blockcopy.  Without the startup write, a libvirtd
 restart in the middle of a copy job would forget that the job was
 underway.  Then at pivot, we were indirectly writing new XML in
 reaction to events that occur as we stop and restart the guest CPUs.
 But there was a race: since pivot is an async action, it is possible
 that the guest is restarted before the pivot completes, so if XML
 changes during the event, that change was not written.  The original
 blockcopy code cleared out the mirror element prior to restarting
 the CPUs, but this is also a race, observed if a user does an async
 pivot and a dumpxml before the event occurs.  Furthermore, this race
 will interfere with active commit, because that code relies on the
 mirror element at the time of the qemu event to determine whether
 to inform the user of a normal commit or an active commit.

 +++ b/src/qemu/qemu_process.c
 @@ -1016,6 +1016,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon 
 ATTRIBUTE_UNUSED,
  virObjectEventPtr event2 = NULL;
  const char *path;
  virDomainDiskDefPtr disk;
 +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
 Hello Eric,
The cfg should be unref by virObjectUnref(cfg) at the end.

You're right.  I also discovered I need to write state after a normal
abort. I'll post an updated version, along with a rebase of my patches
to enable active commit.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv1.5 1/8] storage: Implement storage driver helper to chown disk images

[Eric Blake]

On 07/22/2014 03:20 AM, Peter Krempa wrote:
 Gluster storage works on a similar principle to NFS where it takes the
 uid and gid of the actual process and uses it to access the storage
 volume on the remote server. This introduces a need to chown storage
 files on gluster via native API.
 ---
  src/storage/storage_backend.h |  6 ++
  src/storage/storage_backend_fs.c  | 12 
  src/storage/storage_backend_gluster.c | 12 
  src/storage/storage_driver.c  | 28 
  src/storage/storage_driver.h  |  1 +
  5 files changed, 59 insertions(+)
 

ACK

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv1.5 2/8] storage: Add witness for checking storage volume use in security driver

[Eric Blake]

On 07/22/2014 03:20 AM, Peter Krempa wrote:
 With my intended use of storage driver assist to chown files on remote
 storage we will need a witness that will tell us whether the given
 storage volume supports operations needed by the storage driver.
 ---
  src/storage/storage_driver.c | 30 ++
  src/storage/storage_driver.h |  2 ++
  2 files changed, 32 insertions(+)

ACK.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv1.5 3/8] security: DAC: Remove superfluous link resolution

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
 When restoring security labels in the dac driver the code would resolve
 the file path and use the resolved one to be chown-ed. The setting code
 doesn't do that. Remove the unnecessary code.
 ---
  src/security/security_dac.c | 19 +--
  1 file changed, 1 insertion(+), 18 deletions(-)
 

Code has been there since it was first introduced by commit id
'15f5eaa0'.  I see the security_selinux.c does the same thing and it
predates security_dac.c... Digging deeper finds commit id 'c86afc85e'
which added searching for the path for some unknown/unlogged reason.

One can only wonder why it was felt the restore needed link resolution,
but yet not necessary on the set.

While the Set functions don't necessarily have it, looking at all
callers of virFileResolveLink() causes me to believe there was some
reason (or some path) that perhaps didn't store the Resolved link and
this code is just ensuring that.

Perhaps Eric or DanB could chime in on this as the restore is a fairly
generic path, while the set paths are usually fairly specific.

Not 100% comfortable with giving an ACK


John


 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index 4d2a9d6..cdb2735 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -264,27 +264,10 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
 gid_t gid)
  static int
  virSecurityDACRestoreSecurityFileLabel(const char *path)
  {
 -struct stat buf;
 -int rc = -1;
 -char *newpath = NULL;
 -
  VIR_INFO(Restoring DAC user and group on '%s', path);
 
 -if (virFileResolveLink(path, newpath)  0) {
 -virReportSystemError(errno,
 - _(cannot resolve symlink %s), path);
 -goto err;
 -}
 -
 -if (stat(newpath, buf) != 0)
 -goto err;
 -
  /* XXX record previous ownership */
 -rc = virSecurityDACSetOwnership(newpath, 0, 0);
 -
 - err:
 -VIR_FREE(newpath);
 -return rc;
 +return virSecurityDACSetOwnership(path, 0, 0);
  }
 
 

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

Re: [libvirt] [PATCHv1.5 2/8] storage: Add witness for checking storage volume use in security driver

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
 With my intended use of storage driver assist to chown files on remote
 storage we will need a witness that will tell us whether the given
 storage volume supports operations needed by the storage driver.
 ---
  src/storage/storage_driver.c | 30 ++
  src/storage/storage_driver.h |  2 ++
  2 files changed, 32 insertions(+)
 
 diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
 index efd79b9..b397c6e 100644
 --- a/src/storage/storage_driver.c
 +++ b/src/storage/storage_driver.c
 @@ -2357,6 +2357,36 @@ 
 virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
 backend-storageFileAccess;
  }
 
 +
 +/**
 + * virStorageFileSupportsSecurityDriver:
 + *
 + * @src: a storage file structure
 + *
 + * Check if a storage file supports operations needed by the security
 + * driver to perform labelling */

NIT: Closing comment on new line.


ACK

John

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

Re: [libvirt] [PATCHv1.5 1/8] storage: Implement storage driver helper to chown disk images

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
 Gluster storage works on a similar principle to NFS where it takes the
 uid and gid of the actual process and uses it to access the storage
 volume on the remote server. This introduces a need to chown storage
 files on gluster via native API.
 ---
  src/storage/storage_backend.h |  6 ++
  src/storage/storage_backend_fs.c  | 12 
  src/storage/storage_backend_gluster.c | 12 
  src/storage/storage_driver.c  | 28 
  src/storage/storage_driver.h  |  1 +
  5 files changed, 59 insertions(+)
 

ACK

John

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

Re: [libvirt] [PATCH v2 0/8] Speed up waiting for the session daemon

[Martin Kletzander]

On Tue, Jul 22, 2014 at 01:36:56PM +0100, Daniel P. Berrange wrote:

On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:

This is complete rework of:

http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html

where Daniel suggested we use systemd-like passing of socket fd in
combination with the LISTEN_FDS environment variable:

http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369


Obviously that bug is focused on starting of the session daemon,
but the code we're adding here should work with systemd. Have you
tested that this actually allows for systemd activation of the
privileged libvirtd. If we're adding this FD passing, I think we
really ought to make sure we support this, so we don't have to
revisit it later. Should add a libvirtd.socket unit file too
so we have systemd activation by default for libvirtd.



Obviously I haven't.  I just wanted to get rid of that silly, silly
bug.


NB, we stil need to enable the daemon by default anyway since
libvirtd needs todo autostart of VMs, but having the socket
activation too avoids some race conditions with startup.



Yes, that's one of the reasons why I think it will create more
confusion than races it will resolve.

I'll _try_ to work this in, but how would you suggest to set up the
initial permissions?  Anything the user will change in libvirtd.conf
he will also have to change in the libvirt.socket file, because
someone might use the filesystem-level permission checking for
isolating some users (or anything else) because we don't want to break
that.



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 :|


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

Re: [libvirt] [PATCH v2] leaseshelper: improvements to support all events

[Peter Krempa]

On 07/22/14 01:03, Nehal J Wani wrote:
 This patch enables the helper program to detect event(s) triggered when there
 is a change in lease length or expiry and client-id. This transfers complete
 control of leases database to libvirt and obsoletes use of the lease database
 file (network-name.leases). That file will not be created, read, or written.
 This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
 custom env var to leaseshelper, which helps us map events related to leases
 with their corresponding network bridges, no matter what the event be.
 
 Also, this requires the addition of a new non-lease entry in our custom lease
 database: server-duid. It is required to identify a DHCPv6 server.
 
 Now that dnsmasq doesn't maintain its own leases database, it relies on our
 helper program to tell it about previous leases and server duid. Thus, this
 patch makes our leases program honor an extra action: init, in which it 
 sends
 the known info in a particular format to dnsmasq by printing it to stdout.
 
 ---
  This is compatible with libvirt 1.2.6 as only additions have been
  introduced, and the old leases file (*.staus) will still be supported.
 
  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
 
  src/network/bridge_driver.c |   7 ++
  src/network/leaseshelper.c  | 152 
 +---
  2 files changed, 136 insertions(+), 23 deletions(-)
 
 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 6a2e760..4363cd8 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -1289,7 +1289,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
 network,
  
  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
  virCommandAddArgFormat(cmd, --conf-file=%s, configfile);
 +/* Libvirt gains full control of leases database */
 +virCommandAddArgFormat(cmd, --leasefile-ro);
  virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path);
 +virCommandAddEnvPair(cmd, VIR_BRIDGE_NAME, network-def-bridge);

The invocation is now much nicer!

  
  *cmdout = cmd;
  ret = 0;
 @@ -3432,6 +3435,10 @@ networkGetDHCPLeases(virNetworkPtr network,
  goto error;
  }
  
 +/* Ignore server-duid. It's not part of a lease */
 +if (virJSONValueObjectHasKey(lease_tmp, server-duid))
 +continue;

[1]

 +
  if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp, 
 mac-address))) {
  /* leaseshelper program guarantees that lease will be stored 
 only if
   * mac-address is known otherwise not */
 diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
 index e4b5283..cc4b4ac 100644
 --- a/src/network/leaseshelper.c
 +++ b/src/network/leaseshelper.c

 @@ -89,6 +95,7 @@ enum virLeaseActionFlags {
  VIR_LEASE_ACTION_ADD,   /* Create new lease */
  VIR_LEASE_ACTION_OLD,   /* Lease already exists, renew it */
  VIR_LEASE_ACTION_DEL,   /* Delete the lease */
 +VIR_LEASE_ACTION_INIT,  /* Tell dnsmasq of existing leases on 
 restart */
  
  VIR_LEASE_ACTION_LAST
  };

 @@ -105,6 +112,7 @@ main(int argc, char **argv)
  char *pid_file = NULL;
  char *lease_entries = NULL;
  char *custom_lease_file = NULL;
 +char *server_duid = NULL;
  const char *ip = NULL;
  const char *mac = NULL;
  const char *iaid = virGetEnvAllowSUID(DNSMASQ_IAID);
 @@ -112,20 +120,26 @@ main(int argc, char **argv)
  const char *interface = virGetEnvAllowSUID(DNSMASQ_INTERFACE);
  const char *exptime_tmp = virGetEnvAllowSUID(DNSMASQ_LEASE_EXPIRES);
  const char *hostname = virGetEnvAllowSUID(DNSMASQ_SUPPLIED_HOSTNAME);
 +const char *server_duid_env = virGetEnvAllowSUID(DNSMASQ_SERVER_DUID);
  const char *leases_str = NULL;
  long long currtime = 0;
  long long expirytime = 0;
  size_t i = 0;
 +size_t count_ipv6 = 0;
 +size_t count_ipv4 = 0;
  int action = -1;
  int pid_file_fd = -1;
  int rv = EXIT_FAILURE;
  int custom_lease_file_len = 0;
  bool add = false;
 +bool init = false;
  bool delete = false;
  virJSONValuePtr lease_new = NULL;
  virJSONValuePtr lease_tmp = NULL;
  virJSONValuePtr leases_array = NULL;
  virJSONValuePtr leases_array_new = NULL;
 +virJSONValuePtr *leases_ipv4 = NULL;
 +virJSONValuePtr *leases_ipv6 = NULL;
  
  virSetErrorFunc(NULL, NULL);
  virSetErrorLogPriorityFunc(NULL);
 @@ -156,17 +170,18 @@ main(int argc, char **argv)
  }
  }
  
 -if (argc != 4  argc != 5) {
 +if (argc != 4  argc != 5  argc != 2) {
  /* Refer man page of dnsmasq --dhcp-script for more details */
  usage(EXIT_FAILURE);
  }
  
  /* Make sure dnsmasq knows the interface. The interface name is not known
 - * when dnsmasq (re)starts and throws 'del' events for expired leases.
 - * So, if any old lease has expired, it will 

Re: [libvirt] [PATCH v2 1/8] util: abstract parsing of passed FDs into virGetListenFDs()

[Martin Kletzander]

On Wed, Jul 16, 2014 at 08:29:55PM +0200, Martin Kletzander wrote:

Since not only systemd can do this (we'll be doing it as well few
patches later), change 'systemd' to 'caller' and fix LISTEN_FDS to
LISTEN_PID where applicable.

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/libvirt_private.syms  |  1 +
src/locking/lock_daemon.c | 45 -
src/util/virutil.c| 51 +++
src/util/virutil.h|  2 ++
4 files changed, 58 insertions(+), 41 deletions(-)


[...]

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 95d1ff9..6f3f411 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2227,3 +2227,54 @@ void virUpdateSelfLastChanged(const char *path)
selfLastChanged = sb.st_ctime;
}
}
+
+/*
+ * virGetListenFDs:
+ *
+ * Parse LISTEN_PID and LISTEN_FDS passed from caller.
+ *
+ * Returns number of passed FDs.
+ */
+unsigned int
+virGetListenFDs(void)
+{
+const char *pidstr;
+const char *fdstr;
+unsigned long long procid;
+unsigned int nfds;
+
+VIR_DEBUG(Setting up networking from caller);
+
+if (!(pidstr = virGetEnvAllowSUID(LISTEN_PID))) {
+VIR_DEBUG(No LISTEN_PID from caller);
+return 0;
+}
+
+if (virStrToLong_ull(pidstr, NULL, 10, procid)  0) {
+VIR_DEBUG(Malformed LISTEN_PID from caller %s, pidstr);
+return 0;
+}
+
+if ((pid_t)procid != getpid()) {
+VIR_DEBUG(LISTEN_PID %s is not for us %llu,
+  pidstr, (unsigned long long)getpid());
+return 0;
+}
+
+if (!(fdstr = virGetEnvAllowSUID(LISTEN_FDS))) {
+VIR_DEBUG(No LISTEN_FDS from caller);
+return 0;
+}
+
+if (virStrToLong_ui(fdstr, NULL, 10, nfds)  0) {
+VIR_DEBUG(Malformed LISTEN_FDS from caller %s, fdstr);
+return 0;
+}
+
+unsetenv(LISTEN_PID);
+unsetenv(LISTEN_FDS);
+
+VIR_DEBUG(Got %u file descriptors, nfds);
+
+return nfds;
+}


Note (not just) to self (but also reviewers): This function should
probably set O_CLOEXEC flag on all the passed FDs, but I just realized
that now and am not sure whether this was already done in virtlockd or
not.

Martin


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

Re: [libvirt] [PATCH v1 5/7] domain: Introduce ./hugepages/page/[@size, @unit, @nodeset]

[Michal Privoznik]

On 21.07.2014 17:09, Daniel P. Berrange wrote:

On Thu, Jul 17, 2014 at 06:12:46PM +0200, Michal Privoznik wrote:

+domain type='qemu'
+  nameQEMUGuest1/name
+  uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
+  memory unit='KiB'4194304/memory
+  currentMemory unit='KiB'4194304/currentMemory
+  memoryBacking
+hugepages
+  page size='2048' unit='KiB' nodeset='1'/
+  page size='1048576' unit='KiB' nodeset='0,2-3'/
+/hugepages
+  /memoryBacking
+  vcpu placement='static'4/vcpu
+  numatune
+memory mode='strict' nodeset='0-3'/
+memnode cellid='3' mode='strict' nodeset='3'/
+  /numatune
+  os
+type arch='i686' machine='pc'hvm/type
+boot dev='hd'/
+  /os
+  cpu
+numa
+  cell id='0' cpus='0' memory='1048576'/
+  cell id='1' cpus='1' memory='1048576'/
+  cell id='2' cpus='2' memory='1048576'/
+  cell id='3' cpus='3' memory='1048576'/
+/numa
+  /cpu


There's nothing functionally wrong with what you have here, but I'm
wondering if you considered just adding a page size attribute against
the cell element under numa here ? Feels like that might be a bit
less verbose for the XML


Huh funny. That idea came up to my mind, but I thought it was more 
verbose so I went down this road. I'm not fundamentally against it, but 
I like my approach more. In most common case, users will use only one 
size of huge pages to back their guests, so all they need to do is:


memoryBacking
  hugepages
page size='1' unit='G'/
  /hugepages
/memoryBacking

instead of repeating @pagesize attribute in each cell/. But as far as 
I see it's just question of preference without any technical impact, right?


Michal

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

Re: [libvirt] [PATCHv1.5 4/8] security: DAC: Introduce callback to perform image chown

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
 To integrate the security driver with the storage driver we need to
 pass a callback for a function that will chown storage volumes.
 
 Introduce and document the callback prototype.


ACK

Although I'm still not sure I completely follow how or what role the
cfg-user and cfg-group 'play' or if there needs to be a
relationship with the chownCallback.


John


 ---
  src/qemu/qemu_driver.c  |  3 ++-
  src/security/security_dac.c |  9 +
  src/security/security_dac.h |  3 +++
  src/security/security_manager.c |  4 +++-
  src/security/security_manager.h | 19 ++-
  5 files changed, 35 insertions(+), 3 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index eae23d3..a5a9e0f 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -374,7 +374,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
   cfg-allowDiskFormatProbing,
   cfg-securityDefaultConfined,
   cfg-securityRequireConfined,
 - cfg-dynamicOwnership)))
 + cfg-dynamicOwnership,
 + NULL)))
  goto error;
  if (!stack) {
  if (!(stack = virSecurityManagerNewStack(mgr)))
 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index cdb2735..1fb0c86 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -51,6 +51,7 @@ struct _virSecurityDACData {
  int ngroups;
  bool dynamicOwnership;
  char *baselabel;
 +virSecurityManagerDACChownCallback chownCallback;
  };
 
  typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData;
 @@ -87,6 +88,14 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr 
 mgr,
  priv-dynamicOwnership = dynamicOwnership;
  }
 
 +void
 +virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
 +   virSecurityManagerDACChownCallback 
 chownCallback)
 +{
 +virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 +priv-chownCallback = chownCallback;
 +}
 +
  /* returns 1 if label isn't found, 0 on success, -1 on error */
  static int
  ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
 diff --git a/src/security/security_dac.h b/src/security/security_dac.h
 index dbcf56f..846cefb 100644
 --- a/src/security/security_dac.h
 +++ b/src/security/security_dac.h
 @@ -32,4 +32,7 @@ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr,
  void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
 bool dynamic);
 
 +void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
 +virSecurityManagerDACChownCallback 
 chownCallback);
 +
  #endif /* __VIR_SECURITY_DAC */
 diff --git a/src/security/security_manager.c b/src/security/security_manager.c
 index 8a45e04..8671620 100644
 --- a/src/security/security_manager.c
 +++ b/src/security/security_manager.c
 @@ -152,7 +152,8 @@ virSecurityManagerNewDAC(const char *virtDriver,
   bool allowDiskFormatProbing,
   bool defaultConfined,
   bool requireConfined,
 - bool dynamicOwnership)
 + bool dynamicOwnership,
 + virSecurityManagerDACChownCallback chownCallback)
  {
  virSecurityManagerPtr mgr =
  virSecurityManagerNewDriver(virSecurityDriverDAC,
 @@ -170,6 +171,7 @@ virSecurityManagerNewDAC(const char *virtDriver,
  }
 
  virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership);
 +virSecurityDACSetChownCallback(mgr, chownCallback);
 
  return mgr;
  }
 diff --git a/src/security/security_manager.h b/src/security/security_manager.h
 index 97b6a2e..156f882 100644
 --- a/src/security/security_manager.h
 +++ b/src/security/security_manager.h
 @@ -25,6 +25,7 @@
 
  # include domain_conf.h
  # include vircommand.h
 +# include virstoragefile.h
 
  typedef struct _virSecurityManager virSecurityManager;
  typedef virSecurityManager *virSecurityManagerPtr;
 @@ -39,13 +40,29 @@ virSecurityManagerPtr 
 virSecurityManagerNewStack(virSecurityManagerPtr primary);
  int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
   virSecurityManagerPtr nested);
 
 +/**
 + * virSecurityManagerDACChownCallback:
 + * @src: Storage file to chown
 + * @uid: target uid
 + * @gid: target gid
 + *
 + * A function callback to chown image files described by the disk source 
 struct
 + * @src. The callback shall return 0 on success, -1 on error and errno set 
 (no
 + * libvirt error reported) OR -2 and a libvirt error reported. */
 +typedef int
 +(*virSecurityManagerDACChownCallback)(virStorageSourcePtr 

Re: [libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
 Use the callback to set disk and storage image labels by modifying the
 existing functions and adding wrappers to avoid refactoring a lot of the
 code.
 ---
  src/security/security_dac.c | 89 
 +++--
  1 file changed, 69 insertions(+), 20 deletions(-)
 
 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index 1fb0c86..989329f 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
  }
 
  static int
 -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path,
 +   uid_t uid,
 +   gid_t gid)
  {
 +int rc;
 +int chown_errno;
 +
  VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
 - path, (long) uid, (long) gid);
 + NULLSTR(src ? src-path : path), (long) uid, (long) gid);
 +
 +if (priv  src  priv-chownCallback) {

This is the 'src' option and 'path' is NULL

 +rc = priv-chownCallback(src, uid, gid);
 +
 +/* on -2 returned an error was already reported */
 +if (rc == -2)
 +return -1;
 
 -if (chown(path, uid, gid)  0) {
 +/* on -1 only errno was set */
 +chown_errno = errno;

Thus rc == -1, path == NULL

 +} else {
  struct stat sb;
 -int chown_errno = errno;
 
 -if (stat(path, sb) = 0) {
 +if (!path) {
 +if (!src || !src-path)

Maybe a if !src  !path return 0 should be before the VIR_INFO...
Maybe with a VIR_DEBUG that'll help someone some day figure out why they
aren't getting what they were expecting. Just a thought...

 +return 0;
 +
 +if (!virStorageSourceIsLocalStorage(src))
 +return 0;
 +
 +path = src-path;

Reusing a passed parameter is where things get dicey for me.

 +}
 +
 +rc = chown(path, uid, gid);
 +chown_errno = errno;
 +
 +if (rc  0 
 +stat(path, sb) = 0) {
  if (sb.st_uid == uid 
  sb.st_gid == gid) {
  /* It's alright, there's nothing to change anyway. */
  return 0;
  }
  }
 +}
 
 +if (rc  0) {

I think we can get here with path == NULL and rc == -1, resulting in
subsequent usage of '%s' for path to have 'nil', right?

  if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
  VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
   supported by filesystem,
 @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
 gid_t gid)
  return 0;
  }
 
 +
  static int
 -virSecurityDACRestoreSecurityFileLabel(const char *path)
 +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +{
 +return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
 +}
 +
 +
 +static int
 +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path)
  {
 -VIR_INFO(Restoring DAC user and group on '%s', path);
 +VIR_INFO(Restoring DAC user and group on '%s',
 + NULLSTR(src ? src-path : path));
 
  /* XXX record previous ownership */
 -return virSecurityDACSetOwnership(path, 0, 0);
 +return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);

I know this just follows the previous code, but in the big picture - how
does this play in with future patches where fs  gluster will
seemingly go though this path?


ACK - in general with the 'path' issue above understood.  For this code,
I'm mostly curious.

John
 +}
 +
 +
 +static int
 +virSecurityDACRestoreSecurityFileLabel(const char *path)
 +{
 +return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
  }
 
 
 @@ -294,10 +343,6 @@ 
 virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
  if (!priv-dynamicOwnership)
  return 0;
 
 -/* XXX: Add support for gluster DAC permissions */
 -if (!src-path || !virStorageSourceIsLocalStorage(src))
 -return 0;
 -
  secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
  if (secdef  !secdef-relabel)
  return 0;
 @@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
 mgr,
  return -1;
  }
 
 -return virSecurityDACSetOwnership(src-path, user, group);
 +return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
  }
 
 
 @@ -349,9 +394,6 @@ 
 virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
  if 

[libvirt] [libvirt-python PATCH] Bump version to 1.2.7 for new dev cycle

[Martin Kletzander]

Signed-off-by: Martin Kletzander mklet...@redhat.com
---

Notes:
pretty late, but pushed as trivial

 setup.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.py b/setup.py
index 4e889da..334d7b2 100755
--- a/setup.py
+++ b/setup.py
@@ -309,7 +309,7 @@ class my_clean(clean):
 _c_modules, _py_modules = get_module_lists()

 setup(name = 'libvirt-python',
-  version = '1.2.6',
+  version = '1.2.7',
   url = 'http://www.libvirt.org',
   maintainer = 'Libvirt Maintainers',
   maintainer_email = 'libvir-list@redhat.com',
-- 
2.0.2

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

Re: [libvirt] [PATCHv1.5 7/8] storage: Implement virStorageFileCreate for local and gluster files

[John Ferlan]

On 07/22/2014 05:21 AM, Peter Krempa wrote:
 Add backends for this frontend function so that we can use it in the
 snapshot creation code.
 ---
  src/storage/storage_backend_fs.c  | 17 +
  src/storage/storage_backend_gluster.c | 15 +++
  2 files changed, 32 insertions(+)
 

I suppose this more or less answers my earlier question about how
cfg-user and cfg-group get to play...  Just have to wait long enough I
guess.

ACK

John

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

Re: [libvirt] [PATCHv1.5 6/8] qemu: Implement DAC driver chown callback to co-operate with storage drv

[John Ferlan]

On 07/22/2014 05:21 AM, Peter Krempa wrote:
 Use the storage driver to chown remote images.
 ---
  src/qemu/qemu_driver.c | 48 +++-
  1 file changed, 47 insertions(+), 1 deletion(-)
 

ACK - essentially does what virSecurityDACSetOwnership did other than
the Restore path which used to ResolveLink  stat() (e.g. patch 3)


John

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

Re: [libvirt] [PATCHv1.5 8/8] qemu: snapshot: Use storage driver to pre-create snapshot file

[John Ferlan]

On 07/22/2014 05:21 AM, Peter Krempa wrote:
 Move the last operation done on local files to the storage driver API.
 ---
  src/qemu/qemu_driver.c | 17 +++--
  1 file changed, 7 insertions(+), 10 deletions(-)
 

ACK

John

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

[libvirt] [PATCH RFC] storage: ZFS support

[Roman Bogorodskiy]

Implement ZFS storage backend driver. Currently supported
only on FreeBSD because of ZFS limitations on Linux.

Features supported:

 - pool-start, pool-stop
 - pool-info
 - vol-list
 - vol-create / vol-delete

Pool definition looks like that:

 pool type='zfs'
  namemyzfspool/name
  source
nameactualpoolname/name
  /source
 /pool

The 'actualpoolname' value is a name of the pool on the system,
such as shown by 'zpool list' command. Target makes no sense
here because volumes path is always /dev/zvol/$poolname/$volname.

Users has to create a pool on his own, this driver doesn't
support pool creation currently.

A volume could be used with Qemu by adding an entry like this:

disk type='volume' device='disk'
  driver name='qemu' type='raw'/
  source pool='myzfspool' volume='vol5'/
  target dev='hdc' bus='ide'/
/disk
---
 configure.ac  |  43 +
 include/libvirt/libvirt.h.in  |   1 +
 po/POTFILES.in|   1 +
 src/Makefile.am   |   8 +
 src/conf/storage_conf.c   |  11 +-
 src/conf/storage_conf.h   |   4 +-
 src/qemu/qemu_conf.c  |   1 +
 src/storage/storage_backend.c |   6 +
 src/storage/storage_backend_zfs.c | 344 ++
 src/storage/storage_backend_zfs.h |  29 
 src/storage/storage_driver.c  |   1 +
 tools/virsh-pool.c|   3 +
 12 files changed, 449 insertions(+), 3 deletions(-)
 create mode 100644 src/storage/storage_backend_zfs.c
 create mode 100644 src/storage/storage_backend_zfs.h

diff --git a/configure.ac b/configure.ac
index f37c716..7e46460 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1734,6 +1734,10 @@ AC_ARG_WITH([storage-gluster],
   [AS_HELP_STRING([--with-storage-gluster],
 [with Gluster backend for the storage driver @:@default=check@:@])],
   [],[with_storage_gluster=check])
+AC_ARG_WITH([storage-zfs],
+  [AS_HELP_STRING([--with-storage-zfs],
+[with ZFS backend for the storage driver @:@default=check@:@])],
+  [],[with_storage_zfs=check])
 
 if test $with_libvirtd = no; then
   with_storage_dir=no
@@ -1746,6 +1750,7 @@ if test $with_libvirtd = no; then
   with_storage_rbd=no
   with_storage_sheepdog=no
   with_storage_gluster=no
+  with_storage_zfs=no
 fi
 if test $with_storage_dir = yes ; then
   AC_DEFINE_UNQUOTED([WITH_STORAGE_DIR], 1, [whether directory backend for 
storage driver is enabled])
@@ -1963,6 +1968,43 @@ if test $with_storage_gluster = yes; then
 fi
 AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test $with_storage_gluster = yes])
 
+if test $with_storage_zfs = check; then
+with_storage_zfs=$with_freebsd
+fi
+
+if test $with_storage_zfs = yes  test $with_freebsd = no; then
+AC_MSG_ERROR([The ZFS storage driver can be enabled on FreeBSD only.])
+fi
+
+if test $with_storage_zfs = yes ||
+   test $with_storage_zfs = check; then
+  AC_PATH_PROG([ZFS], [zfs], [], [$PATH:/sbin:/usr/sbin])
+  AC_PATH_PROG([ZPOOL], [zpool], [], [$PATH:/sbin:/usr/sbin])
+
+  if test $with_storage_zfs = yes; then
+if test -z $ZFS || test -z $ZPOOL; then
+  AC_MSG_ERROR([We need zfs and zpool for ZFS storage driver])
+fi
+  else
+if test -z $ZFS || test -z $ZPOOL; then
+  with_storage_zfs=no
+fi
+
+if test $with_storage_zfs = check; then
+  with_storage_zfs=yes
+fi
+  fi
+
+  if test $with_storage_zfs = yes; then
+AC_DEFINE_UNQUOTED([WITH_STORAGE_ZFS], 1,
+  [whether ZFS backend for storage driver is enabled])
+AC_DEFINE_UNQUOTED([ZFS], [$ZFS], [Location of zfs program])
+AC_DEFINE_UNQUOTED([ZPOOL], [$ZPOOL], [Location of zpool program])
+  fi
+fi
+AM_CONDITIONAL([WITH_STORAGE_ZFS],
+  [test $with_storage_zfs = yes])
+
 if test $with_storage_fs = yes ||
test $with_storage_gluster = yes; then
   AC_PATH_PROG([GLUSTER_CLI], [gluster], [], [$PATH:/sbin:/usr/sbin])
@@ -2806,6 +2848,7 @@ AC_MSG_NOTICE([Disk: $with_storage_disk])
 AC_MSG_NOTICE([ RBD: $with_storage_rbd])
 AC_MSG_NOTICE([Sheepdog: $with_storage_sheepdog])
 AC_MSG_NOTICE([ Gluster: $with_storage_gluster])
+AC_MSG_NOTICE([ ZFS: $with_storage_zfs])
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Security Drivers])
 AC_MSG_NOTICE([])
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index ad6785f..47ea695 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3170,6 +3170,7 @@ typedef enum {
 VIR_CONNECT_LIST_STORAGE_POOLS_RBD   = 1  14,
 VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG  = 1  15,
 VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER   = 1  16,
+VIR_CONNECT_LIST_STORAGE_POOLS_ZFS   = 1  17,
 } virConnectListAllStoragePoolsFlags;
 
 int virConnectListAllStoragePools(virConnectPtr conn,
diff --git a/po/POTFILES.in b/po/POTFILES.in
index d10e892..2646bcc 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -148,6 +148,7 @@ src/storage/storage_backend_mpath.c
 src/storage/storage_backend_rbd.c
 

[libvirt] [PATCH] metadata: track title edits across libvirtd restart

[Eric Blake]

https://bugzilla.redhat.com/show_bug.cgi?id=1122205

Although the edits were changing in-memory XML, it was not flushed
to disk; so unless some other action changes XML, a libvirtd restart
would lose the changed information.

* src/conf/domain_conf.c (virDomainObjSetMetadata): Add parameter,
to save live status across restarts.
(virDomainSaveXML): Allow for test driver.
* src/conf/domain_conf.h (virDomainObjSetMetadata): Adjust
signature.
* src/bhyve/bhyve_driver.c (bhyveDomainSetMetadata): Adjust caller.
* src/lxc/lxc_driver.c (lxcDomainSetMetadata): Likewise.
* src/qemu/qemu_driver.c (qemuDomainSetMetadata): Likewise.
* src/test/test_driver.c (testDomainSetMetadata): Likewise.

Signed-off-by: Eric Blake ebl...@redhat.com
---
 src/bhyve/bhyve_driver.c | 3 ++-
 src/conf/domain_conf.c   | 9 -
 src/conf/domain_conf.h   | 1 +
 src/lxc/lxc_driver.c | 3 ++-
 src/qemu/qemu_driver.c   | 3 ++-
 src/test/test_driver.c   | 2 +-
 6 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 9a13076..135cb24 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1046,7 +1046,8 @@ bhyveDomainSetMetadata(virDomainPtr dom,
 goto cleanup;

 ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps,
-  privconn-xmlopt, BHYVE_CONFIG_DIR, flags);
+  privconn-xmlopt, BHYVE_STATE_DIR,
+  BHYVE_CONFIG_DIR, flags);

  cleanup:
 virObjectUnref(caps);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4b00280..6ed6155 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18111,6 +18111,9 @@ virDomainSaveXML(const char *configDir,
 char *configFile = NULL;
 int ret = -1;

+if (!configDir)
+return 0;
+
 if ((configFile = virDomainConfigFile(configDir, def-name)) == NULL)
 goto cleanup;

@@ -19741,6 +19744,7 @@ virDomainObjSetMetadata(virDomainObjPtr vm,
 const char *uri,
 virCapsPtr caps,
 virDomainXMLOptionPtr xmlopt,
+const char *stateDir,
 const char *configDir,
 unsigned int flags)
 {
@@ -19753,9 +19757,12 @@ virDomainObjSetMetadata(virDomainObjPtr vm,
 persistentDef)  0)
 return -1;

-if (flags  VIR_DOMAIN_AFFECT_LIVE)
+if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 if (virDomainDefSetMetadata(vm-def, type, metadata, key, uri)  0)
 return -1;
+if (virDomainSaveStatus(xmlopt, stateDir, vm)  0)
+return -1;
+}

 if (flags  VIR_DOMAIN_AFFECT_CONFIG) {
 if (virDomainDefSetMetadata(persistentDef, type, metadata, key, uri)  
0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f5eb031..d89bbe7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2715,6 +2715,7 @@ int virDomainObjSetMetadata(virDomainObjPtr vm,
 const char *uri,
 virCapsPtr caps,
 virDomainXMLOptionPtr xmlopt,
+const char *stateDir,
 const char *configDir,
 unsigned int flags);

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b7b4b02..855222f 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -5600,7 +5600,8 @@ lxcDomainSetMetadata(virDomainPtr dom,
 goto cleanup;

 ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps,
-  driver-xmlopt, cfg-configDir, flags);
+  driver-xmlopt, cfg-stateDir,
+  cfg-configDir, flags);

  cleanup:
 virObjectUnlock(vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9d23f6d..72e4a91 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16128,7 +16128,8 @@ qemuDomainSetMetadata(virDomainPtr dom,
 goto cleanup;

 ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps,
-  driver-xmlopt, cfg-configDir, flags);
+  driver-xmlopt, cfg-stateDir,
+  cfg-configDir, flags);

  cleanup:
 virObjectUnlock(vm);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 67f637d..3b22cf6 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3042,7 +3042,7 @@ static int testDomainSetMetadata(virDomainPtr dom,

 ret = virDomainObjSetMetadata(privdom, type, metadata, key, uri,
   privconn-caps, privconn-xmlopt,
-  NULL, flags);
+  NULL, NULL, flags);

  cleanup:
 if (privdom)
-- 
1.9.3

--
libvir-list mailing list

[libvirt] [PATCH] conf: avoid memory leaks while parsing seclabel

[Eric Blake]

Our seclabel parsing was repeatedly assigning malloc'd data into a
temporary variable, without first freeing the previous use.  Among
other leaks flagged by valgrind:

==9312== 8 bytes in 1 blocks are definitely lost in loss record 88 of 821
==9312==at 0x4A0645D: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9312==by 0x8C40369: strdup (strdup.c:42)
==9312==by 0x50EA799: virStrdup (virstring.c:676)
==9312==by 0x50FAEB9: virXPathString (virxml.c:90)
==9312==by 0x50FAF1E: virXPathStringLimit (virxml.c:112)
==9312==by 0x510F516: virSecurityLabelDefParseXML (domain_conf.c:4571)
==9312==by 0x510FB20: virSecurityLabelDefsParseXML (domain_conf.c:4720)

While it was multiple problems, it looks like commit da78351 was
to blame for all of them.

* src/conf/domain_conf.c (virSecurityLabelDefParseXML): Plug leaks
detected by valgrind.

Signed-off-by: Eric Blake ebl...@redhat.com
---
 src/conf/domain_conf.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6ed6155..077e035 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4573,6 +4573,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,

 if (!(seclabel = virSecurityLabelDefNew(p)))
 goto error;
+VIR_FREE(p);

 /* set default value */
 seclabel-type = VIR_DOMAIN_SECLABEL_DYNAMIC;
@@ -4606,6 +4607,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 goto error;
 }
 }
+VIR_FREE(p);

 if (seclabel-type == VIR_DOMAIN_SECLABEL_DYNAMIC 
 !seclabel-relabel) {
@@ -4656,6 +4658,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 }

 seclabel-label = p;
+p = NULL;
 }

 /* Only parse imagelabel, if requested live XML with relabeling */
@@ -4670,6 +4673,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 goto error;
 }
 seclabel-imagelabel = p;
+p = NULL;
 }

 /* Only parse baselabel for dynamic label type */
@@ -4677,6 +4681,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 p = virXPathStringLimit(string(./baselabel[1]),
 VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
 seclabel-baselabel = p;
+p = NULL;
 }

 return seclabel;
-- 
1.9.3

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

[libvirt] [PATCH] nwfilter: plub memory leak with firewall

[Eric Blake]

Introduced in commit 70571ccc. Caught by valgrind:

==9816== 170 (32 direct, 138 indirect) bytes in 1 blocks are definitely lost in 
loss record 646 of 821
==9816==at 0x4A081D4: calloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9816==by 0x50836FB: virAlloc (viralloc.c:144)
==9816==by 0x50AEC2B: virFirewallNew (virfirewall.c:204)
==9816==by 0x1E2308ED: ebiptablesDriverProbeStateMatch 
(nwfilter_ebiptables_driver.c:3715)
==9816==by 0x1E2309AD: ebiptablesDriverInit 
(nwfilter_ebiptables_driver.c:3742)

* src/nwfilter/nwfilter_ebiptables_driver.c
(ebiptablesDriverProbeStateMatch): Properly clean up.

Signed-off-by: Eric Blake ebl...@redhat.com
---
 src/nwfilter/nwfilter_ebiptables_driver.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 1701d62..d41133c 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3713,6 +3713,7 @@ ebiptablesDriverProbeStateMatch(void)
 {
 unsigned long version;
 virFirewallPtr fw = virFirewallNew();
+int ret = -1;

 virFirewallStartTransaction(fw, 0);
 virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_IPV4,
@@ -3720,7 +3721,7 @@ ebiptablesDriverProbeStateMatch(void)
--version, NULL);

 if (virFirewallApply(fw)  0)
-return -1;
+goto cleanup;

 /*
  * since version 1.4.16 '-m state --state ...' will be converted to
@@ -3729,7 +3730,10 @@ ebiptablesDriverProbeStateMatch(void)
 if (version = 1 * 100 + 4 * 1000 + 16)
 newMatchState = true;

-return 0;
+ret = 0;
+ cleanup:
+virFirewallFree(fw);
+return ret;
 }

 static int
-- 
1.9.3

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

[libvirt] [PATCH] nodedev: fix pci express memory leak

[Eric Blake]

Leak introduced in commit 16ebf10f (v1.2.6), detected by valgrind:

==9816== 216 (96 direct, 120 indirect) bytes in 6 blocks are definitely lost in 
loss record 665 of 821
==9816==at 0x4A081D4: calloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9816==by 0x50836FB: virAlloc (viralloc.c:144)
==9816==by 0x1DBDBE27: udevProcessPCI (node_device_udev.c:546)
==9816==by 0x1DBDD79D: udevGetDeviceDetails (node_device_udev.c:1293)

While at it, fix switch statements to be a bit more vocal if we forget
some cases when adding new devices.

* src/conf/node_device_conf.c (virNodeDeviceDefFormat)
(virNodeDevCapsDefParseXML): Drop default case.
(virNodeDevCapsDefFree): Likewise, and clear pci_express under pci
case.

Signed-off-by: Eric Blake ebl...@redhat.com
---

I could be persuaded to split this into two patches.

 src/conf/node_device_conf.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 910a371..4b34fec 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -547,7 +547,6 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
 case VIR_NODE_DEV_CAP_FC_HOST:
 case VIR_NODE_DEV_CAP_VPORTS:
 case VIR_NODE_DEV_CAP_LAST:
-default:
 break;
 }

@@ -1405,7 +1404,10 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt,
 case VIR_NODE_DEV_CAP_STORAGE:
 ret = virNodeDevCapStorageParseXML(ctxt, def, node, caps-data);
 break;
-default:
+case VIR_NODE_DEV_CAP_FC_HOST:
+case VIR_NODE_DEV_CAP_VPORTS:
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+case VIR_NODE_DEV_CAP_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(unknown capability type '%d' for '%s'),
caps-type, def-name);
@@ -1665,6 +1667,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
 VIR_FREE(data-pci_dev.iommuGroupDevices[i]);
 }
 VIR_FREE(data-pci_dev.iommuGroupDevices);
+VIR_FREE(data-pci_dev.pci_express);
 break;
 case VIR_NODE_DEV_CAP_USB_DEV:
 VIR_FREE(data-usb_dev.product_name);
@@ -1703,7 +1706,6 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
 case VIR_NODE_DEV_CAP_FC_HOST:
 case VIR_NODE_DEV_CAP_VPORTS:
 case VIR_NODE_DEV_CAP_LAST:
-default:
 /* This case is here to shutup the compiler */
 break;
 }
-- 
1.9.3

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

Re: [libvirt] [PATCH] nwfilter: plub memory leak with firewall

[Martin Kletzander]

On Tue, Jul 22, 2014 at 10:19:54PM -0600, Eric Blake wrote:

Introduced in commit 70571ccc. Caught by valgrind:

==9816== 170 (32 direct, 138 indirect) bytes in 1 blocks are definitely lost in 
loss record 646 of 821
==9816==at 0x4A081D4: calloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9816==by 0x50836FB: virAlloc (viralloc.c:144)
==9816==by 0x50AEC2B: virFirewallNew (virfirewall.c:204)
==9816==by 0x1E2308ED: ebiptablesDriverProbeStateMatch 
(nwfilter_ebiptables_driver.c:3715)
==9816==by 0x1E2309AD: ebiptablesDriverInit 
(nwfilter_ebiptables_driver.c:3742)

* src/nwfilter/nwfilter_ebiptables_driver.c
(ebiptablesDriverProbeStateMatch): Properly clean up.

Signed-off-by: Eric Blake ebl...@redhat.com
---
src/nwfilter/nwfilter_ebiptables_driver.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)



ACK,

Martin


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

Re: [libvirt] [PATCH] conf: avoid memory leaks while parsing seclabel

[Martin Kletzander]

On Tue, Jul 22, 2014 at 10:08:21PM -0600, Eric Blake wrote:

Our seclabel parsing was repeatedly assigning malloc'd data into a
temporary variable, without first freeing the previous use.  Among
other leaks flagged by valgrind:

==9312== 8 bytes in 1 blocks are definitely lost in loss record 88 of 821
==9312==at 0x4A0645D: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9312==by 0x8C40369: strdup (strdup.c:42)
==9312==by 0x50EA799: virStrdup (virstring.c:676)
==9312==by 0x50FAEB9: virXPathString (virxml.c:90)
==9312==by 0x50FAF1E: virXPathStringLimit (virxml.c:112)
==9312==by 0x510F516: virSecurityLabelDefParseXML (domain_conf.c:4571)
==9312==by 0x510FB20: virSecurityLabelDefsParseXML (domain_conf.c:4720)

While it was multiple problems, it looks like commit da78351 was
to blame for all of them.

* src/conf/domain_conf.c (virSecurityLabelDefParseXML): Plug leaks
detected by valgrind.

Signed-off-by: Eric Blake ebl...@redhat.com
---
src/conf/domain_conf.c | 5 +
1 file changed, 5 insertions(+)



ACK,

Martin


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

Re: [libvirt] [PATCH] metadata: track title edits across libvirtd restart

[Martin Kletzander]

On Tue, Jul 22, 2014 at 01:11:03PM -0600, Eric Blake wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1122205

Although the edits were changing in-memory XML, it was not flushed
to disk; so unless some other action changes XML, a libvirtd restart
would lose the changed information.



And there are more places like that.  Take a
qemuDomainSetNumaParameters() for example.  Would it be possible (as
in not devastatingly hard to create a syntax-check rule for
*_driver.c files (only those that are applicable, mostly stateful)
that would check if virCheckFlags is called with
VIR_DOMAIN_AFFECT_LIVE and if yes, then that function would have to
call virDomainSaveStatus() as well?

[...]

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4b00280..6ed6155 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c

[...]

@@ -19753,9 +19757,12 @@ virDomainObjSetMetadata(virDomainObjPtr vm,
persistentDef)  0)
return -1;

-if (flags  VIR_DOMAIN_AFFECT_LIVE)
+if (flags  VIR_DOMAIN_AFFECT_LIVE) {
if (virDomainDefSetMetadata(vm-def, type, metadata, key, uri)  0)
return -1;


One empty line right here would perfectly go with the rest of the
function ;)

ACK with or without this change,

Martin


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

Re: [libvirt] [PATCH] nodedev: fix pci express memory leak

[Martin Kletzander]

On Tue, Jul 22, 2014 at 10:42:29PM -0600, Eric Blake wrote:

Leak introduced in commit 16ebf10f (v1.2.6), detected by valgrind:

==9816== 216 (96 direct, 120 indirect) bytes in 6 blocks are definitely lost in 
loss record 665 of 821
==9816==at 0x4A081D4: calloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9816==by 0x50836FB: virAlloc (viralloc.c:144)
==9816==by 0x1DBDBE27: udevProcessPCI (node_device_udev.c:546)
==9816==by 0x1DBDD79D: udevGetDeviceDetails (node_device_udev.c:1293)

While at it, fix switch statements to be a bit more vocal if we forget
some cases when adding new devices.

* src/conf/node_device_conf.c (virNodeDeviceDefFormat)
(virNodeDevCapsDefParseXML): Drop default case.
(virNodeDevCapsDefFree): Likewise, and clear pci_express under pci
case.

Signed-off-by: Eric Blake ebl...@redhat.com
---

I could be persuaded to split this into two patches.



That would be nice, now that you've mentioned it :)


src/conf/node_device_conf.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 910a371..4b34fec 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c

[...]

@@ -1665,6 +1667,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
VIR_FREE(data-pci_dev.iommuGroupDevices[i]);
}
VIR_FREE(data-pci_dev.iommuGroupDevices);
+VIR_FREE(data-pci_dev.pci_express);


There should be data-pci_dev.pci_express-link_{sta,cap} free()'d too.

ACK with that changed.

Martin


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

Re: [libvirt] [PATCH 1/2] Refactor virMutexInit virRWLockInit and virCondInit

[Martin Kletzander]

On Fri, Jul 18, 2014 at 07:30:38AM -0600, Eric Blake wrote:

On 07/17/2014 10:49 PM, Jincheng Miao wrote:

Implement InitInternal functions for Mutex, RWLock and Cond, these functions
contain error report using virReportSystemErrorFull, it is controlled by
an argument 'quite'.
The related macros are Init and InitQuite, they call InitInternal function
by passing 'false' or 'true' to quite argument.


After your patch 2, do we really have any callers that use the quiet
version? This seems like the sort of patch where ALL callers should be
noisy (especially since the failure is so rare, but also means we are
quite hosed if it happens).  What are the callers that you intend to be
quiet, and what is the justification for them being quiet?



I think there are few callers that error out with VIR_ERROR(), but
most of them just return -1 with no apparent reason.  I'm not sure
about few class initializations (virOnceInit) and driver
initializations.  Do we want to be loud on that front as well?

Martin


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