date:20160211

Peter Krempa (4):
  conf: snapshot: Rename disksorter to virDomainSnapshotCompareDiskIndex
  snapshot: conf: Extract code to generate default external file names
  conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames
  conf: snapshot: Avoid autogenerating duplicate snapshot names

 src/conf/snapshot_conf.c | 142 ---
 1 file changed, 86 insertions(+), 56 deletions(-)

-- 
2.6.2

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

[libvirt] [PATCH 2/4] snapshot: conf: Extract code to generate default external file names

---
 src/conf/snapshot_conf.c | 131 ---
 1 file changed, 77 insertions(+), 54 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 65a78d6..da6fec5 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -429,6 +429,80 @@ virDomainSnapshotDefParseString(const char *xmlStr,
 return ret;
 }

+
+/**
+ * virDomainSnapshotDefAssignExternalNames:
+ * @def: snapshot def object
+ *
+ * Generate default external file names for snapshot targets. Returns 0 on
+ * success, -1 on error.
+ */
+static int
+virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
+{
+size_t i;
+int ret = -1;
+
+for (i = 0; i < def->ndisks; i++) {
+virDomainSnapshotDiskDefPtr disk = >disks[i];
+
+if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL &&
+!disk->src->path) {
+const char *original = virDomainDiskGetSource(def->dom->disks[i]);
+const char *tmp;
+struct stat sb;
+
+if (disk->src->type != VIR_STORAGE_TYPE_FILE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot generate external snapshot name "
+ "for disk '%s' on a '%s' device"),
+   disk->name,
+   virStorageTypeToString(disk->src->type));
+goto cleanup;
+}
+
+if (!original) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot generate external snapshot name "
+ "for disk '%s' without source"),
+   disk->name);
+goto cleanup;
+}
+if (stat(original, ) < 0 || !S_ISREG(sb.st_mode)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("source for disk '%s' is not a regular "
+ "file; refusing to generate external "
+ "snapshot name"),
+   disk->name);
+goto cleanup;
+}
+
+tmp = strrchr(original, '.');
+if (!tmp || strchr(tmp, '/')) {
+if (virAsprintf(>src->path, "%s.%s", original,
+def->name) < 0)
+goto cleanup;
+} else {
+if ((tmp - original) > INT_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("integer overflow"));
+goto cleanup;
+}
+if (virAsprintf(>src->path, "%.*s.%s",
+(int) (tmp - original), original,
+def->name) < 0)
+goto cleanup;
+}
+}
+}
+
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
 static int
 virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
 {
@@ -565,60 +639,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
 qsort(>disks[0], def->ndisks, sizeof(def->disks[0]),
   virDomainSnapshotCompareDiskIndex);

-/* Generate any default external file names, but only if the
- * backing file is a regular file.  */
-for (i = 0; i < def->ndisks; i++) {
-virDomainSnapshotDiskDefPtr disk = >disks[i];
-
-if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL &&
-!disk->src->path) {
-const char *original = virDomainDiskGetSource(def->dom->disks[i]);
-const char *tmp;
-struct stat sb;
-
-if (disk->src->type != VIR_STORAGE_TYPE_FILE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("cannot generate external snapshot name "
- "for disk '%s' on a '%s' device"),
-   disk->name,
-   virStorageTypeToString(disk->src->type));
-goto cleanup;
-}
-
-if (!original) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("cannot generate external snapshot name "
- "for disk '%s' without source"),
-   disk->name);
-goto cleanup;
-}
-if (stat(original, ) < 0 || !S_ISREG(sb.st_mode)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("source for disk '%s' is not a regular "
- "file; refusing to generate external "
- "snapshot name"),
-   disk->name);
-goto cleanup;
-}
-
-tmp = strrchr(original, '.');
-if (!tmp || strchr(tmp, '/')) {
-

[libvirt] [PATCH 3/4] conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames

Get rid of one indentation level by negating condition and remove ugly
pointer arithmetic at the cost of one extra allocation.
---
 src/conf/snapshot_conf.c | 94 +++-
 1 file changed, 44 insertions(+), 50 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index da6fec5..f8a1aed 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -440,66 +440,60 @@ virDomainSnapshotDefParseString(const char *xmlStr,
 static int
 virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
 {
+const char *origpath;
+char *tmppath;
+char *tmp;
+struct stat sb;
 size_t i;
-int ret = -1;

 for (i = 0; i < def->ndisks; i++) {
 virDomainSnapshotDiskDefPtr disk = >disks[i];

-if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL &&
-!disk->src->path) {
-const char *original = virDomainDiskGetSource(def->dom->disks[i]);
-const char *tmp;
-struct stat sb;
-
-if (disk->src->type != VIR_STORAGE_TYPE_FILE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("cannot generate external snapshot name "
- "for disk '%s' on a '%s' device"),
-   disk->name,
-   virStorageTypeToString(disk->src->type));
-goto cleanup;
-}
+if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL ||
+disk->src->path)
+continue;

-if (!original) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("cannot generate external snapshot name "
- "for disk '%s' without source"),
-   disk->name);
-goto cleanup;
-}
-if (stat(original, ) < 0 || !S_ISREG(sb.st_mode)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("source for disk '%s' is not a regular "
- "file; refusing to generate external "
- "snapshot name"),
-   disk->name);
-goto cleanup;
-}
+if (disk->src->type != VIR_STORAGE_TYPE_FILE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot generate external snapshot name "
+ "for disk '%s' on a '%s' device"),
+   disk->name, 
virStorageTypeToString(disk->src->type));
+return -1;
+}

-tmp = strrchr(original, '.');
-if (!tmp || strchr(tmp, '/')) {
-if (virAsprintf(>src->path, "%s.%s", original,
-def->name) < 0)
-goto cleanup;
-} else {
-if ((tmp - original) > INT_MAX) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("integer overflow"));
-goto cleanup;
-}
-if (virAsprintf(>src->path, "%.*s.%s",
-(int) (tmp - original), original,
-def->name) < 0)
-goto cleanup;
-}
+if (!(origpath = virDomainDiskGetSource(def->dom->disks[i]))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot generate external snapshot name "
+ "for disk '%s' without source"),
+   disk->name);
+return -1;
 }
-}

-ret = 0;
+if (stat(origpath, ) < 0 || !S_ISREG(sb.st_mode)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("source for disk '%s' is not a regular "
+ "file; refusing to generate external "
+ "snapshot name"),
+   disk->name);
+return -1;
+}

- cleanup:
-return ret;
+if (VIR_STRDUP(tmppath, origpath) < 0)
+return -1;
+
+/* drop suffix of the file name */
+if ((tmp = strrchr(tmppath, '.')) && !strchr(tmp, '/'))
+*tmp = '\0';
+
+if (virAsprintf(>src->path, "%s.%s", tmppath, def->name) < 0) {
+VIR_FREE(tmppath);
+return -1;
+}
+
+VIR_FREE(tmppath);
+}
+
+return 0;
 }


-- 
2.6.2

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

[libvirt] [PATCH 1/4] conf: snapshot: Rename disksorter to virDomainSnapshotCompareDiskIndex

Stick to the naming pattern.
---
 src/conf/snapshot_conf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 1eda7c2..65a78d6 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -430,7 +430,7 @@ virDomainSnapshotDefParseString(const char *xmlStr,
 }

 static int
-disksorter(const void *a, const void *b)
+virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
 {
 const virDomainSnapshotDiskDef *diska = a;
 const virDomainSnapshotDiskDef *diskb = b;
@@ -562,7 +562,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
 disk->snapshot = default_snapshot;
 }

-qsort(>disks[0], def->ndisks, sizeof(def->disks[0]), disksorter);
+qsort(>disks[0], def->ndisks, sizeof(def->disks[0]),
+  virDomainSnapshotCompareDiskIndex);

 /* Generate any default external file names, but only if the
  * backing file is a regular file.  */
-- 
2.6.2

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

[libvirt] [PATCH 4/4] conf: snapshot: Avoid autogenerating duplicate snapshot names

The snapshot name generator truncates the original file name after a '.'
and replaces the suffix with the snapshot name. If two disks source
images differ only in the suffix portion, the generated name will be
duplicate.

Since this is a corner case just error out stating that a duplicate name
was generated. The user can work around this situation by providing
the file names explicitly.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085
---
 src/conf/snapshot_conf.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f8a1aed..58646bd 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -445,6 +445,7 @@ 
virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
 char *tmp;
 struct stat sb;
 size_t i;
+size_t j;

 for (i = 0; i < def->ndisks; i++) {
 virDomainSnapshotDiskDefPtr disk = >disks[i];
@@ -491,6 +492,17 @@ 
virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
 }

 VIR_FREE(tmppath);
+
+/* verify that we didn't generate a duplicate name */
+for (j = 0; j < i; j++) {
+if (STREQ_NULLABLE(disk->src->path, def->disks[j].src->path)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot generate external snapshot name for "
+ "disk '%s': collision with disk '%s'"),
+   disk->name, def->disks[j].name);
+return -1;
+}
+}
 }

 return 0;
-- 
2.6.2

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

Re: [libvirt] [PATCH] Revert "tests: Don't link mock libraries against libvirt and gnulib"

On Thu, Feb 11, 2016 at 09:54:45AM +0100, Andrea Bolognani wrote:
> On Wed, 2016-02-10 at 18:09 +, Daniel P. Berrange wrote:
> > On Wed, Feb 10, 2016 at 06:33:21PM +0100, Andrea Bolognani wrote:
> > > This reverts commit 6aa90452aa63cb1e1ffa84ff5f93f5873bf810a0.
> > > 
> > > Turns out that not linking against libvirt and gnulib is okay for
> > > regular Linux (and FreeBSD) builds, but makes mingw very unhappy.
> > > 
> > >   .../virnetserverclientmock_la-virnetserverclientmock.o:
> > > In function `virNetSocketGetSELinuxContext':
> > > .../virnetserverclientmock.c:61: undefined reference to `rpl_strdup'
> > >   .../libvirportallocatormock_la-virportallocatortest.o:
> > > In function `init_syms':
> > > .../virportallocatortest.c:61: undefined reference to `virFileClose'
> > > ---
> > > Pushed as build breaker.
> > > 
> > > Bunny ears of shame will be deployed in the morning.
> > 
> > You should still be able to drop linkage of libvirt.la, just keep
> > the gnulib bits.  The gnulib addition is harmless, because it is
> > a static library the linker will drop all functions which are not
> > actually used by the mock.o, so it will end up being a no-op on
> > Linux, and will only pull in rpl_strup on mingw32. We should never
> > link libvirt.la to mock libs though.
> 
> Looks like it is complaining about virFileClose(), plus
> virFilePrint() in the full output, as well. So maybe linking against
> libvirt.la is needed after all?

That's a bug in virportallocatortest.c that was introduced in
0ee90812. The init_syms() fnuction should *not* be calling
VIR_FORCE_CLOSE - it must use close(), because this is the
mock-override and so should not be using libvirt.so symbols.

> If there's no harm in that, how about defining
> 
>   MOCKLIBS_LDADD = \
> $(GNULIB_LIBS) \
> ../src/libvirt.la
> 
> and using it for every mock library, like the existing
> $(MOCKLIB_LDFLAGS)?

Nope as above, we should not be linking libvirt.la to the mock lib,
because the mock lib is supposed to be replacing libvirt.la symbols

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] Revert "tests: Don't link mock libraries against libvirt and gnulib"

On Wed, 2016-02-10 at 18:09 +, Daniel P. Berrange wrote:
> On Wed, Feb 10, 2016 at 06:33:21PM +0100, Andrea Bolognani wrote:
> > This reverts commit 6aa90452aa63cb1e1ffa84ff5f93f5873bf810a0.
> > 
> > Turns out that not linking against libvirt and gnulib is okay for
> > regular Linux (and FreeBSD) builds, but makes mingw very unhappy.
> > 
> >   .../virnetserverclientmock_la-virnetserverclientmock.o:
> > In function `virNetSocketGetSELinuxContext':
> > .../virnetserverclientmock.c:61: undefined reference to `rpl_strdup'
> >   .../libvirportallocatormock_la-virportallocatortest.o:
> > In function `init_syms':
> > .../virportallocatortest.c:61: undefined reference to `virFileClose'
> > ---
> > Pushed as build breaker.
> > 
> > Bunny ears of shame will be deployed in the morning.
> 
> You should still be able to drop linkage of libvirt.la, just keep
> the gnulib bits.  The gnulib addition is harmless, because it is
> a static library the linker will drop all functions which are not
> actually used by the mock.o, so it will end up being a no-op on
> Linux, and will only pull in rpl_strup on mingw32. We should never
> link libvirt.la to mock libs though.

Looks like it is complaining about virFileClose(), plus
virFilePrint() in the full output, as well. So maybe linking against
libvirt.la is needed after all?

If there's no harm in that, how about defining

  MOCKLIBS_LDADD = \
$(GNULIB_LIBS) \
../src/libvirt.la

and using it for every mock library, like the existing
$(MOCKLIB_LDFLAGS)?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH] Prohibit verbose strcat

On Thu, 2016-02-11 at 07:43 +0100, Ján Tomko wrote:
> > > + halt='Use strcat(a, b) instead of strncat(a, b, strlen())' \
> > 
> > s/strlen()/strlen(b)/
> 
> That would make it match itself, so I limited the check to C files
> in_vc_files='\.[ch]$$'

Which is a good idea anyway, I guess :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 3/3] virsh: Add support for text based polkit authentication

On Wed, Feb 10, 2016 at 02:46:36PM -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=872166
> 
> When the login session doesn't have an ssh -X type display agent in
> order for libvirtd to run the polkit session authentication, attempts
> to run 'virsh -c qemu:///system list' from an unauthorized user (or one
> that isn't part of the libvirt /etc/group) will fail with the following
> error from libvirtd:
> 
> error: authentication failed: no agent is available to authenticate -
>polkit agent 'org.libvirt.unix.manage' is not available
> 
> In order to handle the local authentication, add a new switch "--pkauth"
> to virsh which will make use of the virPolkitAgent* API's in order to
> handle the text authentication when not a readonly request.
> 
> With this patch in place, the following occurs:
> 
> $ virsh --pkauth -c qemu:///system list
>  AUTHENTICATING FOR org.libvirt.unix.manage ===
> System policy prevents management of local virtualized systems
> Authenticating as: Some User (SUser)
> Password:
>  AUTHENTICATION COMPLETE ===
>  IdName   State
>  
>   1 somedomain running
> 
> $
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/virsh.c   | 22 +++---
>  tools/virsh.h   |  2 ++
>  tools/virsh.pod | 10 ++
>  tools/vsh.h |  1 +
>  4 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index b96dbda..83cebe0 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -143,6 +143,7 @@ virshConnect(vshControl *ctl, const char *uri, bool 
> readonly)
>  int interval = 5; /* Default */
>  int count = 6;/* Default */
>  bool keepalive_forced = false;
> +virCommandPtr pkagent = NULL;
>  
>  if (ctl->keepalive_interval >= 0) {
>  interval = ctl->keepalive_interval;
> @@ -153,10 +154,17 @@ virshConnect(vshControl *ctl, const char *uri, bool 
> readonly)
>  keepalive_forced = true;
>  }
>  
> +if (!readonly && ctl->pkauth) {
> +if (!(pkagent = virPolkitAgentCreate()))
> +return NULL;
> +if (virPolkitAgentCheck() < 0)
> +goto cleanup;

I'm not sure why you need to run pkcheck here - the auth check is
something the server side does - it doesn't make sense for the
client to be doing that.


> @@ -701,13 +713,14 @@ virshParseArgv(vshControl *ctl, int argc, char **argv)
>  {"readonly", no_argument, NULL, 'r'},
>  {"timing", no_argument, NULL, 't'},
>  {"version", optional_argument, NULL, 'v'},
> +{"pkauth", no_argument, NULL, 'p'},

I think it would nice if we did without this extra argument and made things
"just work". The way polkit agents work is that they have to claim a well
known path on the system dbus instance. If we connect to DBus and check for
existance of the "/org/freedesktop/PolicyKit1/AuthenticationAgent" we will
know whether we should try to provide an auth agent or not.

That said I wonder if there's cause to support a scenario where polkit
agent is not running and people want to run virsh without the agent
still.


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] dbus: Don't unref NULL messages

On Thu, Feb 11, 2016 at 11:25:05AM +0100, Michal Privoznik wrote:
> Apparently we are not the only ones with dumb free functions
> because dbus_message_unref() does not accept NULL either. But if
> I were to vote, this one is even more evil. Instead of returning
> an error just like we do it immediately dereference any pointer
> passed and thus crash you app. Well done DBus!
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   [Switching to Thread 0x7f878ebda700 (LWP 31264)]
>   0x7f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
>   (gdb) bt
>   #0  0x7f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
>   #1  0x7f87be3f004e in dbus_message_unref () from 
> /usr/lib64/libdbus-1.so.3
>   #2  0x7f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at 
> util/virsystemd.c:228
>   #3  0x7f879761bd4d in qemuConnectCgroup (driver=0x7f87600a32a0, 
> vm=0x7f87600c7550) at qemu/qemu_cgroup.c:909
>   #4  0x7f87976386b7 in qemuProcessReconnect (opaque=0x7f87600db840) at 
> qemu/qemu_process.c:3386
>   #5  0x7f87bf6edfff in virThreadHelper (data=0x7f87600d5580) at 
> util/virthread.c:206
>   #6  0x7f87bb602334 in start_thread (arg=0x7f878ebda700) at 
> pthread_create.c:333
>   #7  0x7f87bb3481bd in clone () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>   (gdb) frame 2
>   #2  0x7f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at 
> util/virsystemd.c:228
>   228 dbus_message_unref(reply);
>   (gdb) p reply
>   $1 = (DBusMessage *) 0x0
> 
> Signed-off-by: Michal Privoznik 

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 1/2] tests: Use plain close() in mock code

The virportallocatortest.c file is compiled both as a test case
and as a mock library; in the latter case, it can't use
VIR_FORCE_CLOSE() because mock libraries are not linked against
libvirt.

Replace VIR_FORCE_CLOSE() with plain close() to solve the issue.
---
 tests/virportallocatortest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c
index 58b2155..077aad8 100644
--- a/tests/virportallocatortest.c
+++ b/tests/virportallocatortest.c
@@ -58,7 +58,7 @@ static void init_syms(void)
 return;
 
 host_has_ipv6 = true;
-VIR_FORCE_CLOSE(fd);
+close(fd);
 }
 
 int socket(int domain,
-- 
2.5.0

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

Re: [libvirt] [PATCH 0/2] tests: Fix mock libraries linking

On Thu, 2016-02-11 at 11:17 +, Daniel P. Berrange wrote:
> On Thu, Feb 11, 2016 at 12:15:39PM +0100, Andrea Bolognani wrote:
> > This patches make sure mock libraries do not link against either
> > too many or too few libraries.
> > 
> > Cheers.
> > 
> > Andrea Bolognani (2):
> >   tests: Use plain close() in mock code
> >   tests: Link mock libraries against gnulib and gnulib only
> > 
> >  tests/Makefile.am| 21 +++--
> >  tests/virportallocatortest.c |  2 +-
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> ACK

Pushed, thanks.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 4/4] conf: snapshot: Avoid autogenerating duplicate snapshot names

On Thu, Feb 11, 2016 at 15:24:42 +0100, Erik Skultety wrote:
> On 11/02/16 10:20, Peter Krempa wrote:
> > The snapshot name generator truncates the original file name after a '.'
> > and replaces the suffix with the snapshot name. If two disks source
> > images differ only in the suffix portion, the generated name will be
> > duplicate.
> > 
> > Since this is a corner case just error out stating that a duplicate name
> > was generated. The user can work around this situation by providing
> > the file names explicitly.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085
> > ---
> >  src/conf/snapshot_conf.c | 12 
> >  1 file changed, 12 insertions(+)
> > 
> ...
> 
> ACK

Thanks, I've pushed the series.

Peter


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

Re: [libvirt] [PATCH 0/4] tests: Fix syntax-check failure, plus extras

2016-02-11 Thread Ján Tomko

On Thu, Feb 11, 2016 at 02:36:27PM +0100, Andrea Bolognani wrote:
> Commit a03cbfe0fb introduced a syntax-check failure: patch
> 1/4 prepares for 2/4, where the failure is actually fixed.
> 
> Patches 3/4 and 4/4 improve the naming consistency for mock
> libraries by shuffling files around a bit.
> 
> Cheers.
> 
> 
> Andrea Bolognani (4):
>   tests: Split off the mock part of the port allocator test
>   tests: Allow use of close() in mock libraries
>   tests: Don't use "lib" prefix for mock libraries
>   tests: Rename virmockdbus -> virdbusmock for consistency
> 
>  cfg.mk |   2 +-
>  tests/Makefile.am  |  26 
>  tests/{virmockdbus.c => virdbusmock.c} |   2 +-
>  tests/virfirewalltest.c|   2 +-
>  tests/virpolkittest.c  |   2 +-
>  tests/virportallocatormock.c   | 108 +++
>  tests/virportallocatortest.c   | 112 
> +++--
>  tests/virsystemdtest.c |   2 +-
>  8 files changed, 134 insertions(+), 122 deletions(-)
>  rename tests/{virmockdbus.c => virdbusmock.c} (97%)
>  create mode 100644 tests/virportallocatormock.c

ACK to patches 2-4.

Jan


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/4] tests: Split off the mock part of the port allocator test

2016-02-11 Thread Ján Tomko

On Thu, Feb 11, 2016 at 02:36:28PM +0100, Andrea Bolognani wrote:
> Instead of compiling either the mock or the non-mock part of the
> file based on a compiler flag, split the mock part off to its
> own file.
> ---
>  tests/Makefile.am|   4 +-
>  tests/virportallocatormock.c | 108 ++
>  tests/virportallocatortest.c | 110 
> +++
>  3 files changed, 117 insertions(+), 105 deletions(-)
>  create mode 100644 tests/virportallocatormock.c

> @@ -255,12 +168,3 @@ mymain(void)
>  }
>  
>  VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir 
> "/.libs/libvirportallocatormock.so")
> -# endif
> -
> -#else /* ! defined(RTLD_NEXT) */
> -int
> -main(void)
> -{
> -return EXIT_AM_SKIP;
> -}
> -#endif

I'm afraid removing this will break the test on non-Linux.

ACK if you leave the EXIT_AM_SKIP in.

Jan


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

Re: [libvirt] Found mem leak in libvirtd, need help to debug

2016-02-11 Thread Piotr Rybicki





I still think these are libgfapi leaks; All the definitely lost bytes
come from the library.

==6532== 3,064 (96 direct, 2,968 indirect) bytes in 1 blocks are
definitely lost in loss record 1,106 of 1,142
==6532==at 0x4C2C0D0: calloc (vg_replace_malloc.c:711)
==6532==by 0x10701279: __gf_calloc (mem-pool.c:117)
==6532==by 0x106CC541: xlator_dynload (xlator.c:259)
==6532==by 0xFC4E947: create_master (glfs.c:202)
==6532==by 0xFC4E947: glfs_init_common (glfs.c:863)
==6532==by 0xFC4EB50: glfs_init@@GFAPI_3.4.0 (glfs.c:916)
==6532==by 0xF7E4A33: virStorageFileBackendGlusterInit
(storage_backend_gluster.c:625)
==6532==by 0xF7D56DE: virStorageFileInitAs (storage_driver.c:2788)
==6532==by 0xF7D5E39: virStorageFileGetMetadataRecurse
(storage_driver.c:3048)
==6532==by 0xF7D6295: virStorageFileGetMetadata
(storage_driver.c:3171)
==6532==by 0x1126A2B0: qemuDomainDetermineDiskChain
(qemu_domain.c:3179)
==6532==by 0x11269AE6: qemuDomainCheckDiskPresence
(qemu_domain.c:2998)
==6532==by 0x11292055: qemuProcessLaunch (qemu_process.c:4708)

Care to reporting it to them?


Of course - i will.

But, are You sure there is no need to call glfs_fini() after qemu
process is launched? Are all of those resources still needed in libvirt?

I understand, that libvirt needs to check presence / other-things of
storage, but after qemu is launched?


We call glfs_fini(). And that's the problem. It does not free everything
that glfs_init() allocated. Hence the leaks. Actually every time we call
glfs_init() we print a debug message from
virStorageFileBackendGlusterInit() which wraps it. And then another
debug message from virStorageFileBackendGlusterDeinit() when we call
glfs_fini(). So if you set up debug logs, you can check whether our init
and finish calls match.


Thanks Michal, You are right.

Leak still exists in newest gluster 3.7.8

There is even simpler case to see this memleak. valgrind on:

qemu-img info gluster://SERVER_IP:0/pool/FILE.img

==6100== LEAK SUMMARY:
==6100==definitely lost: 19,846 bytes in 98 blocks
==6100==indirectly lost: 2,479,205 bytes in 182 blocks
==6100==  possibly lost: 240,600 bytes in 7 blocks
==6100==still reachable: 3,271,130 bytes in 2,931 blocks
==6100== suppressed: 0 bytes in 0 blocks

So it's definitely gluster fault.

I've just reported it on gluster-devel@

Best regards
Piotr Rybicki

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

Re: [libvirt] [RFC] vhost-user + shared memory + NUMA

2016-02-11 Thread Pavel Fedin

 Hello!

> >  Ok, then would it be a good compromise if we require , and 
> > only implicitly
> add "shared" if we have vhost-user
> > devices? This way we would not change the way the guest memory is allocated.
> 
> Adding shared implicitly *will* change the way guest memory is allocated,
> as it will have to use tmpfs to make it shared.

 You perhaps didn't get my idea. I meant - we will still need to specify 
 with huge pages, just no . Therefore, the memory will be 
allocated via file backend from hugetlbfs. Only mode will be changed implicitly 
(private -> shared).

> >  IMHO being able to manually specify "shared" both in  and
> > in  would be ambiguous.
> 
> That's not really any different to what we have already with NUMA.
> The top level setting would apply as the default, and the NUMA level
> settings override it if needed.

 Well, the only little drawback would be necessity to add "shared" by itself. 
This would require additional patching to clients (e. g. openstack).

Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia


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

[libvirt] [PATCH] Error out on missing machine type in machine configs

2016-02-11 Thread Ján Tomko

Commit f1a89a8 allowed parsing configs from /etc/libvirt
without validating the emulator capabilities.

Check for the presence of os->type.machine even if the
VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS flag is set,
otherwise the daemon can crash on carelessly crafted input
in the config directory.

https://bugzilla.redhat.com/show_bug.cgi?id=1267256
---
 src/conf/domain_conf.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 67415fa..5d3fed0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14841,6 +14841,12 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;
 }
 VIR_FREE(capsdata);
+} else {
+if (!def->os.machine) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Missing machine type"));
+goto error;
+}
 }
 
 /* Extract domain name */
-- 
2.4.10

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

Re: [libvirt] [PATCH 0/3] Add capability for text based polkit authentication for virsh

On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=872166
> 
> As an alternative to commit id 'e94979e90' which allows polkit
> authentication by adding users to the 'libvirt' group, add the
> ability to start and utilize a text based authentication agent
> for virsh.
> 
> At the very least patch 1 will suffice part of the issue listed
> in the bz - the opaque error message related to "some agent".
> 
> For patch 2, it was far easier to utilize what polkit provides
> in pkttyagent and pkcheck utilities, than adding some code which
> requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being
> #defined for compilation.

Sigh, that define is a bit of a bad joke really. polkit was first
added in Fedora 12, and comparing the header files between then
and now, they've never broken their ABI. They're merely added new
APIs.  IMHO, we can just define that, and use the API from libvirt
without trouble.

> 
> I chose 'pkauth' to mean polkit authentication - figured it was
> a workable shorthand, but if there's better suggestions those
> can be considered.

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] syntax-check: Allow plain close in virportallocatortest

2016-02-11 Thread Michal Privoznik

After a03cbfe0fb9 we switched to a plain close() instead of
VIR_FORCE_CLOSE(). This makes sense because it's a mocking
library and thus should not require anything from libvirt.
However, we forgot add an exemption to our 'forbid close' rule.

Signed-off-by: Michal Privoznik 
---
 cfg.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cfg.mk b/cfg.mk
index 3f78842..dbd22a7 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1158,7 +1158,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \
   
^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
 
 exclude_file_name_regexp--sc_prohibit_close = \
-  
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
+  
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir((cgroup|pci)mock|portallocatortest)\.c)$$)
 
 exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
   (^tests/(qemuhelp|nodeinfo|virpcitest)data/|\.diff$$)
-- 
2.4.10

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

Re: [libvirt] [PATCH] syntax-check: Allow plain close in virportallocatortest

On Thu, 2016-02-11 at 13:18 +0100, Michal Privoznik wrote:
> After a03cbfe0fb9 we switched to a plain close() instead of
> VIR_FORCE_CLOSE(). This makes sense because it's a mocking
> library and thus should not require anything from libvirt.
> However, we forgot add an exemption to our 'forbid close' rule.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  cfg.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 3f78842..dbd22a7 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -1158,7 +1158,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \
>    
> ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>  
>  exclude_file_name_regexp--sc_prohibit_close = \
> -  
> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
> +  (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-
> stream\.c|tests/vir((cgroup|pci)mock|portallocatortest)\.c)$$)
>  
>  exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
>    (^tests/(qemuhelp|nodeinfo|virpcitest)data/|\.diff$$)

Yup, my bad.

I'll try to split off the mock part of virportallocatortest.c instead, so we
can still enjoy this check for the non-mock part of the test case. Stay tuned.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH] dbus: Don't unref NULL messages

2016-02-11 Thread Michal Privoznik

Apparently we are not the only ones with dumb free functions
because dbus_message_unref() does not accept NULL either. But if
I were to vote, this one is even more evil. Instead of returning
an error just like we do it immediately dereference any pointer
passed and thus crash you app. Well done DBus!

  Program received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x7f878ebda700 (LWP 31264)]
  0x7f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
  (gdb) bt
  #0  0x7f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
  #1  0x7f87be3f004e in dbus_message_unref () from /usr/lib64/libdbus-1.so.3
  #2  0x7f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at 
util/virsystemd.c:228
  #3  0x7f879761bd4d in qemuConnectCgroup (driver=0x7f87600a32a0, 
vm=0x7f87600c7550) at qemu/qemu_cgroup.c:909
  #4  0x7f87976386b7 in qemuProcessReconnect (opaque=0x7f87600db840) at 
qemu/qemu_process.c:3386
  #5  0x7f87bf6edfff in virThreadHelper (data=0x7f87600d5580) at 
util/virthread.c:206
  #6  0x7f87bb602334 in start_thread (arg=0x7f878ebda700) at 
pthread_create.c:333
  #7  0x7f87bb3481bd in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:109
  (gdb) frame 2
  #2  0x7f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at 
util/virsystemd.c:228
  228 dbus_message_unref(reply);
  (gdb) p reply
  $1 = (DBusMessage *) 0x0

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/rpc/virnetdaemon.c   |  4 ++--
 src/util/virdbus.c   | 14 +++---
 src/util/virdbus.h   |  1 -
 src/util/virfirewall.c   |  3 +--
 src/util/virsystemd.c|  2 +-
 tests/virdbustest.c  | 20 ++--
 tests/virfirewalltest.c  |  3 +--
 tests/virpolkittest.c|  2 +-
 tests/virsystemdtest.c   |  3 ++-
 10 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5ae3618..4cfaed5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1370,6 +1370,7 @@ virDBusHasSystemBus;
 virDBusMessageDecode;
 virDBusMessageEncode;
 virDBusMessageRead;
+virDBusMessageUnref;
 virDBusSetSharedBus;
 
 
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 910f266..18c962c 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -374,7 +374,7 @@ virNetDaemonGotInhibitReply(DBusPendingCall *pending,
 VIR_FORCE_CLOSE(fd);
 }
 }
-dbus_message_unref(reply);
+virDBusMessageUnref(reply);
 
  cleanup:
 virObjectUnlock(dmn);
@@ -426,7 +426,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn,
  dmn, NULL);
 dmn->autoShutdownCallingInhibit = true;
 }
-dbus_message_unref(message);
+virDBusMessageUnref(message);
 }
 #endif
 
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 78fb795..3f4dbe3 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1398,7 +1398,7 @@ int virDBusCreateMethodV(DBusMessage **call,
 }
 
 if (virDBusMessageEncodeArgs(*call, types, args) < 0) {
-dbus_message_unref(*call);
+virDBusMessageUnref(*call);
 *call = NULL;
 goto cleanup;
 }
@@ -1467,7 +1467,7 @@ int virDBusCreateReplyV(DBusMessage **reply,
 }
 
 if (virDBusMessageEncodeArgs(*reply, types, args) < 0) {
-dbus_message_unref(*reply);
+virDBusMessageUnref(*reply);
 *reply = NULL;
 goto cleanup;
 }
@@ -1586,7 +1586,7 @@ virDBusCall(DBusConnection *conn,
 if (ret == 0 && replyout)
 *replyout = reply;
 else
-dbus_message_unref(reply);
+virDBusMessageUnref(reply);
 }
 return ret;
 }
@@ -1650,8 +1650,7 @@ int virDBusCallMethod(DBusConnection *conn,
 ret = virDBusCall(conn, call, replyout, error);
 
  cleanup:
-if (call)
-dbus_message_unref(call);
+virDBusMessageUnref(call);
 return ret;
 }
 
@@ -1727,7 +1726,7 @@ static int virDBusIsServiceInList(const char *listMethod, 
const char *name)
 }
 
  cleanup:
-dbus_message_unref(reply);
+virDBusMessageUnref(reply);
 return ret;
 }
 
@@ -1763,7 +1762,8 @@ int virDBusIsServiceRegistered(const char *name)
 
 void virDBusMessageUnref(DBusMessage *msg)
 {
-dbus_message_unref(msg);
+if (msg)
+dbus_message_unref(msg);
 }
 
 #else /* ! WITH_DBUS */
diff --git a/src/util/virdbus.h b/src/util/virdbus.h
index 9e86538..86b4223 100644
--- a/src/util/virdbus.h
+++ b/src/util/virdbus.h
@@ -28,7 +28,6 @@
 # else
 #  define DBusConnection void
 #  define DBusMessage void
-#  define dbus_message_unref(m) do {} while (0)
 # endif
 # include "internal.h"
 
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index a972c05..f26fd86 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -822,8 +822,7 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
 
  cleanup:
 virResetError();
-if (reply)
-

[libvirt] [PATCH 0/2] tests: Fix mock libraries linking

This patches make sure mock libraries do not link against either
too many or too few libraries.

Cheers.

Andrea Bolognani (2):
  tests: Use plain close() in mock code
  tests: Link mock libraries against gnulib and gnulib only

 tests/Makefile.am| 21 +++--
 tests/virportallocatortest.c |  2 +-
 2 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.5.0

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

[libvirt] [PATCH 2/2] tests: Link mock libraries against gnulib and gnulib only

Mock libraries should not be linked against libvirt, but some of
them did - fix that.

On the other hand, not linking against gnulib can cause build
failures on mingw, so define a new $(MOCKLIBS_LIBS) variable and
use it everywhere.
---
 tests/Makefile.am | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index c5986f0..5f5a561 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -71,6 +71,9 @@ LDADDS = \
$(GNULIB_LIBS) \
../src/libvirt.la
 
+MOCKLIBS_LIBS = \
+   $(GNULIB_LIBS)
+
 EXTRA_DIST =   \
.valgrind.supp \
bhyvexml2argvdata \
@@ -566,6 +569,7 @@ qemuxml2argvmock_la_SOURCES = \
qemuxml2argvmock.c
 qemuxml2argvmock_la_CFLAGS = $(AM_CFLAGS)
 qemuxml2argvmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+qemuxml2argvmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 qemuxml2xmltest_SOURCES = \
qemuxml2xmltest.c testutilsqemu.c testutilsqemu.h \
@@ -621,6 +625,7 @@ qemucaps2xmlmock_la_SOURCES = \
qemucaps2xmlmock.c
 qemucaps2xmlmock_la_CFLAGS = $(AM_CFLAGS)
 qemucaps2xmlmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+qemucaps2xmlmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 qemuagenttest_SOURCES = \
qemuagenttest.c \
@@ -728,6 +733,7 @@ bhyvexml2argvmock_la_SOURCES = \
bhyvexml2argvmock.c
 bhyvexml2argvmock_la_CFLAGS = $(AM_CFLAGS)
 bhyvexml2argvmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+bhyvexml2argvmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 bhyve_LDADDS = ../src/libvirt_driver_bhyve_impl.la
 if WITH_STORAGE
@@ -946,7 +952,7 @@ virnetserverclientmock_la_SOURCES = \
virnetserverclientmock.c
 virnetserverclientmock_la_CFLAGS = $(AM_CFLAGS)
 virnetserverclientmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
-virnetserverclientmock_la_LIBADD = $(GNULIB_LIBS)
+virnetserverclientmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 if WITH_GNUTLS
 virnettlscontexttest_SOURCES = \
@@ -1025,7 +1031,7 @@ libvirportallocatormock_la_SOURCES = \
virportallocatortest.c
 libvirportallocatormock_la_CFLAGS = $(AM_CFLAGS) -DMOCK_HELPER=1
 libvirportallocatormock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
-libvirportallocatormock_la_LIBADD = ../src/libvirt.la
+libvirportallocatormock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 vircgrouptest_SOURCES = \
vircgrouptest.c testutils.h testutils.c
@@ -1035,6 +1041,7 @@ vircgroupmock_la_SOURCES = \
vircgroupmock.c
 vircgroupmock_la_CFLAGS = $(AM_CFLAGS)
 vircgroupmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+vircgroupmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 vircryptotest_SOURCES = \
vircryptotest.c testutils.h testutils.c
@@ -1051,14 +1058,14 @@ virpcitest_LDADD = $(LDADDS)
 virpcimock_la_SOURCES = \
virpcimock.c
 virpcimock_la_CFLAGS = $(AM_CFLAGS)
-virpcimock_la_LIBADD = $(GNULIB_LIBS) \
-  ../src/libvirt.la
 virpcimock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virpcimock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 nodeinfomock_la_SOURCES = \
nodeinfomock.c
 nodeinfomock_la_CFLAGS = $(AM_CFLAGS)
 nodeinfomock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+nodeinfomock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 virnetdevtest_SOURCES = \
virnetdevtest.c testutils.h testutils.c
@@ -1068,9 +1075,8 @@ virnetdevtest_LDADD = $(LDADDS)
 virnetdevmock_la_SOURCES = \
virnetdevmock.c
 virnetdevmock_la_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS)
-virnetdevmock_la_LIBADD = $(GNULIB_LIBS) \
-  ../src/libvirt.la
 virnetdevmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virnetdevmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 virrotatingfiletest_SOURCES = \
virrotatingfiletest.c testutils.h testutils.c
@@ -1089,11 +1095,13 @@ virnetdevbandwidthtest_LDADD = $(LDADDS) $(LIBXML_LIBS)
 virusbmock_la_SOURCES = virusbmock.c
 virusbmock_la_CFLAGS = $(AM_CFLAGS)
 virusbmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virusbmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 virnetdevbandwidthmock_la_SOURCES = \
virnetdevbandwidthmock.c
 virnetdevbandwidthmock_la_CFLAGS = $(AM_CFLAGS)
 virnetdevbandwidthmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virnetdevbandwidthmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 else ! WITH_LINUX
EXTRA_DIST += virusbtest.c virusbmock.c \
@@ -1110,6 +1118,7 @@ virmockdbus_la_SOURCES = \
virmockdbus.c
 virmockdbus_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
 virmockdbus_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virmockdbus_la_LIBADD = $(MOCKLIBS_LIBS)
 
 virpolkittest_SOURCES = \
virpolkittest.c testutils.h testutils.c
-- 
2.5.0

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

[libvirt] VMware driver: SessionIsActive API / Sessions.ValidateSession permission

2016-02-11 Thread Richard W.M. Jones

The VMware driver currently calls the SessionIsActive API, which
requires the vCenter Sessions.ValidateSession permission.

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/esx/esx_vi.c;h=af822b14cfc5ba93c9c2ab4dfa2cb72a23a74a1a;hb=HEAD#l2068

This causes a problem that you have to give this permission to any
libvirt client accessing VMware, and you have to give it from the very
top level of vCenter, all the way down through the Cluster, Folder,
hypervisor levels. This has caused a bit of pushback from virt-v2v
users who consider that the SessionIsActive API is an "admin" API
which they don't want to give out to roles using v2v.

Is calling SessionIsActive necessary? From my (very limited)
understanding, it seems as if we might use 'SessionManager.
currentSession' property instead, which doesn't require admin
permissions. Actually the code [see link above] already does this
when ctx->hasSessionIsActive is false, but that doesn't apply to
modern vCenter.

See also
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=5699034b65afd49d91dff13c46481bea545cbaac
which doesn't really explain why this was added.

Also, why is it even necessary to check if the session is active here?
Shouldn't we just log in unconditionally?

Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org

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

Re: [libvirt] [PATCH 1/3] polkit: Adjust message when action-id isn't found

On Wed, Feb 10, 2016 at 02:46:34PM -0500, John Ferlan wrote:
> When there isn't a ssh -X type session running and a user has not
> been added to the libvirt group, attempts to run 'virsh -c qemu:///system'
> commands from an otherwise unprivileged user will fail with rather
> generic or opaque error message:
> 
> "error: authentication failed: no agent is available to authenticate"
> 
> This patch will adjust the error message to help reflect not only
> that it was a polkit agent, but which 'action-id' was not found. It
> does so carefully, by adding to the text rather than adjusting it
> since virpolkittest is looking for a specific string. The result on
> a failure then becomes:
> 
> "error: authentication failed: no agent is available to authenticate -
>  polkit agent 'org.libvirt.unix.manage' is not available"
> 
> A bit more history on this - at one time a failure generated the
> following type message when running the 'pkcheck' as a subprocess:
> 
> "error: authentication failed: 
> polkit\56retains_authorization_after_challenge=1
> Authorization requires authentication but no agent is available."
> 
> but, a patch was generated to adjust the error message to help provide
> more details about what failed. This was pushed as commit id '96a108c99'.
> That patch prepended a "polkit: " to the output. It really didn't solve
> the problem, but gave a hint.
> 
> After some time it was deemed using DBus API calls directly was a
> better way to go (since pkcheck calls them anyway). So, commit id
> '1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit
> and adjusted it. Then commit id 'c7542573' adjusted the remote.c
> code to call the new API (virPolkitCheckAuth). Finally, commit id
> '308c0c5a' altered the code to call DBus APIs directly. In doing
> so, it reverted the failing error message to the generic message
> that would have been received from DBus anyway.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/util/virpolkit.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
> index 8da91f2..56b1c31 100644
> --- a/src/util/virpolkit.c
> +++ b/src/util/virpolkit.c
> @@ -1,7 +1,7 @@
>  /*
>   * virpolkit.c: helpers for using polkit APIs
>   *
> - * Copyright (C) 2013, 2014 Red Hat, Inc.
> + * Copyright (C) 2013, 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
> @@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid,
>  virReportError(VIR_ERR_AUTH_CANCELLED, "%s",
> _("user cancelled authentication process"));
>  else if (is_challenge)
> -virReportError(VIR_ERR_AUTH_FAILED, "%s",
> -   _("no agent is available to authenticate"));
> +virReportError(VIR_ERR_AUTH_FAILED,
> +   _("no agent is available to authenticate - "
> + "polkit agent '%s' is not available"),
> +   actionid);

That message doesn't make sense I'm afraid. The 'actionid' is referring
to a permission name, not an agent name.

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/3] util: Introduce API's for Polkit text authentication

On Wed, Feb 10, 2016 at 02:46:35PM -0500, John Ferlan wrote:
> Introduce virPolkitAgentCreate, virPolkitAgentCheck, and virPolkitAgentDestroy
> 
> virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
> command in order to handle the local agent authentication via stdin/stdout.
> 
> virPolkitAgentCheck will run the polkit pkcheck command against the async
> command process in order to perform the authentication

Err, we already have virPolkitCheckAuth which does this via the DBus
API. Using pkcheck is a security flaw in many versions of Linux because
it didn't accept the full set of args required for race-free auth
checking.

> virPolkitAgentDestroy will close the command effectively reaping our
> child process
> 
> Needed to move around or add the "#include vircommand.h" since,
> virpolkit.h now uses it.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/libvirt_private.syms |  3 ++
>  src/util/virpolkit.c | 96 
> +++-
>  src/util/virpolkit.h |  7 
>  tests/virpolkittest.c|  3 +-
>  4 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5ae3618..e4d791d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2028,6 +2028,9 @@ virPidFileWritePath;
>  
>  
>  # util/virpolkit.h
> +virPolkitAgentCheck;
> +virPolkitAgentCreate;
> +virPolkitAgentDestroy;
>  virPolkitCheckAuth;
>  
>  
> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
> index 56b1c31..e7c8603 100644
> --- a/src/util/virpolkit.c
> +++ b/src/util/virpolkit.c
> @@ -26,8 +26,8 @@
>  # include 
>  #endif
>  
> -#include "virpolkit.h"
>  #include "vircommand.h"
> +#include "virpolkit.h"
>  #include "virerror.h"
>  #include "virlog.h"
>  #include "virstring.h"
> @@ -136,6 +136,77 @@ int virPolkitCheckAuth(const char *actionid,
>  }
>  
>  
> +/* virPolkitAgentDestroy:
> + * @cmd: Pointer to the virCommandPtr created during virPolkitAgentCreate
> + *
> + * Destroy resources used by Polkit Agent
> + */
> +void
> +virPolkitAgentDestroy(virCommandPtr cmd)
> +{
> +virCommandFree(cmd);
> +}
> +
> +/* virPolkitAgentCreate:
> + *
> + * Allocate and setup a polkit agent
> + *
> + * Returns a virCommandPtr on success and NULL on failure
> + */
> +virCommandPtr
> +virPolkitAgentCreate(void)
> +{
> +virCommandPtr cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
> +int outfd = STDOUT_FILENO;
> +int errfd = STDERR_FILENO;
> +
> +virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
> +virCommandAddArg(cmd, "--fallback");
> +virCommandSetInputFD(cmd, STDIN_FILENO);
> +virCommandSetOutputFD(cmd, );
> +virCommandSetErrorFD(cmd, );
> +if (virCommandRunAsync(cmd, NULL) < 0)
> +goto error;
> +/* Give it a chance to get started */
> +sleep(1);

Sigh, needing a sleep(1) is a sure sign that we'd be better off
doing this via the Polkit API and not spawning programs. This
sleep is just asking for bug reports about race conditions.

> +
> +return cmd;
> +
> + error:
> +virCommandFree(cmd);
> +return NULL;
> +}

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] [RFC] vhost-user + shared memory + NUMA

2016-02-11 Thread Pavel Fedin

 Hello!

 vhost-user has a small limitation: guest memory must be shared. However, this 
simple requirement is satisfied by Libvirt only in
very complicated case:
1. We have to specify NUMA configuration, because we can have "shared" 
attribute only for node descriptors inside "NUMA" section.
2. We have to specify huge page size, because memory-backend-file is used only 
in this case.

 Isn't it a problem? In order to do a simple thing (use vhost-user) we have to 
add two more quote unobvious things, making the whole
stuff significantly more complicated. This creates even more problems on the 
level above, for example in order to get OpenStack
working with userspace networking, we have to edit all flavors and rebuild all 
instances. And no single documentation mentions it.

 Shouldn't Libvirt simply detect usage of vhost-user, and build some minimal 
configuration with shared memory? For example, it could
be memory-backend-file on /dev/mem.
 If the community agrees that it's a good idea, improving the usability, i can 
propose patches.

Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia


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

Re: [libvirt] [RFC] vhost-user + shared memory + NUMA

On Thu, Feb 11, 2016 at 01:28:47PM +0300, Pavel Fedin wrote:
>  Hello!
> 
>  vhost-user has a small limitation: guest memory must be shared. However, 
> this simple requirement is satisfied by Libvirt only in
> very complicated case:
> 1. We have to specify NUMA configuration, because we can have "shared" 
> attribute only for node descriptors inside "NUMA" section.
> 2. We have to specify huge page size, because memory-backend-file is used 
> only in this case.
> 
>  Isn't it a problem? In order to do a simple thing (use vhost-user) we have 
> to add two more quote unobvious things, making the whole
> stuff significantly more complicated. This creates even more problems on the 
> level above, for example in order to get OpenStack
> working with userspace networking, we have to edit all flavors and rebuild 
> all instances. And no single documentation mentions it.
> 
>  Shouldn't Libvirt simply detect usage of vhost-user, and build some minimal 
> configuration with shared memory? For example, it could
> be memory-backend-file on /dev/mem.
>  If the community agrees that it's a good idea, improving the usability, i 
> can propose patches.

Historically QEMU had a pointless check on the path passed in, to enforce
that it was only hugetlbfs, so could not just pass in a regular tmpfs
file. I think we removed that in QEMU 2.5. I think it is a valid enhance
 to allow specification of "shared" memory backing which
would be mapping to a regular tmpfs.

I don't think we should magically do anything based on existance of
vhost-user though - changes in way the guest memory is allocated should
always require explicit user configuration.

We could however report an error VIR_ERR_CONFIG_UNSUPPORTED if the user
provided a vhost-user device and forgot to request shared memory, given
that its an unusable combination.

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] qemu: add support for LSI SAS1068 (aka MPT Fusion) SCSI controller

2016-02-11 Thread Paolo Bonzini

This does nothing more than adding the new device and capability.
The device is present since QEMU 2.6.0.

Signed-off-by: Paolo Bonzini 
---
 src/qemu/qemu_capabilities.c   |  2 +
 src/qemu/qemu_capabilities.h   |  1 +
 src/qemu/qemu_command.c| 13 ++-
 .../qemuxml2argv-disk-scsi-mptsas1068.args | 27 +
 .../qemuxml2argv-disk-scsi-mptsas1068.xml  | 36 ++
 tests/qemuxml2argvtest.c   |  4 ++
 .../qemuxml2xmlout-disk-scsi-mptsas1068.xml| 44 ++
 tests/qemuxml2xmltest.c|  4 ++
 8 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-mptsas1068.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-mptsas1068.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-mptsas1068.xml

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 3099e34..2b953ea 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -314,6 +314,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
   "vserport-change-event", /* 210 */
   "virtio-balloon-pci.deflate-on-oom",
+  "mptsas1068",
 );
 
 
@@ -1567,6 +1568,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "virtio-tablet-pci", QEMU_CAPS_VIRTIO_TABLET },
 { "virtio-input-host-device", QEMU_CAPS_VIRTIO_INPUT_HOST },
 { "virtio-input-host-pci", QEMU_CAPS_VIRTIO_INPUT_HOST },
+{ "mptsas1068", QEMU_CAPS_SCSI_MPTSAS1068 },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index e5353de..b73c529 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -343,6 +343,7 @@ typedef enum {
 QEMU_CAPS_VSERPORT_CHANGE, /* VSERPORT_CHANGE event */
 QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE, /* virtio-balloon-{device,pci,ccw}.
* deflate-on-oom */
+QEMU_CAPS_SCSI_MPTSAS1068, /* -device mptsas1068 */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d7f19f3..4d4d65e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -872,11 +872,19 @@ qemuSetSCSIControllerModel(virDomainDefPtr def,
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
 /*TODO: need checking work here if necessary */
 break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("This QEMU doesn't support "
+ "the LSI SAS1068 (MPT Fusion) controller"));
+return -1;
+}
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("This QEMU doesn't support "
- "the LSI SAS1078 controller"));
+ "the LSI SAS1078 (MegaRAID) controller"));
 return -1;
 }
 break;
@@ -4753,6 +4761,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
 virBufferAddLit(, "spapr-vscsi");
 break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+virBufferAddLit(, "mptsas1068");
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
 virBufferAddLit(, "megasas");
 break;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-mptsas1068.args 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-mptsas1068.args
new file mode 100644
index 000..bdfc1af
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-mptsas1068.args
@@ -0,0 +1,27 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefconfig \
+-nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait \
+-no-acpi \
+-boot c \
+-device mptsas1068,id=scsi0,bus=pci.0,addr=0x3 \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-drive file=/tmp/scsidisk.img,format=raw,if=none,id=drive-scsi0-0-4-0 \
+-device 
scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,drive=drive-scsi0-0-4-0,\

Re: [libvirt] [PATCH 0/2] tests: Fix mock libraries linking

On Thu, Feb 11, 2016 at 12:15:39PM +0100, Andrea Bolognani wrote:
> This patches make sure mock libraries do not link against either
> too many or too few libraries.
> 
> Cheers.
> 
> Andrea Bolognani (2):
>   tests: Use plain close() in mock code
>   tests: Link mock libraries against gnulib and gnulib only
> 
>  tests/Makefile.am| 21 +++--
>  tests/virportallocatortest.c |  2 +-
>  2 files changed, 16 insertions(+), 7 deletions(-)

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

Re: [libvirt] [PATCH 4/4] conf: snapshot: Avoid autogenerating duplicate snapshot names



On 11/02/16 10:20, Peter Krempa wrote:
> The snapshot name generator truncates the original file name after a '.'
> and replaces the suffix with the snapshot name. If two disks source
> images differ only in the suffix portion, the generated name will be
> duplicate.
> 
> Since this is a corner case just error out stating that a duplicate name
> was generated. The user can work around this situation by providing
> the file names explicitly.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085
> ---
>  src/conf/snapshot_conf.c | 12 
>  1 file changed, 12 insertions(+)
> 
...

ACK

Erik

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

Re: [libvirt] [PATCH] migration: add option to set target ndb server port

2016-02-11 Thread Maxim Nestratov


29.01.2016 18:18, Maxim Nestratov пишет:

13.01.2016 14:01, Nikolay Shirokovskiy пишет:

Current libvirt + qemu pair lacks secure migrations in case of
VMs with non-shared disks. The only option to migrate securely
natively is to use tunneled mode and some kind of secure
destination URI. But tunelled mode does not support non-shared
disks.

The other way to make migration secure is to organize a tunnel
by external means. This is possible in case of shared disks
migration thru use of proper combination of destination URI,
migration URI and VIR_MIGRATE_PARAM_LISTEN_ADDRESS migration
param. But again this is not possible in case of non shared disks
migration as we have no option to control target nbd server port.
But fixing this much more simplier that supporting non-shared
disks in tunneled mode.

So this patch adds option to set target ndb port.

Finally all qemu migration connections will be secured AFAIK but
even in this case this patch could be convinient if one wants
all migration traffic be put in a single connection.
---
  include/libvirt/libvirt-domain.h |  10 +++
  src/qemu/qemu_driver.c   |  25 ---
  src/qemu/qemu_migration.c| 138 
+++

  src/qemu/qemu_migration.h|   3 +
  tools/virsh-domain.c |  12 
  tools/virsh.pod  |   5 +-
  6 files changed, 141 insertions(+), 52 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h 
b/include/libvirt/libvirt-domain.h

index d26faa5..e577f26 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -757,6 +757,16 @@ typedef enum {
   */
  # define VIR_MIGRATE_PARAM_MIGRATE_DISKS"migrate_disks"
  +/**
+ * VIR_MIGRATE_PARAM_NBD_PORT:
+ *
+ * virDomainMigrate* params field: port that destination nbd server 
should use
+ * for incoming disks migration. Type is VIR_TYPED_PARAM_INT. If set 
to 0 or
+ * omitted, libvirt will choose a suitable default. At the moment 
this is only

+ * supported by the QEMU driver.
+ */
+# define VIR_MIGRATE_PARAM_NBD_PORT"nbd_port"
+
  /* Domain migration. */
  virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr 
dconn,

 unsigned long flags, const char *dname,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ccf68b..4d00ba4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12082,7 +12082,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn,
  ret = qemuMigrationPrepareDirect(driver, dconn,
   NULL, 0, NULL, NULL, /* No 
cookies */

   uri_in, uri_out,
- , origname, NULL, 0, NULL, 
flags);
+ , origname, NULL, 0, NULL, 
0, flags);

 cleanup:
  VIR_FREE(origname);
@@ -12135,7 +12135,7 @@ qemuDomainMigratePerform(virDomainPtr dom,
   * Consume any cookie we were able to decode though
   */
  ret = qemuMigrationPerform(driver, dom->conn, vm,
-   NULL, dconnuri, uri, NULL, NULL, 0, 
NULL,
+   NULL, dconnuri, uri, NULL, NULL, 0, 
NULL, 0,

 cookie, cookielen,
 NULL, NULL, /* No output cookies in 
v2 */

 flags, dname, resource, false);
@@ -12308,7 +12308,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn,
   cookiein, cookieinlen,
   cookieout, cookieoutlen,
   uri_in, uri_out,
- , origname, NULL, 0, NULL, 
flags);
+ , origname, NULL, 0, NULL, 
0, flags);

 cleanup:
  VIR_FREE(origname);
@@ -12334,6 +12334,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr 
dconn,

  const char *dname = NULL;
  const char *uri_in = NULL;
  const char *listenAddress = cfg->migrationAddress;
+int nbdPort = 0;
  int nmigrate_disks;
  const char **migrate_disks = NULL;
  char *origname = NULL;
@@ -12354,7 +12355,10 @@ 
qemuDomainMigratePrepare3Params(virConnectPtr dconn,

  _in) < 0 ||
  virTypedParamsGetString(params, nparams,
VIR_MIGRATE_PARAM_LISTEN_ADDRESS,
-) < 0)
+) < 0 ||
+virTypedParamsGetInt(params, nparams,
+VIR_MIGRATE_PARAM_NBD_PORT,
+) < 0)
  goto cleanup;
nmigrate_disks = virTypedParamsGetStringList(params, nparams,
@@ -12385,7 +12389,8 @@ qemuDomainMigratePrepare3Params(virConnectPtr 
dconn,

   cookieout, cookieoutlen,
   uri_in, uri_out,
   , origname, listenAddress,
-

Re: [libvirt] [PATCH v7 00/13] qemu: Add quorum support to libvirt

On Wed, Feb 10, 2016 at 17:01:22 +0200, Alberto Garcia wrote:
> On Tue, Jan 26, 2016 at 05:36:36PM +0100, Peter Krempa wrote:

[...]

> > Whoah. Data corruption accross network? I'm not quite sure whether
> > I'd use this to cover up a problem with the storage technology or
> > network rather than just fix the root cause. If you have 3 copies,
> > and manage to have a sector where all 3 differ then the quorum
> > driver won't help. And it will make it even harder to find any
> > possible problems.
> 
> But in that case you detect that it went wrong and you get an I/O
> error. The problem with silent data corruption is that it can be hard
> to detect.

Yes, and that's why it should be fixed at the network storage technology
layer rather than anywhere else.

> 
> If there's a bit-flip across the network Quorum can detect it,
> report it and correct the faulty version without needing to rebuild
> everything.

I still think that you do wan't to rebulild the whole volume in such
case if you care about your data in the slightest. Otherwise you don't
have to do stuff like this.

[...]

> > > Quorum is also used for the COLO block replication functionality
> > > currently being discussed in QEMU:
> > > 
> > >http://wiki.qemu.org/Features/BlockReplication

[...]

> Yes, I just wanted to point out one other example of how Quorum is
> being used. This current series of Quorum for libvirt is not taking
> COLO into account at all, in fact it is still under review in QEMU.

Yes and there are apparently some design issues/major problems. If they
are going to use this, we should probably wait on the result of that
discussion.

[...]

> > 1) Apart from abusing quorums in fifo mode for COLO I still don't
> > think that they are hugely useful. (no, data corruption on NFS
> > didn't persuade me)
> 
> It is one of the main reasons why Quorum was written. Here's one more
> example of silent data corruption over the network:
> 
> https://cds.cern.ch/record/2026187/files/Adler32_Data_Corruption.pdf

That underlines the fact that the network storage protocol does a
terrible job in this case. Additionally in the described case the
highlighted advantage of adler32 is speed. Patching that with
triplicating the data and necessary bandwidth to transfer it does not
stack with that really well. Additionally the regular use case remains
still broken.

> > 2) The implementation in this series as in current state adds a lot
> > of code to mintain that wouldn't much used be and is incomplete in
> > many aspects:

[...]

> > * no support for the quorum failure events and reporting
> > * no way to control 'rewrite-corrupted'
> 
> I can look into these.
> 
> > * since we don't use node-names yet, it's not really possible to do
> >   block jobs on quorum disks, thus they are forbidden
> 
> I'm not sure what's the status of node names in libvirt, I could also
> try to help to make it happen.

They are basically non-existent. To be honest I think that the node name
support stuff and better approach at constructing block devices and
their backing chains and better handling of block jobs  should be done
prior to quorum.

This series tries to partially do the stuff that is a plan how to
approach some stuff regarding disks. One of them is that the backing
chain of a disk is persisted in the XML and then fully constructed.

By adding this code the refactor will be even more painful as it will
currently be.

I'm actually planing to do this in short term future, but unfortunately
this is not a weekend project.

> 
> > * since block jobs are forbidden and rewrite-corrupted can't be
> > * enabled, no way to do the rebuild
> 
> 'rewrite-corrupted' can be easily added to the series so I don't
> think that's a problem. The block jobs thing I would need to see
> first. Would you really need to have node names in order to rebuild a
> Quorum?

Most probably yes. Without them, it will be just an ugly hack.

Peter

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

Re: [libvirt] [PATCHv3 0/2] notify about reverting to a snapshot

On Tue, Jan 05, 2016 at 17:19:19 -0500, John Ferlan wrote:
> On 12/23/2015 09:25 AM, Dmitry Andreev wrote:
> > Reverting to snapshot may change domain configuration but
> > there will be no events about that.

I'm afraid there will be more places like this ...

> > 
> > Lack of the event become a problem for virt-manager 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1081148
> > 
> > This patch-set introduces new event and emits it in
> > qemuDomainRevertToSnapshot.

[...]

> > 
> > 
> 
> This seems to be a reasonable approach; however, since Peter understands
> the snapshot code better than I do, I expect he will want to chime in
> (I've CC'd him).

I don't currently see a problem with emiting the event. We indeed
replace the complete definition with something else so a 'DEFINED'
event is probably appropriate.

Peter


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

Re: [libvirt] [PATCH 2/4] snapshot: conf: Extract code to generate default external file names

On 11/02/16 10:20, Peter Krempa wrote:
> ---
>  src/conf/snapshot_conf.c | 131 
> ---
>  1 file changed, 77 insertions(+), 54 deletions(-)
> 
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 65a78d6..da6fec5 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -429,6 +429,80 @@ virDomainSnapshotDefParseString(const char *xmlStr,
>  return ret;
>  }
> ...

ACK

Erik

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

Re: [libvirt] [RFC] vhost-user + shared memory + NUMA

On Thu, Feb 11, 2016 at 01:54:49PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > Historically QEMU had a pointless check on the path passed in, to enforce
> > that it was only hugetlbfs, so could not just pass in a regular tmpfs
> > file. I think we removed that in QEMU 2.5. I think it is a valid enhance
> >  to allow specification of "shared" memory backing which
> > would be mapping to a regular tmpfs.
> > 
> > I don't think we should magically do anything based on existance of
> > vhost-user though - changes in way the guest memory is allocated should
> > always require explicit user configuration.
> 
>  Ok, then would it be a good compromise if we require , and 
> only implicitly add "shared" if we have vhost-user
> devices? This way we would not change the way the guest memory is allocated.

Adding shared implicitly *will* change the way guest memory is allocated,
as it will have to use tmpfs to make it shared.

>  IMHO being able to manually specify "shared" both in  and
> in  would be ambiguous.

That's not really any different to what we have already with NUMA.
The top level setting would apply as the default, and the NUMA level
settings override it if needed.

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 3/4] conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames

On 11/02/16 10:20, Peter Krempa wrote:
> Get rid of one indentation level by negating condition and remove ugly
> pointer arithmetic at the cost of one extra allocation.
> ---
>  src/conf/snapshot_conf.c | 94 
> +++-
>  1 file changed, 44 insertions(+), 50 deletions(-)
> 
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index da6fec5..f8a1aed 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -440,66 +440,60 @@ virDomainSnapshotDefParseString(const char *xmlStr,
> ...
> ... 
> -tmp = strrchr(original, '.');
> -if (!tmp || strchr(tmp, '/')) {
> -if (virAsprintf(>src->path, "%s.%s", original,
> -def->name) < 0)
> -goto cleanup;
> -} else {
> -if ((tmp - original) > INT_MAX) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("integer overflow"));
> -goto cleanup;
> -}
> -if (virAsprintf(>src->path, "%.*s.%s",
> -(int) (tmp - original), original,
> -def->name) < 0)
> -goto cleanup;
> -}

This was black magic indeed.

> +if (!(origpath = virDomainDiskGetSource(def->dom->disks[i]))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("cannot generate external snapshot name "
> + "for disk '%s' without source"),
> +   disk->name);
> +return -1;
>  }
> -}
> 
> -ret = 0;
> +if (stat(origpath, ) < 0 || !S_ISREG(sb.st_mode)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("source for disk '%s' is not a regular "
> + "file; refusing to generate external "
> + "snapshot name"),
> +   disk->name);
> +return -1;
> +}
> 
> - cleanup:
> -return ret;
> +if (VIR_STRDUP(tmppath, origpath) < 0)
> +return -1;
> +
> +/* drop suffix of the file name */
> +if ((tmp = strrchr(tmppath, '.')) && !strchr(tmp, '/'))
> +*tmp = '\0';
> +
> +if (virAsprintf(>src->path, "%s.%s", tmppath, def->name) < 0) {
> +VIR_FREE(tmppath);
> +return -1;
> +}
> +
> +VIR_FREE(tmppath);
> +}
> +
> +return 0;
>  }
> 

Nicely done, ACK.

Erik

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

[libvirt] [PATCH 1/4] tests: Split off the mock part of the port allocator test

Instead of compiling either the mock or the non-mock part of the
file based on a compiler flag, split the mock part off to its
own file.
---
 tests/Makefile.am|   4 +-
 tests/virportallocatormock.c | 108 ++
 tests/virportallocatortest.c | 110 +++
 3 files changed, 117 insertions(+), 105 deletions(-)
 create mode 100644 tests/virportallocatormock.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5f5a561..1fe7d6b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1028,8 +1028,8 @@ virportallocatortest_SOURCES = \
 virportallocatortest_LDADD = $(LDADDS)
 
 libvirportallocatormock_la_SOURCES = \
-   virportallocatortest.c
-libvirportallocatormock_la_CFLAGS = $(AM_CFLAGS) -DMOCK_HELPER=1
+   virportallocatormock.c
+libvirportallocatormock_la_CFLAGS = $(AM_CFLAGS)
 libvirportallocatormock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
 libvirportallocatormock_la_LIBADD = $(MOCKLIBS_LIBS)
 
diff --git a/tests/virportallocatormock.c b/tests/virportallocatormock.c
new file mode 100644
index 000..e44191e
--- /dev/null
+++ b/tests/virportallocatormock.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2013-2014 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: Daniel P. Berrange 
+ */
+
+#include 
+#include 
+
+#if HAVE_DLFCN_H
+# include 
+#endif
+
+#if defined(RTLD_NEXT)
+# include "internal.h"
+# include 
+# include 
+# include 
+# include 
+# include 
+# include 
+
+static bool host_has_ipv6;
+static int (*realsocket)(int domain, int type, int protocol);
+
+static void init_syms(void)
+{
+int fd;
+
+if (realsocket)
+return;
+
+realsocket = dlsym(RTLD_NEXT, "socket");
+
+if (!realsocket) {
+fprintf(stderr, "Unable to find 'socket' symbol\n");
+abort();
+}
+
+fd = realsocket(AF_INET6, SOCK_STREAM, 0);
+if (fd < 0)
+return;
+
+host_has_ipv6 = true;
+close(fd);
+}
+
+int socket(int domain,
+   int type,
+   int protocol)
+{
+init_syms();
+
+if (getenv("LIBVIRT_TEST_IPV4ONLY") && domain == AF_INET6) {
+errno = EAFNOSUPPORT;
+return -1;
+}
+
+return realsocket(domain, type, protocol);
+}
+
+int bind(int sockfd ATTRIBUTE_UNUSED,
+ const struct sockaddr *addr,
+ socklen_t addrlen ATTRIBUTE_UNUSED)
+{
+struct sockaddr_in saddr;
+
+memcpy(, addr, sizeof(saddr));
+
+if (host_has_ipv6 && !getenv("LIBVIRT_TEST_IPV4ONLY")) {
+if (saddr.sin_port == htons(5900) ||
+(saddr.sin_family == AF_INET &&
+ saddr.sin_port == htons(5904)) ||
+(saddr.sin_family == AF_INET6 &&
+ (saddr.sin_port == htons(5905) ||
+  saddr.sin_port == htons(5906 {
+errno = EADDRINUSE;
+return -1;
+}
+return 0;
+}
+
+if (saddr.sin_port == htons(5900) ||
+saddr.sin_port == htons(5904) ||
+saddr.sin_port == htons(5905) ||
+saddr.sin_port == htons(5906)) {
+errno = EADDRINUSE;
+return -1;
+}
+
+return 0;
+}
+
+#endif /* ! defined(RTLD_NEXT) */
diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c
index 077aad8..8a8f413 100644
--- a/tests/virportallocatortest.c
+++ b/tests/virportallocatortest.c
@@ -22,101 +22,14 @@
 #include 
 #include "virfile.h"
 #include "testutils.h"
+#include "virutil.h"
+#include "virerror.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virportallocator.h"
+#include "virstring.h"
 
-#if HAVE_DLFCN_H
-# include 
-#endif
-
-#if defined(RTLD_NEXT)
-# ifdef MOCK_HELPER
-#  include "internal.h"
-#  include 
-#  include 
-#  include 
-#  include 
-#  include 
-
-static bool host_has_ipv6;
-static int (*realsocket)(int domain, int type, int protocol);
-
-static void init_syms(void)
-{
-int fd;
-
-if (realsocket)
-return;
-
-realsocket = dlsym(RTLD_NEXT, "socket");
-
-if (!realsocket) {
-fprintf(stderr, "Unable to find 'socket' symbol\n");
-abort();
-}
-
-fd = realsocket(AF_INET6, SOCK_STREAM, 0);
-if (fd < 0)
-return;
-
-host_has_ipv6 = true;
-close(fd);
-}
-
-int socket(int domain,
-   int type,
-   int protocol)
-{
-init_syms();
-
-if

[libvirt] [PATCH 2/4] tests: Allow use of close() in mock libraries

As mock libraries are not to be linked against libvirt, the
sc_prohibit_close syntax-check rule does not apply.

This fixes a syntax-check failure introduced by commit a03cbfe0fb.
---
 cfg.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cfg.mk b/cfg.mk
index 3f78842..5b864af 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1158,7 +1158,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \
   
^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
 
 exclude_file_name_regexp--sc_prohibit_close = \
-  
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
+  
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
 
 exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
   (^tests/(qemuhelp|nodeinfo|virpcitest)data/|\.diff$$)
-- 
2.5.0

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

[libvirt] [PATCH 0/4] tests: Fix syntax-check failure, plus extras

Commit a03cbfe0fb introduced a syntax-check failure: patch
1/4 prepares for 2/4, where the failure is actually fixed.

Patches 3/4 and 4/4 improve the naming consistency for mock
libraries by shuffling files around a bit.

Cheers.


Andrea Bolognani (4):
  tests: Split off the mock part of the port allocator test
  tests: Allow use of close() in mock libraries
  tests: Don't use "lib" prefix for mock libraries
  tests: Rename virmockdbus -> virdbusmock for consistency

 cfg.mk |   2 +-
 tests/Makefile.am  |  26 
 tests/{virmockdbus.c => virdbusmock.c} |   2 +-
 tests/virfirewalltest.c|   2 +-
 tests/virpolkittest.c  |   2 +-
 tests/virportallocatormock.c   | 108 +++
 tests/virportallocatortest.c   | 112 +++--
 tests/virsystemdtest.c |   2 +-
 8 files changed, 134 insertions(+), 122 deletions(-)
 rename tests/{virmockdbus.c => virdbusmock.c} (97%)
 create mode 100644 tests/virportallocatormock.c

-- 
2.5.0

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

[libvirt] [PATCH 3/4] tests: Don't use "lib" prefix for mock libraries

virportallocatormock was the only one using it, and has been
changed accordingly.
---
 tests/Makefile.am| 10 +-
 tests/virportallocatortest.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1fe7d6b..359610f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -415,7 +415,7 @@ endif ! WITH_SECDRIVER_APPARMOR
 EXTRA_DIST += $(test_scripts)
 
 test_libraries = libshunload.la \
-   libvirportallocatormock.la \
+   virportallocatormock.la \
virnetserverclientmock.la \
vircgroupmock.la \
virpcimock.la \
@@ -1027,11 +1027,11 @@ virportallocatortest_SOURCES = \
virportallocatortest.c testutils.h testutils.c
 virportallocatortest_LDADD = $(LDADDS)
 
-libvirportallocatormock_la_SOURCES = \
+virportallocatormock_la_SOURCES = \
virportallocatormock.c
-libvirportallocatormock_la_CFLAGS = $(AM_CFLAGS)
-libvirportallocatormock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
-libvirportallocatormock_la_LIBADD = $(MOCKLIBS_LIBS)
+virportallocatormock_la_CFLAGS = $(AM_CFLAGS)
+virportallocatormock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virportallocatormock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 vircgrouptest_SOURCES = \
vircgrouptest.c testutils.h testutils.c
diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c
index 8a8f413..6f8ec65 100644
--- a/tests/virportallocatortest.c
+++ b/tests/virportallocatortest.c
@@ -167,4 +167,4 @@ mymain(void)
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir 
"/.libs/libvirportallocatormock.so")
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virportallocatormock.so")
-- 
2.5.0

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

[libvirt] [PATCH 4/4] tests: Rename virmockdbus -> virdbusmock for consistency

All mock libraries were called vir*mock except for this one; now
the naming is consistent across the board.
---
 tests/Makefile.am  | 14 +++---
 tests/{virmockdbus.c => virdbusmock.c} |  2 +-
 tests/virfirewalltest.c|  2 +-
 tests/virpolkittest.c  |  2 +-
 tests/virsystemdtest.c |  2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)
 rename tests/{virmockdbus.c => virdbusmock.c} (97%)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 359610f..90981dc 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -435,7 +435,7 @@ endif WITH_BHYVE
 
 if WITH_DBUS
 test_libraries += \
-   virmockdbus.la
+   virdbusmock.la
 endif WITH_DBUS
 
 if WITH_LINUX
@@ -1114,11 +1114,11 @@ virdbustest_SOURCES = \
 virdbustest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
 virdbustest_LDADD = $(LDADDS) $(DBUS_LIBS)
 
-virmockdbus_la_SOURCES = \
-   virmockdbus.c
-virmockdbus_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
-virmockdbus_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
-virmockdbus_la_LIBADD = $(MOCKLIBS_LIBS)
+virdbusmock_la_SOURCES = \
+   virdbusmock.c
+virdbusmock_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
+virdbusmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virdbusmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 virpolkittest_SOURCES = \
virpolkittest.c testutils.h testutils.c
@@ -1131,7 +1131,7 @@ virsystemdtest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
 virsystemdtest_LDADD = $(LDADDS) $(DBUS_LIBS)
 
 else ! WITH_DBUS
-EXTRA_DIST += virdbustest.c virmockdbus.c virsystemdtest.c
+EXTRA_DIST += virdbustest.c virdbusmock.c virsystemdtest.c
 endif ! WITH_DBUS
 
 viruritest_SOURCES = \
diff --git a/tests/virmockdbus.c b/tests/virdbusmock.c
similarity index 97%
rename from tests/virmockdbus.c
rename to tests/virdbusmock.c
index 4261e6a..a62689e 100644
--- a/tests/virmockdbus.c
+++ b/tests/virdbusmock.c
@@ -1,5 +1,5 @@
 /*
- * virmockdbus.c: mocking of dbus message send/reply
+ * virdbusmock.c: mocking of dbus message send/reply
  *
  * Copyright (C) 2013-2014 Red Hat, Inc.
  *
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index 8f6fc9e..f1f29c6 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -1184,7 +1184,7 @@ mymain(void)
 }
 
 # if WITH_DBUS
-VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmockdbus.so")
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virdbusmock.so")
 # else
 VIRT_TEST_MAIN(mymain)
 # endif
diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
index b39beed..1ef7635 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -349,7 +349,7 @@ mymain(void)
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmockdbus.so")
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virdbusmock.so")
 
 #else /* ! (WITH_DBUS && __linux__) */
 int
diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
index 101f5e0..5e72de4 100644
--- a/tests/virsystemdtest.c
+++ b/tests/virsystemdtest.c
@@ -602,7 +602,7 @@ mymain(void)
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmockdbus.so")
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virdbusmock.so")
 
 #else /* ! (WITH_DBUS && __linux__) */
 int
-- 
2.5.0

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

Re: [libvirt] [PATCH 1/4] conf: snapshot: Rename disksorter to virDomainSnapshotCompareDiskIndex

On 11/02/16 10:20, Peter Krempa wrote:
> Stick to the naming pattern.
> ---
>  src/conf/snapshot_conf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 1eda7c2..65a78d6 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -430,7 +430,7 @@ virDomainSnapshotDefParseString(const char *xmlStr,
>  }
> 
>  static int
> -disksorter(const void *a, const void *b)
> +virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
>  {
>  const virDomainSnapshotDiskDef *diska = a;
>  const virDomainSnapshotDiskDef *diskb = b;
> @@ -562,7 +562,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
>  disk->snapshot = default_snapshot;
>  }
> 
> -qsort(>disks[0], def->ndisks, sizeof(def->disks[0]), disksorter);
> +qsort(>disks[0], def->ndisks, sizeof(def->disks[0]),
> +  virDomainSnapshotCompareDiskIndex);
> 
>  /* Generate any default external file names, but only if the
>   * backing file is a regular file.  */
> 

ACK

Erik

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

[libvirt] [PATCH 1/3] rbd: Add volStorageBackendRBDGetFeatures() for internal calls

As more and more features are added to RBD volumes we will need to
call this method more often.

By moving it into a internal function we can re-use code inside the
storage backend.

Signed-off-by: Wido den Hollander 
---
 src/storage/storage_backend_rbd.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index 5d73370..5f2469f 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -280,6 +280,24 @@ 
virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr)
 }
 
 static int
+volStorageBackendRBDGetFeatures(rbd_image_t image,
+const char *volname,
+uint64_t *features)
+{
+int r, ret = -1;
+
+if ((r = rbd_get_features(image, features)) < 0) {
+virReportSystemError(-r, _("failed to get the features of RBD image "
+ "%s"), volname);
+goto cleanup;
+}
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+static int
 volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
virStoragePoolObjPtr pool,
virStorageBackendRBDStatePtr ptr)
@@ -685,11 +703,8 @@ virStorageBackendRBDImageInfo(rbd_image_t image,
 goto cleanup;
 }
 
-if ((r = rbd_get_features(image, features)) < 0) {
-virReportSystemError(-r, _("failed to get the features of RBD image 
%s"),
- volname);
+if (volStorageBackendRBDGetFeatures(image, volname, features) < 0)
 goto cleanup;
-}
 
 if ((r = rbd_get_stripe_unit(image, stripe_unit)) < 0) {
 virReportSystemError(-r, _("failed to get the stripe unit of RBD image 
%s"),
-- 
1.9.1

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

[libvirt] [PATCH 2/3] rbd: rbd_diff_iterate2() is available in librbd since 266

In commit 0b15f920 there is a #ifdef which requires LIBRBD_VERSION_CODE
266 or newer for rbd_diff_iterate2()

rbd_diff_iterate2() is available since 266, so this if-statement should
require anything newer than 265.

Signed-off-by: Wido den Hollander 
---
 src/storage/storage_backend_rbd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index 5f2469f..887db0b 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -792,8 +792,10 @@ virStorageBackendRBDSnapshotFindNoDiff(rbd_image_t image,
  * rbd_diff_iterate2() is available in versions above Ceph 0.94 (Hammer)
  * It uses a object map inside Ceph which is faster than rbd_diff_iterate()
  * which iterates all objects.
+ * LIBRBD_VERSION_CODE for Ceph 0.94 is 265. In 266 and upwards diff_iterate2
+ * is available
  */
-#if LIBRBD_VERSION_CODE > 266
+#if LIBRBD_VERSION_CODE > 265
 r = rbd_diff_iterate2(image, snaps[i].name, 0, info.size, 0, 1,
   virStorageBackendRBDIterateCb, (void *));
 #else
-- 
1.9.1

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

[libvirt] Improvements to the RBD storage backend

Hi,

After review by John Ferlan I'm sending a revised version of my earlier
patch which adds the fast-diff feature for RBD volumes.

Before the patch there is one small improvement which is later used
by the fast-diff feature.

The other is a bugfix where the #ifdef in the code wasn't requiring the
right version of librbd.

Wido

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

[libvirt] [PATCH 3/3] rbd: Use RBD fast-diff for querying actual volume allocation

Since Ceph version Infernalis (9.2.0) the new fast-diff mechanism
of RBD allows for querying actual volume usage.

Prior to this version there was no easy and fast way to query how
much allocation a RBD volume had inside a Ceph cluster.

To use the fast-diff feature it needs to be enabled per RBD image
and is only supported by Ceph cluster running version Infernalis
(9.2.0) or newer.

Without the fast-diff feature enabled libvirt will report an allocation
identical to the image capacity. This is how libvirt behaves currently.

'virsh vol-info rbd/image2' might output for example:

  Name:   image2
  Type:   network
  Capacity:   1,00 GiB
  Allocation: 124,00 MiB

Newly created volumes will have the fast-diff feature enabled if the
backing Ceph cluster supports it.

Signed-off-by: Wido den Hollander 
---
 src/storage/storage_backend_rbd.c | 84 +--
 1 file changed, 81 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index 887db0b..c00f6b2 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -297,6 +297,68 @@ volStorageBackendRBDGetFeatures(rbd_image_t image,
 return ret;
 }
 
+#if LIBRBD_VERSION_CODE > 265
+static bool
+volStorageBackendRBDUseFastDiff(uint64_t features)
+{
+return features & RBD_FEATURE_FAST_DIFF;
+}
+
+static int
+virStorageBackendRBDRefreshVolInfoCb(uint64_t offset ATTRIBUTE_UNUSED,
+ size_t len,
+ int exists,
+ void *arg)
+{
+uint64_t *used_size = (uint64_t *)(arg);
+if (exists)
+(*used_size) += len;
+
+return 0;
+}
+
+static int
+virStorageBackendRBDSetAllocation(virStorageVolDefPtr vol,
+  rbd_image_t *image,
+  rbd_image_info_t *info)
+{
+int r, ret = -1;
+uint64_t allocation = 0;
+
+if ((r = rbd_diff_iterate2(image, NULL, 0, info->size, 0, 1,
+   ,
+   )) < 0) {
+virReportSystemError(-r, _("failed to iterate RBD image '%s'"),
+ vol->name);
+goto cleanup;
+}
+
+VIR_DEBUG("Found %zu bytes allocated for RBD image %s",
+  allocation, vol->name);
+
+vol->target.allocation = allocation;
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+#else
+static int
+volStorageBackendRBDUseFastDiff(uint64_t features ATTRIBUTE_UNUSED)
+{
+return false;
+}
+
+static int
+virStorageBackendRBDSetAllocation(virStorageVolDefPtr vol ATTRIBUTE_UNUSED,
+  rbd_image_t *image ATTRIBUTE_UNUSED,
+  rbd_image_info_t *info ATTRIBUTE_UNUSED)
+{
+return false;
+}
+#endif
+
 static int
 volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
virStoragePoolObjPtr pool,
@@ -306,6 +368,7 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
 int r = 0;
 rbd_image_t image = NULL;
 rbd_image_info_t info;
+uint64_t features;
 
 if ((r = rbd_open_read_only(ptr->ioctx, vol->name, , NULL)) < 0) {
 ret = -r;
@@ -321,15 +384,30 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr 
vol,
 goto cleanup;
 }
 
-VIR_DEBUG("Refreshed RBD image %s/%s (size: %zu obj_size: %zu num_objs: 
%zu)",
-  pool->def->source.name, vol->name, info.size, info.obj_size,
-  info.num_objs);
+if (volStorageBackendRBDGetFeatures(image, vol->name, ) < 0)
+goto cleanup;
 
 vol->target.capacity = info.size;
 vol->target.allocation = info.obj_size * info.num_objs;
 vol->type = VIR_STORAGE_VOL_NETWORK;
 vol->target.format = VIR_STORAGE_FILE_RAW;
 
+if (volStorageBackendRBDUseFastDiff(features)) {
+VIR_DEBUG("RBD image %s/%s has fast-diff feature enabled. "
+  "Querying for actual allocation",
+  pool->def->source.name, vol->name);
+
+if (virStorageBackendRBDSetAllocation(vol, image, ) < 0)
+goto cleanup;
+} else {
+vol->target.allocation = info.obj_size * info.num_objs;
+}
+
+VIR_DEBUG("Refreshed RBD image %s/%s (capacity: %llu allocation: %llu "
+  "obj_size: %zu num_objs: %zu)",
+  pool->def->source.name, vol->name, vol->target.capacity,
+  vol->target.allocation, info.obj_size, info.num_objs);
+
 VIR_FREE(vol->target.path);
 if (virAsprintf(>target.path, "%s/%s",
 pool->def->source.name,
-- 
1.9.1

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

[libvirt] [PATCH 1/3] network: consolidate connection count updates for device pool

networkReleaseActualDevice() and networkNotifyActualDevice() both were
updating the individual devices' connections count in two separate
places (unlike networkAllocateActualDevice() which does it in a single
unified place after success:). The code is correct, but prone to
confusion / later breakage. All of these updates are anyway located at
the end of if/else clauses that are (with the exception of a single
VIR_DEBUG() in each case) immediately followed by the success: label
anyway, so this patch replaces the duplicated ++/-- instructions with
a single ++/-- inside a qualifying "if (dev)" down below success:.
(NB: if dev != NULL, by definition we are using a device (either pci
or netdev, doesn't matter for these purposes) from the network's pool)

The VIR_DEBUG args (which will be replaced in a followup patch anyway)
were all adjusted to account for the connection count being out of
date at the time.
---
 src/network/bridge_driver.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c19696c..dcd38ed 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1,7 +1,7 @@
 /*
  * bridge_driver.c: core driver methods for managing network
  *
- * Copyright (C) 2006-2015 Red Hat, Inc.
+ * Copyright (C) 2006-2016 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -4389,10 +4389,8 @@ networkNotifyActualDevice(virDomainDefPtr dom,
 goto error;
 }
 
-/* we are now assured of success, so mark the allocation */
-dev->connections++;
 VIR_DEBUG("Using physical device %s, connections %d",
-  dev->device.dev, dev->connections);
+  dev->device.dev, dev->connections + 1);
 
 }  else /* if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) */ {
 virDomainHostdevDefPtr hostdev;
@@ -,8 +4442,6 @@ networkNotifyActualDevice(virDomainDefPtr dom,
 goto error;
 }
 
-/* we are now assured of success, so mark the allocation */
-dev->connections++;
 VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d",
   dev->device.pci.domain, dev->device.pci.bus,
   dev->device.pci.slot, dev->device.pci.function,
@@ -4454,6 +4450,8 @@ networkNotifyActualDevice(virDomainDefPtr dom,
 
  success:
 netdef->connections++;
+if (dev)
+dev->connections++;
 VIR_DEBUG("Using network %s, %d connections",
   netdef->name, netdef->connections);
 
@@ -4562,9 +4560,8 @@ networkReleaseActualDevice(virDomainDefPtr dom,
 goto error;
 }
 
-dev->connections--;
 VIR_DEBUG("Releasing physical device %s, connections %d",
-  dev->device.dev, dev->connections);
+  dev->device.dev, dev->connections - 1);
 
 } else /* if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) */ {
 virDomainHostdevDefPtr hostdev;
@@ -4598,16 +4595,18 @@ networkReleaseActualDevice(virDomainDefPtr dom,
 goto error;
 }
 
-dev->connections--;
 VIR_DEBUG("Releasing physical device %04x:%02x:%02x.%x, connections 
%d",
   dev->device.pci.domain, dev->device.pci.bus,
   dev->device.pci.slot, dev->device.pci.function,
-  dev->connections);
+  dev->connections - 1);
 }
 
  success:
 if (iface->data.network.actual) {
 netdef->connections--;
+if (dev)
+dev->connections--;
+
 VIR_DEBUG("Releasing network %s, %d connections",
   netdef->name, netdef->connections);
 
-- 
2.5.0

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

Re: [libvirt] [PATCH 2/3] util: Introduce API's for Polkit text authentication



On 02/11/2016 05:02 AM, Daniel P. Berrange wrote:
> On Wed, Feb 10, 2016 at 02:46:35PM -0500, John Ferlan wrote:
>> Introduce virPolkitAgentCreate, virPolkitAgentCheck, and 
>> virPolkitAgentDestroy
>>
>> virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
>> command in order to handle the local agent authentication via stdin/stdout.
>>
>> virPolkitAgentCheck will run the polkit pkcheck command against the async
>> command process in order to perform the authentication
> 
> Err, we already have virPolkitCheckAuth which does this via the DBus
> API. Using pkcheck is a security flaw in many versions of Linux because
> it didn't accept the full set of args required for race-free auth
> checking.
> 

oh - right.  duh.

>> virPolkitAgentDestroy will close the command effectively reaping our
>> child process
>>
>> Needed to move around or add the "#include vircommand.h" since,
>> virpolkit.h now uses it.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/libvirt_private.syms |  3 ++
>>  src/util/virpolkit.c | 96 
>> +++-
>>  src/util/virpolkit.h |  7 
>>  tests/virpolkittest.c|  3 +-
>>  4 files changed, 107 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 5ae3618..e4d791d 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2028,6 +2028,9 @@ virPidFileWritePath;
>>  
>>  
>>  # util/virpolkit.h
>> +virPolkitAgentCheck;
>> +virPolkitAgentCreate;
>> +virPolkitAgentDestroy;
>>  virPolkitCheckAuth;
>>  
>>  
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index 56b1c31..e7c8603 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -26,8 +26,8 @@
>>  # include 
>>  #endif
>>  
>> -#include "virpolkit.h"
>>  #include "vircommand.h"
>> +#include "virpolkit.h"
>>  #include "virerror.h"
>>  #include "virlog.h"
>>  #include "virstring.h"
>> @@ -136,6 +136,77 @@ int virPolkitCheckAuth(const char *actionid,
>>  }
>>  
>>  
>> +/* virPolkitAgentDestroy:
>> + * @cmd: Pointer to the virCommandPtr created during virPolkitAgentCreate
>> + *
>> + * Destroy resources used by Polkit Agent
>> + */
>> +void
>> +virPolkitAgentDestroy(virCommandPtr cmd)
>> +{
>> +virCommandFree(cmd);
>> +}
>> +
>> +/* virPolkitAgentCreate:
>> + *
>> + * Allocate and setup a polkit agent
>> + *
>> + * Returns a virCommandPtr on success and NULL on failure
>> + */
>> +virCommandPtr
>> +virPolkitAgentCreate(void)
>> +{
>> +virCommandPtr cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
>> +int outfd = STDOUT_FILENO;
>> +int errfd = STDERR_FILENO;
>> +
>> +virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
>> +virCommandAddArg(cmd, "--fallback");
>> +virCommandSetInputFD(cmd, STDIN_FILENO);
>> +virCommandSetOutputFD(cmd, );
>> +virCommandSetErrorFD(cmd, );
>> +if (virCommandRunAsync(cmd, NULL) < 0)
>> +goto error;
>> +/* Give it a chance to get started */
>> +sleep(1);
> 
> Sigh, needing a sleep(1) is a sure sign that we'd be better off
> doing this via the Polkit API and not spawning programs. This
> sleep is just asking for bug reports about race conditions.
> 

So after more than a few cycles spent trying to figure out the build
system - I came back to this code...

I found that by changing this to a usleep(10 * 1000); also did the trick
(although 1 * 1000 fails).

Of course if I change the virsh code to utilize a retry loop that checks
on "no agent is available to authenticate", then the sleep isn't
necessary, although it could cut down on the retries...

I'll send out something shortly.

John

>> +
>> +return cmd;
>> +
>> + error:
>> +virCommandFree(cmd);
>> +return NULL;
>> +}
> 
> Regards,
> Daniel
> 

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

[libvirt] [PATCH v2 0/3] Add capability for text based polkit authentication for virsh

v1: http://www.redhat.com/archives/libvir-list/2016-February/msg00545.html

Differences to v1:

 - Patch 1, adjust error message slightly

 - Patch 2, remove the unnecessary virPolkitAgentCheck and the sleep

 - Patch 3, remove the --pkauth parameter
adjust the logic to attempt the authentications in a loop 
  with the first being one for the graphical agent. If that
  fails because there was no agent to be found, then start
  up the text based agent. Also add logic to wait for the
  text based authentication agent to start (just in case).

John Ferlan (3):
  polkit: Adjust message when action-id isn't found
  util: Introduce API's for Polkit text authentication
  virsh: Add support for text based polkit authentication

 src/libvirt_private.syms |  2 ++
 src/util/virpolkit.c | 64 +---
 src/util/virpolkit.h |  5 
 tests/virpolkittest.c|  3 ++-
 tools/virsh.c| 49 
 tools/virsh.h|  2 ++
 6 files changed, 115 insertions(+), 10 deletions(-)

-- 
2.5.0

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

[libvirt] [PATCH v2 2/3] util: Introduce API's for Polkit text authentication

Introduce virPolkitAgentCreate and virPolkitAgentDestroy

virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
command in order to handle the local agent authentication via stdin/stdout.

virPolkitAgentDestroy will close the command effectively reaping our
child process

Needed to move around or add the "#include vircommand.h" since,
virpolkit.h now uses it.

Signed-off-by: John Ferlan 
---
 src/libvirt_private.syms |  2 ++
 src/util/virpolkit.c | 56 +++-
 src/util/virpolkit.h |  5 +
 tests/virpolkittest.c|  3 ++-
 4 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 4cfaed5..8f2358f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2029,6 +2029,8 @@ virPidFileWritePath;
 
 
 # util/virpolkit.h
+virPolkitAgentCreate;
+virPolkitAgentDestroy;
 virPolkitCheckAuth;
 
 
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index d837a14..48d214a 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -26,8 +26,8 @@
 # include 
 #endif
 
-#include "virpolkit.h"
 #include "vircommand.h"
+#include "virpolkit.h"
 #include "virerror.h"
 #include "virlog.h"
 #include "virstring.h"
@@ -136,6 +136,46 @@ int virPolkitCheckAuth(const char *actionid,
 }
 
 
+/* virPolkitAgentDestroy:
+ * @cmd: Pointer to the virCommandPtr created during virPolkitAgentCreate
+ *
+ * Destroy resources used by Polkit Agent
+ */
+void
+virPolkitAgentDestroy(virCommandPtr cmd)
+{
+virCommandFree(cmd);
+}
+
+/* virPolkitAgentCreate:
+ *
+ * Allocate and setup a polkit agent
+ *
+ * Returns a virCommandPtr on success and NULL on failure
+ */
+virCommandPtr
+virPolkitAgentCreate(void)
+{
+virCommandPtr cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
+int outfd = STDOUT_FILENO;
+int errfd = STDERR_FILENO;
+
+virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
+virCommandAddArg(cmd, "--fallback");
+virCommandSetInputFD(cmd, STDIN_FILENO);
+virCommandSetOutputFD(cmd, );
+virCommandSetErrorFD(cmd, );
+if (virCommandRunAsync(cmd, NULL) < 0)
+goto error;
+
+return cmd;
+
+ error:
+virCommandFree(cmd);
+return NULL;
+}
+
+
 #elif WITH_POLKIT0
 int virPolkitCheckAuth(const char *actionid,
pid_t pid,
@@ -254,4 +294,18 @@ int virPolkitCheckAuth(const char *actionid 
ATTRIBUTE_UNUSED,
 }
 
 
+void
+virPolkitAgentDestroy(virCommandPtr cmd ATTRIBUTE_UNUSED)
+{
+return; /* do nothing */
+}
+
+
+virCommandPtr
+virPolkitAgentCreate(void)
+{
+virReportError(VIR_ERR_AUTH_FAILED, "%s",
+   _("polkit text authentication agent unavailable"));
+return NULL;
+}
 #endif /* WITH_POLKIT1 */
diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h
index 36122d0..f0aea37 100644
--- a/src/util/virpolkit.h
+++ b/src/util/virpolkit.h
@@ -24,6 +24,8 @@
 
 # include "internal.h"
 
+# define PKTTYAGENT "/usr/bin/pkttyagent"
+
 int virPolkitCheckAuth(const char *actionid,
pid_t pid,
unsigned long long startTime,
@@ -31,4 +33,7 @@ int virPolkitCheckAuth(const char *actionid,
const char **details,
bool allowInteraction);
 
+void virPolkitAgentDestroy(virCommandPtr cmd);
+virCommandPtr virPolkitAgentCreate(void);
+
 #endif /* __VIR_POLKIT_H__ */
diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
index b39beed..3ccb779 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013, 2014 Red Hat, Inc.
+ * Copyright (C) 2013, 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
@@ -27,6 +27,7 @@
 # include 
 # include 
 
+# include "vircommand.h"
 # include "virpolkit.h"
 # include "virdbus.h"
 # include "virlog.h"
-- 
2.5.0

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

[libvirt] [PATCH v2 1/3] polkit: Adjust message when action-id isn't found

When there isn't a ssh -X type session running and a user has not
been added to the libvirt group, attempts to run 'virsh -c qemu:///system'
commands from an otherwise unprivileged user will fail with rather
generic or opaque error message:

"error: authentication failed: no agent is available to authenticate"

This patch will adjust the error message to help reflect not only
that it was a polkit agent, but which 'action-id' was not found. It
does so carefully, by adding to the text rather than adjusting it
since virpolkittest is looking for a specific string. The result on
a failure then becomes:

"error: authentication failed: no agent is available to authenticate -
 no polkit agent for action 'org.libvirt.unix.manage'"

A bit more history on this - at one time a failure generated the
following type message when running the 'pkcheck' as a subprocess:

"error: authentication failed: polkit\56retains_authorization_after_challenge=1
Authorization requires authentication but no agent is available."

but, a patch was generated to adjust the error message to help provide
more details about what failed. This was pushed as commit id '96a108c99'.
That patch prepended a "polkit: " to the output. It really didn't solve
the problem, but gave a hint.

After some time it was deemed using DBus API calls directly was a
better way to go (since pkcheck calls them anyway). So, commit id
'1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit
and adjusted it. Then commit id 'c7542573' adjusted the remote.c
code to call the new API (virPolkitCheckAuth). Finally, commit id
'308c0c5a' altered the code to call DBus APIs directly. In doing
so, it reverted the failing error message to the generic message
that would have been received from DBus anyway.

Signed-off-by: John Ferlan 
---
 src/util/virpolkit.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 8da91f2..d837a14 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -1,7 +1,7 @@
 /*
  * virpolkit.c: helpers for using polkit APIs
  *
- * Copyright (C) 2013, 2014 Red Hat, Inc.
+ * Copyright (C) 2013, 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
@@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid,
 virReportError(VIR_ERR_AUTH_CANCELLED, "%s",
_("user cancelled authentication process"));
 else if (is_challenge)
-virReportError(VIR_ERR_AUTH_FAILED, "%s",
-   _("no agent is available to authenticate"));
+virReportError(VIR_ERR_AUTH_FAILED,
+   _("no agent is available to authenticate - "
+ "no polkit agent for action '%s'"),
+   actionid);
 else
 virReportError(VIR_ERR_AUTH_FAILED, "%s",
_("access denied by policy"));
-- 
2.5.0

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

[libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication

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

When the login session doesn't have an ssh -X type display agent in
order for libvirtd to run the polkit session authentication, attempts
to run 'virsh -c qemu:///system list' from an unauthorized user (or one
that isn't part of the libvirt /etc/group) will fail with the following
error from libvirtd:

error: authentication failed: no agent is available to authenticate -
   no polkit agent for action 'org.libvirt.unix.manage'

In order to handle the local authentication, we will use the new
virPolkitAgentCreate API in order to create a text based authentication
agent for our non readonly session to authenticate with.

The new code will execute in a loop allowing 5 failures to authenticate
before failing out. It also has a check for a failure to start the text
polkit authentication agent as testing as shown the agent startup isn't
necessarily immediate.

With this patch in place, the following occurs:

$ virsh -c qemu:///system list
 AUTHENTICATING FOR org.libvirt.unix.manage ===
System policy prevents management of local virtualized systems
Authenticating as: Some User (SUser)
Password:
 AUTHENTICATION COMPLETE ===
 IdName   State
 
  1 somedomain running

$

Signed-off-by: John Ferlan 
---
 tools/virsh.c | 49 -
 tools/virsh.h |  2 ++
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index b96dbda..c9da9f1 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -143,6 +143,9 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
 int interval = 5; /* Default */
 int count = 6;/* Default */
 bool keepalive_forced = false;
+virCommandPtr pkagent = NULL;
+int authfail = 0;
+int agentstart = 0;
 
 if (ctl->keepalive_interval >= 0) {
 interval = ctl->keepalive_interval;
@@ -153,10 +156,43 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
 keepalive_forced = true;
 }
 
-c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
-   readonly ? VIR_CONNECT_RO : 0);
-if (!c)
-return NULL;
+do {
+virErrorPtr err;
+
+if ((c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
+readonly ? VIR_CONNECT_RO : 0)))
+break;
+
+if (readonly)
+goto cleanup;
+
+err = virGetLastError();
+if (err && strstr(err->message,
+  _("no agent is available to authenticate"))) {
+if (!pkagent) {
+if (!(pkagent = virPolkitAgentCreate()))
+goto cleanup;
+}
+agentstart++;
+} else if (err && strstr(err->message, _("authentication failed:"))) {
+authfail++;
+} else {
+goto cleanup;
+}
+virResetLastError();
+/* If we fail to authenticate 5 times or we fail to successfully
+ * connect with the agent after 10 attempts, then fail out.
+ * No sense prolonging the agony
+ */
+} while (authfail < 5 && agentstart < 10);
+
+if (!c) {
+/* If we failed because we couldn't start the agent, give a reason */
+if (agentstart == 10)
+vshError(ctl, "%s",
+ _("Cannot setup a polkit text authentication agent"));
+goto cleanup;
+}
 
 if (interval > 0 &&
 virConnectSetKeepAlive(c, interval, count) != 0) {
@@ -165,12 +201,15 @@ virshConnect(vshControl *ctl, const char *uri, bool 
readonly)
  _("Cannot setup keepalive on connection "
"as requested, disconnecting"));
 virConnectClose(c);
-return NULL;
+c = NULL;
+goto cleanup;
 }
 vshDebug(ctl, VSH_ERR_INFO, "%s",
  _("Failed to setup keepalive on connection\n"));
 }
 
+ cleanup:
+virPolkitAgentDestroy(pkagent);
 return c;
 }
 
diff --git a/tools/virsh.h b/tools/virsh.h
index 8b5e5ba..a6c7289 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -36,6 +36,8 @@
 # include "internal.h"
 # include "virerror.h"
 # include "virthread.h"
+# include "vircommand.h"
+# include "virpolkit.h"
 # include "vsh.h"
 
 # define VIRSH_PROMPT_RW"virsh # "
-- 
2.5.0

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

[libvirt] [PATCH 2/3] network: consolidated info log for all network allocate/free operations

There are three functions that deal with allocating and freeing
devices from a networks netdev/pci device pool:
network(Allocate|Notify|Release)ActualDevice(). These functions also
maintain a counter of the number of domains currently using a network
(regardless of whether or not that network uses a device pool). Each
of these functions had multiple log messages (output using VIR_DEBUG)
that were in slightly different formats and gave varying amounts of
information.

This patch creates a single function to log the pertinent information
in a consistent manner for all three of these functions. Along with
assuring that all the functions produce a consistent form of output
(and making it simpler to change), it adds the MAC address of the
domain interface involved in the operation, making it possible to
verify which interface of which domain the operation is being done for
(assuming that all MAC addresses are unique, of course).

All of these messages are raised from DEBUG to INFO, since they don't
happen that often (once per interface per domain/libvirtd start or
domain stop), and can be very informative and helpful - eliminating
the need to log debug level messages makes it much easier to sort
these out.
---
 src/network/bridge_driver.c | 80 ++---
 1 file changed, 39 insertions(+), 41 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index dcd38ed..c305f8e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3863,6 +3863,40 @@ int networkRegister(void)
  * driver, we will need to present these functions via a second
  * "backend" function table.
  */
+static void
+networkLogAllocation(virNetworkDefPtr netdef,
+ virDomainNetType actualType,
+ virNetworkForwardIfDefPtr dev,
+ virDomainNetDefPtr iface,
+ bool inUse)
+{
+char macStr[VIR_MAC_STRING_BUFLEN];
+const char *verb = inUse ? "using" : "releasing";
+
+if (!dev) {
+VIR_INFO("MAC %s %s network %s (%d connections)",
+ virMacAddrFormat(>mac, macStr), verb,
+ netdef->name, netdef->connections);
+} else {
+/* mark the allocation */
+dev->connections++;
+if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+VIR_INFO("MAC %s %s network %s (%d connections) "
+ "physical device %04x:%02x:%02x.%x (%d connections)",
+ virMacAddrFormat(>mac, macStr), verb,
+ netdef->name, netdef->connections,
+ dev->device.pci.domain, dev->device.pci.bus,
+ dev->device.pci.slot, dev->device.pci.function,
+ dev->connections);
+} else {
+VIR_INFO("MAC %s %s network %s (%d connections) "
+ "physical device %s (%d connections)",
+ virMacAddrFormat(>mac, macStr), verb,
+ netdef->name, netdef->connections,
+ dev->device.dev, dev->connections);
+}
+}
+}
 
 /* networkAllocateActualDevice:
  * @dom: domain definition that @iface belongs to
@@ -4236,23 +4270,8 @@ networkAllocateActualDevice(virDomainDefPtr dom,
 
 if (netdef) {
 netdef->connections++;
-VIR_DEBUG("Using network %s, %d connections",
-  netdef->name, netdef->connections);
-
-if (dev) {
-/* mark the allocation */
+if (dev)
 dev->connections++;
-if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
-VIR_DEBUG("Using physical device %s, %d connections",
-  dev->device.dev, dev->connections);
-} else {
-VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, 
connections %d",
-  dev->device.pci.domain, dev->device.pci.bus,
-  dev->device.pci.slot, dev->device.pci.function,
-  dev->connections);
-}
-}
-
 /* finally we can call the 'plugged' hook script if any */
 if (networkRunHook(network, dom, iface,
VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
@@ -4263,6 +4282,7 @@ networkAllocateActualDevice(virDomainDefPtr dom,
 dev->connections--;
 goto error;
 }
+networkLogAllocation(netdef, actualType, dev, iface, true);
 }
 
 ret = 0;
@@ -4388,10 +4408,6 @@ networkNotifyActualDevice(virDomainDefPtr dom,
netdef->name, actualDev);
 goto error;
 }
-
-VIR_DEBUG("Using physical device %s, connections %d",
-  dev->device.dev, dev->connections + 1);
-
 }  else /* if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) */ {
 virDomainHostdevDefPtr hostdev;
 
@@ -4441,20 +4457,12 @@ networkNotifyActualDevice(virDomainDefPtr dom,

[libvirt] [PATCH 3/3] util: clean up and expand 802.1QbX negotiation logging

The existing log messages for this have several problems; there are
two lines of log when one will suffice, they duplicate the function
name in log message (when it's already included by VIR_DEBUG), they're
missing some useful bits, they get logged even when the call is a NOP.

This patch cleans up the problems with those existing logs, and also
adds a new VIR_INFO-level log down at the function that is actually
creating and sending the netlink message that logs *everything* going
into the netlink message (which turns out to be much more useful in
practice for me; I didn't want to eliminate the logs at the existing
location though, in case they are useful in some scenario I'm
unfamiliar with; anyway those logs are remaining at debug level, so it
shouldn't be a bother to anyone).
---
 src/util/virnetdevvportprofile.c | 60 ++--
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index 8a6b601..8100656 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -653,6 +653,40 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int 
ifindex,
 uint32_t dst_pid = 0;
 struct nl_msg *nl_msg;
 struct nlattr *vfports = NULL, *vfport;
+char macStr[VIR_MAC_STRING_BUFLEN];
+char hostUUIDStr[VIR_UUID_STRING_BUFLEN];
+char instanceUUIDStr[VIR_UUID_STRING_BUFLEN];
+const char *opName;
+
+switch (op) {
+case PORT_REQUEST_PREASSOCIATE:
+opName = "PREASSOCIATE";
+break;
+case PORT_REQUEST_PREASSOCIATE_RR:
+opName = "PREASSOCIATE_RR";
+break;
+case PORT_REQUEST_ASSOCIATE:
+opName = "ASSOCIATE";
+break;
+case PORT_REQUEST_DISASSOCIATE:
+opName = "DISASSOCIATE";
+break;
+default:
+opName = "(unknown)";
+break;
+}
+VIR_INFO("%s: ifname: %s ifindex: %d vf: %d vlanid: %d mac: %s "
+ "profileId: %s instanceId: %s hostUUID: %s",
+ opName, ifname ? ifname : "(unspecified)",
+ ifindex, vf, vlanid,
+ macaddr ? virMacAddrFormat(macaddr, macStr) : "(unspecified)",
+ profileId ? profileId : "(unspecified)",
+ (instanceId
+  ? virUUIDFormat(instanceId, instanceUUIDStr)
+  : "(unspecified)"),
+ (hostUUID
+  ? virUUIDFormat(hostUUID, hostUUIDStr)
+  : "(unspecified)"));
 
 nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST);
 if (!nl_msg) {
@@ -1197,11 +1231,16 @@ virNetDevVPortProfileAssociate(const char 
*macvtap_ifname,
bool setlink_only)
 {
 int rc = 0;
+char uuidStr[VIR_UUID_STRING_BUFLEN];
+char macStr[VIR_MAC_STRING_BUFLEN];
 
-VIR_DEBUG("Associating port profile '%p' on link device '%s'",
-  virtPort, (macvtap_ifname ? macvtap_ifname : linkdev));
-
-VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, 
virNetDevVPortProfileOpTypeToString(vmOp));
+VIR_DEBUG("profile:'%p' vmOp: %s device: %s@%s mac: %s uuid: %s",
+  virtPort, virNetDevVPortProfileOpTypeToString(vmOp),
+  (macvtap_ifname ? macvtap_ifname : ""), linkdev,
+  (macvtap_macaddr
+   ? virMacAddrFormat(macvtap_macaddr, macStr)
+   : "(unspecified)"),
+  vmuuid ? virUUIDFormat(vmuuid, uuidStr) : "(unspecified)");
 
 if (!virtPort || vmOp == VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)
 return 0;
@@ -1259,11 +1298,14 @@ virNetDevVPortProfileDisassociate(const char 
*macvtap_ifname,
   virNetDevVPortProfileOp vmOp)
 {
 int rc = 0;
-
-VIR_DEBUG("Disassociating port profile id '%p' on link device '%s' ",
-  virtPort, macvtap_ifname);
-
-VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, 
virNetDevVPortProfileOpTypeToString(vmOp));
+char macStr[VIR_MAC_STRING_BUFLEN];
+
+VIR_DEBUG("profile:'%p' vmOp: %s device: %s@%s mac: %s",
+  virtPort, virNetDevVPortProfileOpTypeToString(vmOp),
+  (macvtap_ifname ? macvtap_ifname : ""), linkdev,
+  (macvtap_macaddr
+   ? virMacAddrFormat(macvtap_macaddr, macStr)
+   : "(unspecified)"));
 
 if (!virtPort)
return 0;
-- 
2.5.0

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

[libvirt] [PATCH 0/3] networking logging improvements

1/3 was originally part of 2/3, but I decided I should split it out
since it changes something other than logging (although it should
still be a NOP). 2/3 changes what's logged when a domain interface is
connected to / disconnected from a network, and 3/3 changes what is
logged during 802.1QbX associate/disassociate operations.

All of these logging changed have proven useful to me when debugging
802.1QbX problems on a downstream version of libvirt (0.10.2 in
RHEL6), so I thought I'd share the joy :-)

Laine Stump (3):
  network: consolidate connection count updates for device pool
  network: consolidated info log for all network allocate/free
operations
  util: clean up and expand 802.1QbX negotiation logging

 src/network/bridge_driver.c  | 91 +++-
 src/util/virnetdevvportprofile.c | 60 ++
 2 files changed, 95 insertions(+), 56 deletions(-)

-- 
2.5.0

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

Re: [libvirt] [PATCH 0/3] Add capability for text based polkit authentication for virsh



On 02/11/2016 12:45 PM, Daniel P. Berrange wrote:
> On Thu, Feb 11, 2016 at 12:22:12PM -0500, John Ferlan wrote:
>>
>>
>> On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
>>> On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=872166

 As an alternative to commit id 'e94979e90' which allows polkit
 authentication by adding users to the 'libvirt' group, add the
 ability to start and utilize a text based authentication agent
 for virsh.

 At the very least patch 1 will suffice part of the issue listed
 in the bz - the opaque error message related to "some agent".

 For patch 2, it was far easier to utilize what polkit provides
 in pkttyagent and pkcheck utilities, than adding some code which
 requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being
 #defined for compilation.
>>>
>>> Sigh, that define is a bit of a bad joke really. polkit was first
>>> added in Fedora 12, and comparing the header files between then
>>> and now, they've never broken their ABI. They're merely added new
>>> APIs.  IMHO, we can just define that, and use the API from libvirt
>>> without trouble.
>>>
>>
>> I had code generated that tried to use those API's, but couldn't find
>> the correct magic incantation to convince the build to find the
>> polkitagent/polkitagent.h file.
>>
>> #define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE
>> #include 
>>
>> ...
>> util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such
>> file or directory
>> ...
>>
>>
>> /usr/include/polkit-1/polkitagent/polkitagent.h
>>
>>
>> That is, how do I ensure that somehow automagically add that
>> -I/usr/include/polkit-1 ?
>>
>>
>> I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and
>> POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck.
> 
> If you show your complete patch for this, I can take  a look  and
> see what's missing
> 

There's really not much to show...

I took src/util/polkit.c and added:

#define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE
#include 


For configure.ac, I tried adding :

POLKIT_AGENT_CFLAGS=
POLKIT_AGENT_LIBS=

Under the existing

POLKIT_CFLAGS=
POLKIT_LIBS=

and

AC_SUBST([POLKIT_AGENT_CFLAGS])
AC_SUBST([POLKIT_AGENT_LIBS])

after

 AC_SUBST([POLKIT_CFLAGS])
 AC_SUBST([POLKIT_LIBS])


Then in src/Makefile.am

I added $(POLKIT_AGENT_CFLAGS) after $(POLKIT_CFLAGS) and
$(POLKIT_AGENT_LIBS) after $(POLKIT_LIBS)


I usually try to avoid the configure.ac and Makefile.am - it's all black
magic to me.  Happy when it works...

John






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

Re: [libvirt] [PATCH] Revert "tests: Don't link mock libraries against libvirt and gnulib"

On Thu, 2016-02-11 at 09:49 +, Daniel P. Berrange wrote:
> On Thu, Feb 11, 2016 at 09:54:45AM +0100, Andrea Bolognani wrote:
> > On Wed, 2016-02-10 at 18:09 +, Daniel P. Berrange wrote:
> > > On Wed, Feb 10, 2016 at 06:33:21PM +0100, Andrea Bolognani wrote:
> > > > This reverts commit 6aa90452aa63cb1e1ffa84ff5f93f5873bf810a0.
> > > >  
> > > > Turns out that not linking against libvirt and gnulib is okay for
> > > > regular Linux (and FreeBSD) builds, but makes mingw very unhappy.
> > > >  
> > > >    .../virnetserverclientmock_la-virnetserverclientmock.o:
> > > >  In function `virNetSocketGetSELinuxContext':
> > > >  .../virnetserverclientmock.c:61: undefined reference to 
> > > > `rpl_strdup'
> > > >    .../libvirportallocatormock_la-virportallocatortest.o:
> > > >  In function `init_syms':
> > > >  .../virportallocatortest.c:61: undefined reference to 
> > > > `virFileClose'
> > > > ---
> > > > Pushed as build breaker.
> > > >  
> > > > Bunny ears of shame will be deployed in the morning.
> > >  
> > > You should still be able to drop linkage of libvirt.la, just keep
> > > the gnulib bits.  The gnulib addition is harmless, because it is
> > > a static library the linker will drop all functions which are not
> > > actually used by the mock.o, so it will end up being a no-op on
> > > Linux, and will only pull in rpl_strup on mingw32. We should never
> > > link libvirt.la to mock libs though.
> > 
> > Looks like it is complaining about virFileClose(), plus
> > virFilePrint() in the full output, as well. So maybe linking against
> > libvirt.la is needed after all?
> 
> That's a bug in virportallocatortest.c that was introduced in
> 0ee90812. The init_syms() fnuction should *not* be calling
> VIR_FORCE_CLOSE - it must use close(), because this is the
> mock-override and so should not be using libvirt.so symbols.

Okay, I will change it to use plain close().

> > If there's no harm in that, how about defining
> > 
> >   MOCKLIBS_LDADD = \
> > $(GNULIB_LIBS) \
> > ../src/libvirt.la
> > 
> > and using it for every mock library, like the existing
> > $(MOCKLIB_LDFLAGS)?
> 
> Nope as above, we should not be linking libvirt.la to the mock lib,
> because the mock lib is supposed to be replacing libvirt.la symbols

I will define $(MOCKLIBS_LDADD) to contain just $(GNULIB_LIBS)
then, and make sure mock libraries link against it.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [RFC] vhost-user + shared memory + NUMA

2016-02-11 Thread Pavel Fedin

 Hello!

> Historically QEMU had a pointless check on the path passed in, to enforce
> that it was only hugetlbfs, so could not just pass in a regular tmpfs
> file. I think we removed that in QEMU 2.5. I think it is a valid enhance
>  to allow specification of "shared" memory backing which
> would be mapping to a regular tmpfs.
> 
> I don't think we should magically do anything based on existance of
> vhost-user though - changes in way the guest memory is allocated should
> always require explicit user configuration.

 Ok, then would it be a good compromise if we require , and only 
implicitly add "shared" if we have vhost-user
devices? This way we would not change the way the guest memory is allocated.
 IMHO being able to manually specify "shared" both in  and in 
 would be ambiguous.

Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia


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

Re: [libvirt] [PATCH 0/3] Add capability for text based polkit authentication for virsh

On Thu, Feb 11, 2016 at 12:22:12PM -0500, John Ferlan wrote:
> 
> 
> On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
> > On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=872166
> >>
> >> As an alternative to commit id 'e94979e90' which allows polkit
> >> authentication by adding users to the 'libvirt' group, add the
> >> ability to start and utilize a text based authentication agent
> >> for virsh.
> >>
> >> At the very least patch 1 will suffice part of the issue listed
> >> in the bz - the opaque error message related to "some agent".
> >>
> >> For patch 2, it was far easier to utilize what polkit provides
> >> in pkttyagent and pkcheck utilities, than adding some code which
> >> requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being
> >> #defined for compilation.
> > 
> > Sigh, that define is a bit of a bad joke really. polkit was first
> > added in Fedora 12, and comparing the header files between then
> > and now, they've never broken their ABI. They're merely added new
> > APIs.  IMHO, we can just define that, and use the API from libvirt
> > without trouble.
> > 
> 
> I had code generated that tried to use those API's, but couldn't find
> the correct magic incantation to convince the build to find the
> polkitagent/polkitagent.h file.
> 
> #define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE
> #include 
> 
> ...
> util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such
> file or directory
> ...
> 
> 
> /usr/include/polkit-1/polkitagent/polkitagent.h
> 
> 
> That is, how do I ensure that somehow automagically add that
> -I/usr/include/polkit-1 ?
> 
> 
> I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and
> POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck.

If you show your complete patch for this, I can take  a look  and
see what's missing

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 0/3] Add capability for text based polkit authentication for virsh



On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
> On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=872166
>>
>> As an alternative to commit id 'e94979e90' which allows polkit
>> authentication by adding users to the 'libvirt' group, add the
>> ability to start and utilize a text based authentication agent
>> for virsh.
>>
>> At the very least patch 1 will suffice part of the issue listed
>> in the bz - the opaque error message related to "some agent".
>>
>> For patch 2, it was far easier to utilize what polkit provides
>> in pkttyagent and pkcheck utilities, than adding some code which
>> requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being
>> #defined for compilation.
> 
> Sigh, that define is a bit of a bad joke really. polkit was first
> added in Fedora 12, and comparing the header files between then
> and now, they've never broken their ABI. They're merely added new
> APIs.  IMHO, we can just define that, and use the API from libvirt
> without trouble.
> 

I had code generated that tried to use those API's, but couldn't find
the correct magic incantation to convince the build to find the
polkitagent/polkitagent.h file.

#define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE
#include 

...
util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such
file or directory
...


/usr/include/polkit-1/polkitagent/polkitagent.h


That is, how do I ensure that somehow automagically add that
-I/usr/include/polkit-1 ?


I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and
POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck.

Tks -

John
>>
>> I chose 'pkauth' to mean polkit authentication - figured it was
>> a workable shorthand, but if there's better suggestions those
>> can be considered.
> 
> Regards,
> Daniel
> 

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

Re: [libvirt] [PATCH 1/4] tests: Split off the mock part of the port allocator test

On Thu, 2016-02-11 at 15:45 +0100, Ján Tomko wrote:
> On Thu, Feb 11, 2016 at 02:36:28PM +0100, Andrea Bolognani wrote:
> > Instead of compiling either the mock or the non-mock part of the
> > file based on a compiler flag, split the mock part off to its
> > own file.
> > ---
> >  tests/Makefile.am|   4 +-
> >  tests/virportallocatormock.c | 108 
> >++
> >  tests/virportallocatortest.c | 110 
> >+++
> >  3 files changed, 117 insertions(+), 105 deletions(-)
> >  create mode 100644 tests/virportallocatormock.c
> 
> > @@ -255,12 +168,3 @@ mymain(void)
> >  }
> >  
> >  VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir 
> >"/.libs/libvirportallocatormock.so")
> > -# endif
> > -
> > -#else /* ! defined(RTLD_NEXT) */
> > -int
> > -main(void)
> > -{
> > -return EXIT_AM_SKIP;
> > -}
> > -#endif
> 
> I'm afraid removing this will break the test on non-Linux.
> 
> ACK if you leave the EXIT_AM_SKIP in.

I'm reasonably confident it wouldn't make any difference, because
we don't check for the availability of that symbol anywhere else
in the code, not even in the dozen or so test cases that actually
use dynamic symbol loading. We even include  without
checking whether HAVE_DLFCN_H is defined in some cases.

That said, better safe than sorry, so I've restored the check and
pushed the whole series. Thanks for your review :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 0/2] tests: Fix mock libraries linking