Re: [libvirt] Usability Enhancement: Import/Export VMs GUI

2016-03-09 Thread Martin Kletzander 

On Fri, Mar 04, 2016 at 04:23:18PM +, ban...@openmailbox.org wrote:

The single most important usability feature missed by our less technical
users who migrate  from VirtualBox is a one click import/export of VMs
and their config settings.

I was optimistic about the Gnome Boxes effort on Govf lib but
unfortunately it was never realized and I would hesitate to recommend it
because libvirt/virt-manager has the security advantages of sVirt.

An ideal solution would work across all KVM frontends.



This is a great idea, but I don't know about such one-click solution.
This could be suitable for virt-manager and libguestfs projects (Cc'd
both projects).  If nobody grabs it right now, it could be at least
suggested as GSoC and/or Outreachy project idea.  Or were you looking
forward to adding such solution?

Have a nice day,
Martin


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

[libvirt] [PATCH 22/21] virt-admin: Don't tell everyone needlessly we're connected

2016-03-09 Thread Martin Kletzander 
There are cases when we don't want to tell the user we are connected.
That's for example when we first connect to the server without the
command 'connect' itself.  That helps to clear out output of first
command, mainly when running non-interactively.

Signed-off-by: Martin Kletzander 
---
 tools/virt-admin.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index bc9ae9366280..5ce35f8dd5d3 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -119,8 +119,6 @@ vshAdmConnect(vshControl *ctl, unsigned int flags)

 if (priv->wantReconnect)
 vshPrint(ctl, "%s\n", _("Reconnected to the admin server"));
-else
-vshPrint(ctl, "%s\n", _("Connected to the admin server"));
 }

 return 0;
@@ -288,6 +286,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
 {
 const char *name = NULL;
 vshAdmControlPtr priv = ctl->privData;
+bool connected = priv->conn;

 if (vshCommandOptStringReq(ctl, cmd, "name", ) < 0)
 return false;
@@ -296,6 +295,8 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
 ctl->connname = vshStrdup(ctl, name);

 vshAdmReconnect(ctl);
+if (!connected)
+vshPrint(ctl, "%s\n", _("Connected to the admin server"));

 return !!priv->conn;
 }
--
2.7.2

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

Re: [libvirt] [PATCH 0/4] docs: website: improve readability

2016-03-09 Thread Martin Kletzander 

On Wed, Mar 09, 2016 at 09:12:21PM -0500, Cole Robinson wrote:

Some css tweaks to improve test readability and be more consistent with
other sites (I was using en.wikipedia.org as a reference)

First 2 bits are cleanups, latter 2 bits are actual changes.

Front before: http://i.imgur.com/zvFqBa9.png
Front after:  http://i.imgur.com/gsHa5Pq.png
List before:  http://i.imgur.com/nh6hMsy.png
List after:   http://i.imgur.com/WxgE4y0.png
Text before:  http://i.imgur.com/8bvgEYg.png
Text after:   http://i.imgur.com/wg0hk4r.png



I like this series.  ACK from me, although you might decide yourself
whether such design change should have more decision-making people talk
about it ;)


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

[libvirt] [PATCH 15/21] gendispatch: Be able to generate multi-return values

2016-03-09 Thread Martin Kletzander 
Let's call it modern_ret_as_list as opposed to single_ret_as_list.  The
latter was able to return list of things.  However the new, more modern,
version came and it is used since listAllDomains till nowadays in
ListServers.

Signed-off-by: Martin Kletzander 
---
 src/rpc/gendispatch.pl | 200 ++---
 1 file changed, 172 insertions(+), 28 deletions(-)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 29f2c6ac033b..d6ce9b85e158 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -617,6 +617,9 @@ elsif ($mode eq "server") {
 my $single_ret_list_max_var = "undefined";
 my $single_ret_list_max_define = "undefined";
 my $multi_ret = 0;
+my $modern_ret_as_list = 0;
+my $modern_ret_struct_name = "undefined";
+my $single_ret_list_error_msg_type = "undefined";

 if ($rettype ne "void" and
 scalar(@{$call->{ret_members}}) > 1) {
@@ -633,7 +636,16 @@ elsif ($mode eq "server") {

 push(@ret_list, "memcpy(ret->$3, tmp.$3, 
sizeof(ret->$3));");
 } elsif ($ret_member =~ m/^(unsigned 
)?(char|short|int|hyper) (\S+);/) {
-push(@ret_list, "ret->$3 = tmp.$3;");
+if (!$modern_ret_as_list) {
+push(@ret_list, "ret->$3 = tmp.$3;");
+}
+} elsif ($ret_member =~ m/admin_nonnull_(server) 
(\S+)<(\S+)>;/) {
+$modern_ret_struct_name = $1;
+$single_ret_list_error_msg_type = $1;
+$single_ret_list_name = $2;
+$single_ret_list_max_define = $3;
+
+$modern_ret_as_list = 1;
 } else {
 die "unhandled type for multi-return-value: 
$ret_member";
 }
@@ -817,27 +829,43 @@ elsif ($mode eq "server") {

 # select struct type for multi-return-value functions
 if ($multi_ret) {
-if (!(defined $call->{ret_offset})) {
-die "multi-return-value without insert@ annotation: 
$call->{ret}";
-}
+if (defined $call->{ret_offset}) {
+push_privconn(\@args_list);

-push_privconn(\@args_list);
+if ($modern_ret_as_list) {
+my $struct_name = 
name_to_TypeName($modern_ret_struct_name);

-my $struct_name = $call->{ProcName};
-$struct_name =~ s/Get//;
+if ($structprefix eq "admin") {
+$struct_name = "Net${struct_name}";
+}

-splice(@args_list, $call->{ret_offset}, 0, (""));
+push(@vars_list, "vir${struct_name}Ptr *result = NULL");
+push(@vars_list, "int nresults = 0");

-if ($call->{ProcName} eq "DomainBlockStats" ||
-$call->{ProcName} eq "DomainInterfaceStats") {
-# SPECIAL: virDomainBlockStats and virDomainInterfaceStats
-#  have a 'Struct' suffix on the actual struct name
-#  and take the struct size as additional argument
-$struct_name .= "Struct";
-splice(@args_list, $call->{ret_offset} + 1, 0, 
("sizeof(tmp)"));
-}
+@args_list = grep {!/\bneed_results\b/} @args_list;

-push(@vars_list, "vir$struct_name tmp");
+splice(@args_list, $call->{ret_offset}, 0,
+   ("args->need_results ?  : NULL"));
+} else {
+my $struct_name = $call->{ProcName};
+$struct_name =~ s/Get//;
+
+splice(@args_list, $call->{ret_offset}, 0, (""));
+
+if ($call->{ProcName} eq "DomainBlockStats" ||
+$call->{ProcName} eq "DomainInterfaceStats") {
+# SPECIAL: virDomainBlockStats and 
virDomainInterfaceStats
+#  have a 'Struct' suffix on the actual struct 
name
+#  and take the struct size as additional 
argument
+$struct_name .= "Struct";
+splice(@args_list, $call->{ret_offset} + 1, 0, 
("sizeof(tmp)"));
+}
+
+push(@vars_list, "vir$struct_name tmp");
+}
+} else {
+die "multi-return-value without insert@ annotation: 
$call->{ret}";
+}
 }

 if ($call->{streamflag} ne "none") {
@@ -868,6 +896,10 @@ elsif ($mode eq "server") {
 print "{\n";
 print "int rv = -1;\n";

+if ($modern_ret_as_list) {
+print "size_t i;\n";
+}
+
 foreach my $var (@vars_list) {
 print "$var;\n";
 }
@@ -974,9

[libvirt] [PATCH 16/21] admin: Generate ConnectListServers dispatch helpers

2016-03-09 Thread Martin Kletzander 
Since we have the opportunity now, let's save some precious code lines.

Signed-off-by: Martin Kletzander 
---
 daemon/admin.c | 45 -
 daemon/admin_server.c  |  6 ++---
 daemon/admin_server.h  |  6 ++---
 po/POTFILES.in |  1 -
 src/admin/admin_protocol.x |  4 +--
 src/admin/admin_remote.c   | 63 --
 6 files changed, 8 insertions(+), 117 deletions(-)

diff --git a/daemon/admin.c b/daemon/admin.c
index cae2a84d84b8..3169cddb5807 100644
--- a/daemon/admin.c
+++ b/daemon/admin.c
@@ -133,49 +133,4 @@ adminConnectGetLibVersion(virNetDaemonPtr dmn 
ATTRIBUTE_UNUSED,
 return 0;
 }

-static int
-adminDispatchConnectListServers(virNetServerPtr server ATTRIBUTE_UNUSED,
-virNetServerClientPtr client,
-virNetMessagePtr msg ATTRIBUTE_UNUSED,
-virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
-admin_connect_list_servers_args *args,
-admin_connect_list_servers_ret *ret)
-{
-virNetServerPtr *servers = NULL;
-int nservers = 0;
-int rv = -1;
-size_t i;
-struct daemonAdmClientPrivate *priv =
-virNetServerClientGetPrivateData(client);
-
-if ((nservers =
-adminDaemonListServers(priv->dmn,
-   args->need_results ?  : NULL,
-   args->flags)) < 0)
-goto cleanup;
-
-if (servers && nservers) {
-if (VIR_ALLOC_N(ret->servers.servers_val, nservers) < 0)
-goto cleanup;
-
-ret->servers.servers_len = nservers;
-for (i = 0; i < nservers; i++)
-make_nonnull_server(ret->servers.servers_val + i, servers[i]);
-} else {
-ret->servers.servers_len = 0;
-ret->servers.servers_val = NULL;
-}
-
-ret->ret = nservers;
-rv = 0;
-
- cleanup:
-if (rv < 0)
-virNetMessageSaveError(rerr);
-if (servers && nservers > 0)
-for (i = 0; i < nservers; i++)
-virObjectUnref(servers[i]);
-VIR_FREE(servers);
-return rv;
-}
 #include "admin_dispatch.h"
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index a35f33130607..6eabbe4ae6d5 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -37,9 +37,9 @@
 VIR_LOG_INIT("daemon.admin_server");

 int
-adminDaemonListServers(virNetDaemonPtr dmn,
-   virNetServerPtr **servers,
-   unsigned int flags)
+adminConnectListServers(virNetDaemonPtr dmn,
+virNetServerPtr **servers,
+unsigned int flags)
 {
 int ret = -1;
 virNetServerPtr *srvs = NULL;
diff --git a/daemon/admin_server.h b/daemon/admin_server.h
index 606442c19f5f..b77653f6c384 100644
--- a/daemon/admin_server.h
+++ b/daemon/admin_server.h
@@ -26,8 +26,8 @@

 # include "rpc/virnetdaemon.h"

-int adminDaemonListServers(virNetDaemonPtr dmn,
-   virNetServerPtr **servers,
-   unsigned int flags);
+int adminConnectListServers(virNetDaemonPtr dmn,
+virNetServerPtr **servers,
+unsigned int flags);

 #endif /* __LIBVIRTD_ADMIN_SERVER_H__ */
diff --git a/po/POTFILES.in b/po/POTFILES.in
index ff207cbb03e7..171d2b1e1534 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -10,7 +10,6 @@ gnulib/lib/gai_strerror.c
 gnulib/lib/regcomp.c
 src/access/viraccessdriverpolkit.c
 src/access/viraccessmanager.c
-src/admin/admin_remote.c
 src/bhyve/bhyve_command.c
 src/bhyve/bhyve_device.c
 src/bhyve/bhyve_driver.c
diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x
index 742ed2a89cd1..205bfe8f7a52 100644
--- a/src/admin/admin_protocol.x
+++ b/src/admin/admin_protocol.x
@@ -60,7 +60,7 @@ struct admin_connect_list_servers_args {
 unsigned int flags;
 };

-struct admin_connect_list_servers_ret {
+struct admin_connect_list_servers_ret { /* insert@1 */
 admin_nonnull_server servers;
 unsigned int ret;
 };
@@ -103,7 +103,7 @@ enum admin_procedure {
 ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3,

 /**
-  * @generate: none
+  * @generate: both
   */
 ADMIN_PROC_CONNECT_LIST_SERVERS = 4
 };
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c
index 3745b9e7017a..21e0dd30c4bf 100644
--- a/src/admin/admin_remote.c
+++ b/src/admin/admin_remote.c
@@ -224,66 +224,3 @@ remoteAdminPrivNew(const char *sock_path)
 virObjectUnref(priv);
 return NULL;
 }
-
-static int
-remoteAdminConnectListServers(virAdmConnectPtr conn,
-  virAdmServerPtr **servers,
-  unsigned int flags)
-{
-int rv = -1;
-size_t i;
-virAdmServerPtr *tmp_srvs = NULL;
-remoteAdminPrivPtr priv = conn->privateData;
-admin_connect_list_servers_args args;
-

[libvirt] [PATCH 18/21] admin: Add virAdmConnectLookupServer

2016-03-09 Thread Martin Kletzander 
It does not have a suffix ByName because there are no other means of
looking up the server and since the name is known, this should be the
preferred one.

Signed-off-by: Martin Kletzander 
---
 daemon/admin_server.c   | 10 ++
 daemon/admin_server.h   |  5 +
 include/libvirt/libvirt-admin.h |  4 
 src/admin/admin_protocol.x  | 16 +++-
 src/admin_protocol-structs  |  8 
 src/libvirt-admin.c | 36 
 src/libvirt_admin_private.syms  |  2 ++
 src/libvirt_admin_public.syms   |  1 +
 8 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 6eabbe4ae6d5..1d16bc9e9379 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -56,3 +56,13 @@ adminConnectListServers(virNetDaemonPtr dmn,
  cleanup:
 return ret;
 }
+
+virNetServerPtr
+adminConnectLookupServer(virNetDaemonPtr dmn,
+ const char *name,
+ unsigned int flags)
+{
+virCheckFlags(flags, NULL);
+
+return virNetDaemonGetServer(dmn, name);
+}
diff --git a/daemon/admin_server.h b/daemon/admin_server.h
index b77653f6c384..9d0adf02c7ad 100644
--- a/daemon/admin_server.h
+++ b/daemon/admin_server.h
@@ -25,9 +25,14 @@
 # define __LIBVIRTD_ADMIN_SERVER_H__

 # include "rpc/virnetdaemon.h"
+# include "rpc/virnetserver.h"

 int adminConnectListServers(virNetDaemonPtr dmn,
 virNetServerPtr **servers,
 unsigned int flags);

+virNetServerPtr adminConnectLookupServer(virNetDaemonPtr dmn,
+ const char *name,
+ unsigned int flags);
+
 #endif /* __LIBVIRTD_ADMIN_SERVER_H__ */
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index e9ec3941023a..25bcbf47ddeb 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -106,6 +106,10 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr 
conn,

 const char *virAdmServerGetName(virAdmServerPtr srv);

+virAdmServerPtr virAdmConnectLookupServer(virAdmConnectPtr conn,
+  const char *name,
+  unsigned int flags);
+
 # ifdef __cplusplus
 }
 # endif
diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x
index 205bfe8f7a52..6590980ce1ca 100644
--- a/src/admin/admin_protocol.x
+++ b/src/admin/admin_protocol.x
@@ -65,6 +65,15 @@ struct admin_connect_list_servers_ret { /* insert@1 */
 unsigned int ret;
 };

+struct admin_connect_lookup_server_args {
+admin_nonnull_string name;
+unsigned int flags;
+};
+
+struct admin_connect_lookup_server_ret {
+admin_nonnull_server srv;
+};
+
 /* Define the program number, protocol version and procedure numbers here. */
 const ADMIN_PROGRAM = 0x06900690;
 const ADMIN_PROTOCOL_VERSION = 1;
@@ -105,5 +114,10 @@ enum admin_procedure {
 /**
   * @generate: both
   */
-ADMIN_PROC_CONNECT_LIST_SERVERS = 4
+ADMIN_PROC_CONNECT_LIST_SERVERS = 4,
+
+/**
+  * @generate: both
+  */
+ADMIN_PROC_CONNECT_LOOKUP_SERVER = 5
 };
diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs
index 8f2633ae7ce2..d8aca0617147 100644
--- a/src/admin_protocol-structs
+++ b/src/admin_protocol-structs
@@ -19,9 +19,17 @@ struct admin_connect_list_servers_ret {
 } servers;
 u_int  ret;
 };
+struct admin_connect_lookup_server_args {
+admin_nonnull_string   name;
+u_int  flags;
+};
+struct admin_connect_lookup_server_ret {
+admin_nonnull_server   srv;
+};
 enum admin_procedure {
 ADMIN_PROC_CONNECT_OPEN = 1,
 ADMIN_PROC_CONNECT_CLOSE = 2,
 ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3,
 ADMIN_PROC_CONNECT_LIST_SERVERS = 4,
+ADMIN_PROC_CONNECT_LOOKUP_SERVER = 5,
 };
diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index add4fcc3576a..7a7bebf7d6c9 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -639,3 +639,39 @@ virAdmConnectListServers(virAdmConnectPtr conn,
 virDispatchError(NULL);
 return -1;
 }
+
+/**
+ * virAdmConnectLookupServer:
+ * @conn: daemon connection reference
+ * @name: name of the server too lookup
+ * @flags: unused, must be 0
+ *
+ * Collect list of all servers provided by daemon the client is connected to.
+ *
+ * Returns the number of servers available on daemon side or -1 in case of a
+ * failure, setting @servers to NULL. There is a guaranteed extra element set
+ * to NULL in the @servers list returned to make the iteration easier, 
excluding
+ * this extra element from the final count.
+ * Caller is responsible to call virAdmServerFree() on each list element,
+ * followed by freeing @servers.
+ */
+virAdmServerPtr
+virAdmConnectLookupServer(virAdmConnectPtr conn,
+

[libvirt] [PATCH 10/21] Change virNetDaemonGetServerNames to virNetDaemonGetServers

2016-03-09 Thread Martin Kletzander 
For now it does not matter which ones we return as the code is similarly
complex, however it will fit in with other constructs in the future,
mainly when we will be able to generate dispatch helpers.

Signed-off-by: Martin Kletzander 
---
 daemon/admin_server.c   | 18 +-
 src/libvirt_remote.syms |  1 +
 src/rpc/virnetdaemon.c  | 43 +--
 src/rpc/virnetdaemon.h  |  2 +-
 4 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 0ccb39e91c77..a35f33130607 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -42,33 +42,17 @@ adminDaemonListServers(virNetDaemonPtr dmn,
unsigned int flags)
 {
 int ret = -1;
-const char **srv_names = NULL;
 virNetServerPtr *srvs = NULL;
-size_t i;
-ssize_t nsrvs = 0;

 virCheckFlags(0, -1);

-if ((nsrvs = virNetDaemonGetServerNames(dmn, _names)) < 0)
+if ((ret = virNetDaemonGetServers(dmn, )) < 0)
 goto cleanup;

 if (servers) {
-if (VIR_ALLOC_N(srvs, nsrvs) < 0)
-goto cleanup;
-
-for (i = 0; i < nsrvs; i++) {
-if (!(srvs[i] = virNetDaemonGetServer(dmn, srv_names[i])))
-goto cleanup;
-}
-
 *servers = srvs;
 srvs = NULL;
 }
-
-ret = nsrvs;
-
  cleanup:
-VIR_FREE(srv_names);
-virObjectListFree(srvs);
 return ret;
 }
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 4a3c5dfc8835..66f93833afda 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -65,6 +65,7 @@ virNetDaemonAddSignalHandler;
 virNetDaemonAutoShutdown;
 virNetDaemonClose;
 virNetDaemonGetServer;
+virNetDaemonGetServers;
 virNetDaemonHasClients;
 virNetDaemonIsPrivileged;
 virNetDaemonNew;
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 9181ad7c474b..aeeeff23cb91 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -196,39 +196,54 @@ virNetDaemonGetServer(virNetDaemonPtr dmn,
 }


+struct collectData {
+virNetServerPtr **servers;
+size_t nservers;
+};
+
+
+static int
+collectServers(void *payload,
+   const void *name ATTRIBUTE_UNUSED,
+   void *opaque)
+{
+virNetServerPtr srv = virObjectRef(payload);
+struct collectData *data = opaque;
+
+if (!srv)
+return -1;
+
+return VIR_APPEND_ELEMENT(*data->servers, data->nservers, srv);
+}
+
+
 /*
  * Returns number of names allocated in *servers, on error sets
  * *servers to NULL and returns -1.  List of *servers must be free()d,
  * but not the items in it (similarly to virHashGetItems).
  */
 ssize_t
-virNetDaemonGetServerNames(virNetDaemonPtr dmn,
-   const char ***servers)
+virNetDaemonGetServers(virNetDaemonPtr dmn,
+   virNetServerPtr **servers)
 {
-virHashKeyValuePairPtr items = NULL;
-size_t nservers = 0;
+struct collectData data = { servers, 0 };
 ssize_t ret = -1;
-size_t i;

 *servers = NULL;

 virObjectLock(dmn);

-items = virHashGetItems(dmn->servers, NULL);
-if (!items)
+if (virHashForEach(dmn->servers, collectServers, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get all servers from daemon"));
 goto cleanup;
-
-for (i = 0; items[i].key; i++) {
-if (VIR_APPEND_ELEMENT(*servers, nservers, items[i].key) < 0)
-goto cleanup;
 }

-ret = nservers;
+ret = data.nservers;

  cleanup:
 if (ret < 0)
-VIR_FREE(*servers);
-VIR_FREE(items);
+virObjectListFree(*servers);
 virObjectUnlock(dmn);
 return ret;
 }
diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h
index 0084dfd6527a..211009c5030a 100644
--- a/src/rpc/virnetdaemon.h
+++ b/src/rpc/virnetdaemon.h
@@ -82,6 +82,6 @@ bool virNetDaemonHasClients(virNetDaemonPtr dmn);

 virNetServerPtr virNetDaemonGetServer(virNetDaemonPtr dmn,
   const char *serverName);
-ssize_t virNetDaemonGetServerNames(virNetDaemonPtr dmn, const char ***servers);
+ssize_t virNetDaemonGetServers(virNetDaemonPtr dmn, virNetServerPtr **servers);

 #endif /* __VIR_NET_DAEMON_H__ */
-- 
2.7.2

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

[libvirt] [PATCH 21/21] remote: Generate what's possible

2016-03-09 Thread Martin Kletzander 
Since gendisplatch can now generate "modern" *ListAll* functions, let
them all be generated.

Signed-off-by: Martin Kletzander 
---
 daemon/remote.c  | 599 ---
 src/remote/remote_driver.c   | 654 ---
 src/remote/remote_protocol.x |  40 +--
 3 files changed, 20 insertions(+), 1273 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index f5ca2acc98e2..2bf9e8392252 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1512,64 +1512,6 @@ 
remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS
 }

 static int
-remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED,
-virNetServerClientPtr client,
-virNetMessagePtr msg ATTRIBUTE_UNUSED,
-virNetMessageErrorPtr rerr,
-remote_connect_list_all_domains_args *args,
-remote_connect_list_all_domains_ret *ret)
-{
-virDomainPtr *doms = NULL;
-int ndomains = 0;
-size_t i;
-int rv = -1;
-struct daemonClientPrivate *priv = 
virNetServerClientGetPrivateData(client);
-
-if (!priv->conn) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
-goto cleanup;
-}
-
-if ((ndomains = virConnectListAllDomains(priv->conn,
- args->need_results ?  : NULL,
- args->flags)) < 0)
-goto cleanup;
-
-if (ndomains > REMOTE_DOMAIN_LIST_MAX) {
-virReportError(VIR_ERR_RPC,
-   _("Too many domains '%d' for limit '%d'"),
-   ndomains, REMOTE_DOMAIN_LIST_MAX);
-goto cleanup;
-}
-
-if (doms && ndomains) {
-if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0)
-goto cleanup;
-
-ret->domains.domains_len = ndomains;
-
-for (i = 0; i < ndomains; i++)
-make_nonnull_domain(ret->domains.domains_val + i, doms[i]);
-} else {
-ret->domains.domains_len = 0;
-ret->domains.domains_val = NULL;
-}
-
-ret->ret = ndomains;
-
-rv = 0;
-
- cleanup:
-if (rv < 0)
-virNetMessageSaveError(rerr);
-if (doms && ndomains > 0)
-for (i = 0; i < ndomains; i++)
-virObjectUnref(doms[i]);
-VIR_FREE(doms);
-return rv;
-}
-
-static int
 remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server 
ATTRIBUTE_UNUSED,
 virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
 virNetMessagePtr msg 
ATTRIBUTE_UNUSED,
@@ -4564,547 +4506,6 @@ remoteDispatchDomainGetDiskErrors(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 return rv;
 }

-static int
-remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr,
- remote_domain_list_all_snapshots_args 
*args,
- remote_domain_list_all_snapshots_ret *ret)
-{
-virDomainSnapshotPtr *snaps = NULL;
-int nsnaps = 0;
-size_t i;
-int rv = -1;
-struct daemonClientPrivate *priv = 
virNetServerClientGetPrivateData(client);
-virDomainPtr dom = NULL;
-
-if (!priv->conn) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
-goto cleanup;
-}
-
-if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
-goto cleanup;
-
-if ((nsnaps = virDomainListAllSnapshots(dom,
-args->need_results ?  : NULL,
-args->flags)) < 0)
-goto cleanup;
-
-if (nsnaps > REMOTE_DOMAIN_SNAPSHOT_LIST_MAX) {
-virReportError(VIR_ERR_RPC,
-   _("Too many domain snapshots '%d' for limit '%d'"),
-   nsnaps, REMOTE_DOMAIN_SNAPSHOT_LIST_MAX);
-goto cleanup;
-}
-
-if (snaps && nsnaps) {
-if (VIR_ALLOC_N(ret->snapshots.snapshots_val, nsnaps) < 0)
-goto cleanup;
-
-ret->snapshots.snapshots_len = nsnaps;
-
-for (i = 0; i < nsnaps; i++)
-make_nonnull_domain_snapshot(ret->snapshots.snapshots_val + i,
- snaps[i]);
-} else {
-ret->snapshots.snapshots_len = 0;
-ret->snapshots.snapshots_val = NULL;
-}
-
-ret->ret = nsnaps;
-rv = 0;
-
- cleanup:
-if (rv < 0)
-virNetMessageSaveError(rerr);
-virObjectUnref(dom);
-if (snaps && nsnaps > 0)
-for (i = 0; i < nsnaps; i++)
-

[libvirt] [PATCH 08/21] Expose virNetServerGetName

2016-03-09 Thread Martin Kletzander 
Signed-off-by: Martin Kletzander 
---
 src/libvirt_remote.syms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 90a453c8be9c..4a3c5dfc8835 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -100,6 +100,7 @@ virNetServerAddClient;
 virNetServerAddProgram;
 virNetServerAddService;
 virNetServerClose;
+virNetServerGetName;
 virNetServerHasClients;
 virNetServerNew;
 virNetServerNewPostExecRestart;
-- 
2.7.2

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

[libvirt] [PATCH 00/21] Various (tiny) clean-ups and changes for daemon/admin

2016-03-09 Thread Martin Kletzander 
I wanted to make virAdmConnectListAllServers()'s dispatch function
generated by our mighty gendispatch script.  And when doing that I
stumbled upon various other things, changed what seemed better and at
the end I ended up with one huge commit.  Because lot of those changes
should not be done together in one commit, I split each tiny part so
it's nicer to review.  However, some might point out that some commits
do not belong together.  To that I say it was a pain splitting the
commit, don't make me split the series as well, please =)

This "conflicts" with Erik's series that deals with worker threads
tuning.  We about know that and we will merge those together, or
rather rebase on top of each other based on which version of
virAdmConnectLookupServer{,ByName}() you and we like better.


Martin Kletzander (21):
  admin: Make virAdmServerFree() handle NULL gracefully
  admin: Check for flags properly
  server: Store server name in server object
  daemon: Get server name from the server itself
  virerror: Introduce new error type NO_SERVER
  daemon: Set error for unknown server name
  daemon: Properly check for clients
  Expose virNetServerGetName
  admin: Do not work with virAdm on the server side
  Change virNetDaemonGetServerNames to virNetDaemonGetServers
  admin: Don't use priority for admin APIs
  virt-admin: Don't leak uri all over the place
  admin: Be consistent when resetting errors
  gendispatch: Cluster, don't capture if not needed
  gendispatch: Be able to generate multi-return values
  admin: Generate ConnectListServers dispatch helpers
  gendispatch: Accept server as an argument
  admin: Add virAdmConnectLookupServer
  gendispatch: Remember the name of snapshot variable name
  gendispatch: Support modern listing of more types
  remote: Generate what's possible

 daemon/admin.c  |  49 +--
 daemon/admin_server.c   |  36 +--
 daemon/admin_server.h   |  12 +-
 daemon/libvirtd.c   |  10 +-
 daemon/remote.c | 599 
 include/libvirt/libvirt-admin.h |   4 +
 include/libvirt/virterror.h |   1 +
 po/POTFILES.in  |   1 -
 src/admin/admin_protocol.x  |  21 +-
 src/admin/admin_remote.c|  63 
 src/admin_protocol-structs  |   8 +
 src/libvirt-admin.c |  48 ++-
 src/libvirt_admin_private.syms  |   2 +
 src/libvirt_admin_public.syms   |   1 +
 src/libvirt_remote.syms |   2 +
 src/locking/lock_daemon.c   |   5 +-
 src/logging/log_daemon.c|   5 +-
 src/lxc/lxc_controller.c|   5 +-
 src/remote/remote_driver.c  | 654 
 src/remote/remote_protocol.x|  40 +--
 src/rpc/gendispatch.pl  | 374 +--
 src/rpc/virnetdaemon.c  |  65 ++--
 src/rpc/virnetdaemon.h  |   3 +-
 src/rpc/virnetserver.c  |  20 +-
 src/rpc/virnetserver.h  |   6 +-
 src/util/virerror.c |   6 +
 tests/virnetdaemontest.c|  10 +-
 tools/virt-admin.c  |   8 +-
 28 files changed, 509 insertions(+), 1549 deletions(-)

--
2.7.2

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

[libvirt] [PATCH 07/21] daemon: Properly check for clients

2016-03-09 Thread Martin Kletzander 
virHashForEach() returns 0 if everything went ince, so our session
daemon was timing out even when there was a client connected.

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

Signed-off-by: Martin Kletzander 
---
 src/rpc/virnetdaemon.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 8ddcd9baddc9..9181ad7c474b 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -849,15 +849,23 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 static int
 daemonServerHasClients(void *payload,
const void *key ATTRIBUTE_UNUSED,
-   void *opaque ATTRIBUTE_UNUSED)
+   void *opaque)
 {
+bool *clients = opaque;
 virNetServerPtr srv = payload;

-return virNetServerHasClients(srv);
+if (virNetServerHasClients(srv))
+*clients = true;
+
+return 0;
 }

 bool
 virNetDaemonHasClients(virNetDaemonPtr dmn)
 {
-return virHashForEach(dmn->servers, daemonServerHasClients, NULL) > 0;
+bool ret = false;
+
+virHashForEach(dmn->servers, daemonServerHasClients, );
+
+return ret;
 }
-- 
2.7.2

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

[libvirt] [PATCH 02/21] admin: Check for flags properly

2016-03-09 Thread Martin Kletzander 
Function virAdmConnectListServers() forgot to check for flags at all,
virAdmConnectOpen() on the other hand checked them but did no dispatch
the error.  virCheckFlags() should be used only when there should be no
other thing done after erroring out and since they are used on different
places then just public API, they cannot dispatch errors.  So let' suse
virCheckFlagsGoto instead.

Signed-off-by: Martin Kletzander 
---
 src/libvirt-admin.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 54ae5ad3135e..44b4d4090e59 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -204,7 +204,7 @@ virAdmConnectOpen(const char *name, unsigned int flags)

 VIR_DEBUG("flags=%x", flags);
 virResetLastError();
-virCheckFlags(VIR_CONNECT_NO_ALIASES, NULL);
+virCheckFlagsGoto(VIR_CONNECT_NO_ALIASES, error);

 if (!(conn = virAdmConnectNew()))
 goto error;
@@ -603,7 +603,7 @@ int virAdmServerFree(virAdmServerPtr srv)
  * @conn: daemon connection reference
  * @servers: Pointer to a list to store an array containing objects or NULL
  *   if the list is not required (number of servers only)
- * @flags: bitwise-OR of virAdmConnectListServersFlags
+ * @flags: unused, must be 0
  *
  * Collect list of all servers provided by daemon the client is connected to.
  *
@@ -624,6 +624,7 @@ virAdmConnectListServers(virAdmConnectPtr conn,
 VIR_DEBUG("conn=%p, servers=%p, flags=%x", conn, servers, flags);

 virResetLastError();
+virCheckFlagsGoto(0, error);

 if (servers)
 *servers = NULL;
-- 
2.7.2

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

[libvirt] [PATCH 04/21] daemon: Get server name from the server itself

2016-03-09 Thread Martin Kletzander 
Since servers know their name, there is no need to supply such
information twice.  Also defeats inconsistencies.

Signed-off-by: Martin Kletzander 
---
 daemon/libvirtd.c | 4 ++--
 src/locking/lock_daemon.c | 2 +-
 src/logging/log_daemon.c  | 2 +-
 src/lxc/lxc_controller.c  | 2 +-
 src/rpc/virnetdaemon.c| 2 +-
 src/rpc/virnetdaemon.h| 1 -
 tests/virnetdaemontest.c  | 2 +-
 7 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index beddec1bb852..3d38a46ee7d2 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1400,7 +1400,7 @@ int main(int argc, char **argv) {
 }

 if (!(dmn = virNetDaemonNew()) ||
-virNetDaemonAddServer(dmn, "libvirtd", srv) < 0) {
+virNetDaemonAddServer(dmn, srv) < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
@@ -1474,7 +1474,7 @@ int main(int argc, char **argv) {
 goto cleanup;
 }

-if (virNetDaemonAddServer(dmn, "admin", srvAdm) < 0) {
+if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
 ret = VIR_DAEMON_ERR_INIT;
 goto cleanup;
 }
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index c89b842f0faf..973e6918c9fe 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -171,7 +171,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool 
privileged)
 goto error;

 if (!(lockd->dmn = virNetDaemonNew()) ||
-virNetDaemonAddServer(lockd->dmn, "virtlockd", srv) < 0)
+virNetDaemonAddServer(lockd->dmn, srv) < 0)
 goto error;

 if (!(lockd->lockspaces = virHashCreate(VIR_LOCK_DAEMON_NUM_LOCKSPACES,
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 866e8a85f4fa..68f0647638b3 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -161,7 +161,7 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool 
privileged)
 goto error;

 if (!(logd->dmn = virNetDaemonNew()) ||
-virNetDaemonAddServer(logd->dmn, "virtlogd", logd->srv) < 0)
+virNetDaemonAddServer(logd->dmn, logd->srv) < 0)
 goto error;

 if (!(logd->handler = virLogHandlerNew(privileged,
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 21cf71fa08a5..b1b55f0e02e1 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -968,7 +968,7 @@ static int virLXCControllerSetupServer(virLXCControllerPtr 
ctrl)
 goto error;

 if (!(ctrl->daemon = virNetDaemonNew()) ||
-virNetDaemonAddServer(ctrl->daemon, "LXC", srv) < 0)
+virNetDaemonAddServer(ctrl->daemon, srv) < 0)
 goto error;

 virNetDaemonUpdateServices(ctrl->daemon, true);
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 7ae06dd38007..3dc47792a8ba 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -158,10 +158,10 @@ virNetDaemonNew(void)

 int
 virNetDaemonAddServer(virNetDaemonPtr dmn,
-  const char *serverName,
   virNetServerPtr srv)
 {
 int ret = -1;
+const char *serverName = virNetServerGetName(srv);

 virObjectLock(dmn);

diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h
index 9a3404f544fd..0084dfd6527a 100644
--- a/src/rpc/virnetdaemon.h
+++ b/src/rpc/virnetdaemon.h
@@ -38,7 +38,6 @@
 virNetDaemonPtr virNetDaemonNew(void);

 int virNetDaemonAddServer(virNetDaemonPtr dmn,
-  const char *serverName,
   virNetServerPtr srv);

 virNetServerPtr virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c
index 443a406b77f2..1608923d09d7 100644
--- a/tests/virnetdaemontest.c
+++ b/tests/virnetdaemontest.c
@@ -164,7 +164,7 @@ static char *testGenerateJSON(const char *server_name)
 if (!(dmn = virNetDaemonNew()))
 goto cleanup;

-if (virNetDaemonAddServer(dmn, server_name, srv) < 0)
+if (virNetDaemonAddServer(dmn, srv) < 0)
 goto cleanup;

 if (!(json = virNetDaemonPreExecRestart(dmn)))
-- 
2.7.2

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

[libvirt] [PATCH 12/21] virt-admin: Don't leak uri all over the place

2016-03-09 Thread Martin Kletzander 
virAdmConnectGetURI() returns string that needs to be free()'d but we
haven't done that very much.

Signed-off-by: Martin Kletzander 
---
 tools/virt-admin.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index c47053639dd0..bc9ae9366280 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -69,9 +69,6 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn ATTRIBUTE_UNUSED,
 virErrorPtr error;
 char *uri = NULL;

-if (reason == VIR_CONNECT_CLOSE_REASON_CLIENT)
-return;
-
 error = virSaveLastError();
 uri = virAdmConnectGetURI(conn);

@@ -98,6 +95,8 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn ATTRIBUTE_UNUSED,
 virSetError(error);
 virFreeError(error);
 }
+
+VIR_FREE(uri);
 }

 static int
@@ -323,7 +322,7 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 int nsrvs = 0;
 size_t i;
 bool ret = false;
-const char *uri = NULL;
+char *uri = NULL;
 virAdmServerPtr *srvs = NULL;
 vshAdmControlPtr priv = ctl->privData;

@@ -347,6 +346,7 @@ cmdSrvList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 virAdmServerFree(srvs[i]);
 VIR_FREE(srvs);
 }
+VIR_FREE(uri);

 return ret;
 }
-- 
2.7.2

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

[libvirt] [PATCH 19/21] gendispatch: Remember the name of snapshot variable name

2016-03-09 Thread Martin Kletzander 
Until now, the script assumed that snapshot name is 'snap', but that's
going to change.

Signed-off-by: Martin Kletzander 
---
 src/rpc/gendispatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index bd422842bc48..21f16d19bbed 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -510,14 +510,14 @@ elsif ($mode eq "server") {
 push(@args_list, "$2");
 push(@free_list,
  "virObjectUnref($2);");
-} elsif ($args_member =~ m/^remote_nonnull_domain_snapshot /) {
+} elsif ($args_member =~ m/^remote_nonnull_domain_snapshot 
(\S+);$/) {
 push(@vars_list, "virDomainPtr dom = NULL");
 push(@vars_list, "virDomainSnapshotPtr snapshot = NULL");
 push(@getters_list,
- "if (!(dom = get_nonnull_domain(priv->conn, 
args->snap.dom)))\n" .
+ "if (!(dom = get_nonnull_domain(priv->conn, 
args->${1}.dom)))\n" .
  "goto cleanup;\n" .
  "\n" .
- "if (!(snapshot = 
get_nonnull_domain_snapshot(dom, args->snap)))\n" .
+ "if (!(snapshot = 
get_nonnull_domain_snapshot(dom, args->${1})))\n" .
  "goto cleanup;\n");
 push(@args_list, "snapshot");
 push(@free_list,
-- 
2.7.2

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

[libvirt] [PATCH 20/21] gendispatch: Support modern listing of more types

2016-03-09 Thread Martin Kletzander 
Signed-off-by: Martin Kletzander 
---
 src/rpc/gendispatch.pl | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 21f16d19bbed..5e800ab05e41 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -649,7 +649,7 @@ elsif ($mode eq "server") {
 if (!$modern_ret_as_list) {
 push(@ret_list, "ret->$3 = tmp.$3;");
 }
-} elsif ($ret_member =~ m/admin_nonnull_(server) 
(\S+)<(\S+)>;/) {
+} elsif ($ret_member =~ 
m/(?:admin|remote)_nonnull_(secret|nwfilter|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server)
 (\S+)<(\S+)>;/) {
 $modern_ret_struct_name = $1;
 $single_ret_list_error_msg_type = $1;
 $single_ret_list_name = $2;
@@ -1401,7 +1401,7 @@ elsif ($mode eq "client") {
 }

 push(@ret_list, "memcpy(result->$3, ret.$3, 
sizeof(result->$3));");
-} elsif ($ret_member =~ m/admin_nonnull_(server) 
(\S+)<(\S+)>;/) {
+} elsif ($ret_member =~ 
m/(?:admin|remote)_nonnull_(secret|nwfilter|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server)
 (\S+)<(\S+)>;/) {
 my $proc_name = name_to_TypeName($1);

 if ($structprefix eq "admin") {
@@ -1413,6 +1413,7 @@ elsif ($mode eq "client") {
 $modern_ret_struct_name = $1;
 $single_ret_list_name = $2;
 $single_ret_list_max_var = $3;
+$single_ret_list_error_msg_type = $1;

 $modern_ret_as_list = 1;
 } elsif ($ret_member =~ m/<\S+>;/ or $ret_member =~ 
m/\[\S+\];/) {
@@ -1729,12 +1730,13 @@ elsif ($mode eq "client") {
 $callflags = "REMOTE_CALL_LXC";
 }

+my $call_priv = $priv_src;
 if ($structprefix ne "admin") {
-$priv_src = "$priv_src, priv";
+$call_priv = "$call_priv, priv";
 }

 print "\n";
-print "if (call($priv_src, $callflags, $call->{constname},\n";
+print "if (call($call_priv, $callflags, $call->{constname},\n";
 print " (xdrproc_t)xdr_$argtype, (char *)$call_args,\n";
 print " (xdrproc_t)xdr_$rettype, (char *)$call_ret) == -1) 
{\n";

@@ -1777,6 +1779,9 @@ elsif ($mode eq "client") {
 print "}\n";
 print "\n";
 } elsif ($modern_ret_as_list) {
+if ($modern_ret_struct_name eq "domain_snapshot") {
+$priv_src =~ s/->conn//;
+}
 print "if (result) {\n";
 print "if (VIR_ALLOC_N(tmp_results, 
ret.$single_ret_list_name.${single_ret_list_name}_len + 1) < 0)\n";
 print "goto cleanup;\n";
-- 
2.7.2

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

[libvirt] [PATCH 11/21] admin: Don't use priority for admin APIs

2016-03-09 Thread Martin Kletzander 
There are no priority workers as they dn't make sense for now.

Signed-off-by: Martin Kletzander 
---
 src/admin/admin_protocol.x | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x
index 089ce57c748e..742ed2a89cd1 100644
--- a/src/admin/admin_protocol.x
+++ b/src/admin/admin_protocol.x
@@ -104,7 +104,6 @@ enum admin_procedure {

 /**
   * @generate: none
-  * @priority: high
   */
 ADMIN_PROC_CONNECT_LIST_SERVERS = 4
 };
-- 
2.7.2

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

[libvirt] [PATCH 13/21] admin: Be consistent when resetting errors

2016-03-09 Thread Martin Kletzander 
Resetting an error should be the first thing public API does.

Signed-off-by: Martin Kletzander 
---
 src/libvirt-admin.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 44b4d4090e59..add4fcc3576a 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -371,11 +371,12 @@ virAdmConnectIsAlive(virAdmConnectPtr conn)

 VIR_DEBUG("conn=%p", conn);

+virResetLastError();
+
 if (!conn)
 return 0;

 virCheckAdmConnectReturn(conn, -1);
-virResetLastError();

 priv = conn->privateData;
 virObjectLock(priv);
-- 
2.7.2

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

[libvirt] [PATCH 05/21] virerror: Introduce new error type NO_SERVER

2016-03-09 Thread Martin Kletzander 
This serves the same purpose as VIR_ERR_NO_xxx where xxx is any object
that API can be called upon.  Only this partucular one is used for
daemon's servers.

Signed-off-by: Martin Kletzander 
---
 include/libvirt/virterror.h | 1 +
 src/util/virerror.c | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 83f76d84cbbc..1f0702b5890a 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -311,6 +311,7 @@ typedef enum {
 VIR_ERR_XML_INVALID_SCHEMA = 92,/* XML document doesn't validate 
against schema */
 VIR_ERR_MIGRATE_FINISH_OK = 93, /* Finish API succeeded but it is 
expected to return NULL */
 VIR_ERR_AUTH_UNAVAILABLE = 94, /* authentication unavailable */
+VIR_ERR_NO_SERVER = 95, /* Server was not found */
 } virErrorNumber;

 /**
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 377c2b11a2c2..61b9ea016032 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1380,6 +1380,12 @@ virErrorMsg(virErrorNumber error, const char *info)
 case VIR_ERR_MIGRATE_FINISH_OK:
 errmsg = _("migration successfully aborted");
 break;
+case VIR_ERR_NO_SERVER:
+if (info == NULL)
+errmsg = _("Server not found");
+else
+errmsg = _("Server not found: %s");
+break;
 }
 return errmsg;
 }
-- 
2.7.2

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

[libvirt] [PATCH 01/21] admin: Make virAdmServerFree() handle NULL gracefully

2016-03-09 Thread Martin Kletzander 
We don't want to end up like with virDomainFree() and other, right?

Signed-off-by: Martin Kletzander 
---
 src/libvirt-admin.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 36674441b18b..54ae5ad3135e 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -588,6 +588,10 @@ int virAdmServerFree(virAdmServerPtr srv)
 VIR_DEBUG("server=%p", srv);

 virResetLastError();
+
+if (!srv)
+return 0;
+
 virCheckAdmServerReturn(srv, -1);

 virObjectUnref(srv);
-- 
2.7.2

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

[libvirt] [PATCH 14/21] gendispatch: Cluster, don't capture if not needed

2016-03-09 Thread Martin Kletzander 
We were using parenteses for grouping admin|remote even though we didn't
need to capture what's in it.  That caused some changes to be grater
than needed and, to be honest, some confusion as well.  Let's use it as
it should be used.  It'll also make future changes more consistent.

Signed-off-by: Martin Kletzander 
---
 src/rpc/gendispatch.pl | 118 -
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 5c99409edb26..29f2c6ac033b 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -523,16 +523,16 @@ elsif ($mode eq "server") {
 push(@free_list,
  "virObjectUnref(snapshot);\n" .
  "virObjectUnref(dom);");
-} elsif ($args_member =~ 
m/^(?:(admin|remote)_string|remote_uuid) (\S+)<\S+>;/) {
+} elsif ($args_member =~ 
m/^(?:(?:admin|remote)_string|remote_uuid) (\S+)<\S+>;/) {
 push_privconn(\@args_list);
-push(@args_list, "args->$2.$2_val");
-push(@args_list, "args->$2.$2_len");
-} elsif ($args_member =~ 
m/^(?:opaque|(admin|remote)_nonnull_string) (\S+)<\S+>;(.*)$/) {
+push(@args_list, "args->$1.$1_val");
+push(@args_list, "args->$1.$1_len");
+} elsif ($args_member =~ 
m/^(?:opaque|(?:admin|remote)_nonnull_string) (\S+)<\S+>;(.*)$/) {
 push_privconn(\@args_list);

 my $cast = "";
-my $arg_name = $2;
-my $annotation = $3;
+my $arg_name = $1;
+my $annotation = $2;

 if ($annotation ne "") {
 if ($annotation =~ m/\s*\/\*\s*(.*)\s*\*\//) {
@@ -571,16 +571,16 @@ elsif ($mode eq "server") {
 push_privconn(\@args_list);

 push(@args_list, "(unsigned char *) args->$1");
-} elsif ($args_member =~ m/^(admin|remote)_string (\S+);/) {
+} elsif ($args_member =~ m/^(?:admin|remote)_string (\S+);/) {
 push_privconn(\@args_list);

-push(@vars_list, "char *$2");
-push(@optionals_list, "$2");
-push(@args_list, "$2");
-} elsif ($args_member =~ m/^(admin|remote)_nonnull_string 
(\S+);/) {
+push(@vars_list, "char *$1");
+push(@optionals_list, "$1");
+push(@args_list, "$1");
+} elsif ($args_member =~ m/^(?:admin|remote)_nonnull_string 
(\S+);/) {
 push_privconn(\@args_list);

-push(@args_list, "args->$2");
+push(@args_list, "args->$1");
 } elsif ($args_member =~ m/^(unsigned )?int (\S+);/) {
 push_privconn(\@args_list);

@@ -637,52 +637,52 @@ elsif ($mode eq "server") {
 } else {
 die "unhandled type for multi-return-value: 
$ret_member";
 }
-} elsif ($ret_member =~ m/^(admin|remote)_nonnull_string 
(\S+)<(\S+)>;\s*\/\*\s*insert@(\d+)\s*\*\//) {
+} elsif ($ret_member =~ m/^(?:admin|remote)_nonnull_string 
(\S+)<(\S+)>;\s*\/\*\s*insert@(\d+)\s*\*\//) {
 push(@vars_list, "int len");
-splice(@args_list, int($4), 0, ("ret->$2.$2_val"));
-push(@ret_list, "ret->$2.$2_len = len;");
-push(@free_list_on_error, "VIR_FREE(ret->$2.$2_val);");
+splice(@args_list, int($3), 0, ("ret->$1.$1_val"));
+push(@ret_list, "ret->$1.$1_len = len;");
+push(@free_list_on_error, "VIR_FREE(ret->$1.$1_val);");
 $single_ret_var = "len";
 $single_ret_by_ref = 0;
 $single_ret_check = " < 0";
 $single_ret_as_list = 1;
-$single_ret_list_name = $2;
-$single_ret_list_max_var = "max$2";
-$single_ret_list_max_define = $3;
+$single_ret_list_name = $1;
+$single_ret_list_max_var = "max$1";
+$single_ret_list_max_define = $2;
 } elsif ($ret_member =~ m/^(admin|remote)_nonnull_string 
(\S+)<\S+>;/) {
 # error out on unannotated arrays
 die "$1_nonnull_string array without insert@ 
annotation: $ret_member";
-} elsif ($ret_member =~ m/^(admin|remote)_nonnull_string 
(\S+);/) {
+} elsif ($ret_member =~ m/^(?:admin|remote)_nonnull_string 
(\S+);/) {
 if ($call->{ProcName} eq "ConnectGetType") {
 # SPECIAL: virConnectGetType returns a constant

[libvirt] [PATCH 06/21] daemon: Set error for unknown server name

2016-03-09 Thread Martin Kletzander 
Signed-off-by: Martin Kletzander 
---
 src/rpc/virnetdaemon.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 3dc47792a8ba..8ddcd9baddc9 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -187,6 +187,11 @@ virNetDaemonGetServer(virNetDaemonPtr dmn,
 srv = virObjectRef(virHashLookup(dmn->servers, serverName));
 virObjectUnlock(dmn);

+if (!srv) {
+virReportError(VIR_ERR_NO_SERVER,
+   _("No server named '%s'"), serverName);
+}
+
 return srv;
 }

-- 
2.7.2

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

[libvirt] [PATCH 09/21] admin: Do not work with virAdm on the server side

2016-03-09 Thread Martin Kletzander 
virAdm is prefix only used on the client side.  Or at least for now.  On
server, though, this corresponds to virNet structures (virAdmConnect is
virNetDaemon, virAdmServer should be virNetServer, in the future
virAdmClient will be resolved to virNetServerClient, and so on).

This will also make future work clearer and easier.

Signed-off-by: Martin Kletzander 
---
 daemon/admin.c| 6 +++---
 daemon/admin_server.c | 6 +++---
 daemon/admin_server.h | 7 +++
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/daemon/admin.c b/daemon/admin.c
index 0c1ddc07095b..cae2a84d84b8 100644
--- a/daemon/admin.c
+++ b/daemon/admin.c
@@ -82,9 +82,9 @@ remoteAdmClientInitHook(virNetServerClientPtr client 
ATTRIBUTE_UNUSED,

 static void
 make_nonnull_server(admin_nonnull_server *srv_dst,
-virAdmServerPtr srv_src)
+virNetServerPtr srv_src)
 {
-ignore_value(VIR_STRDUP_QUIET(srv_dst->name, srv_src->name));
+ignore_value(VIR_STRDUP_QUIET(srv_dst->name, 
virNetServerGetName(srv_src)));
 }

 /* Functions */
@@ -141,7 +141,7 @@ adminDispatchConnectListServers(virNetServerPtr server 
ATTRIBUTE_UNUSED,
 admin_connect_list_servers_args *args,
 admin_connect_list_servers_ret *ret)
 {
-virAdmServerPtr *servers = NULL;
+virNetServerPtr *servers = NULL;
 int nservers = 0;
 int rv = -1;
 size_t i;
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 0196bfe8d47d..0ccb39e91c77 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -38,12 +38,12 @@ VIR_LOG_INIT("daemon.admin_server");

 int
 adminDaemonListServers(virNetDaemonPtr dmn,
-   virAdmServerPtr **servers,
+   virNetServerPtr **servers,
unsigned int flags)
 {
 int ret = -1;
 const char **srv_names = NULL;
-virAdmServerPtr *srvs = NULL;
+virNetServerPtr *srvs = NULL;
 size_t i;
 ssize_t nsrvs = 0;

@@ -57,7 +57,7 @@ adminDaemonListServers(virNetDaemonPtr dmn,
 goto cleanup;

 for (i = 0; i < nsrvs; i++) {
-if (!(srvs[i] = virAdmGetServer(NULL, srv_names[i])))
+if (!(srvs[i] = virNetDaemonGetServer(dmn, srv_names[i])))
 goto cleanup;
 }

diff --git a/daemon/admin_server.h b/daemon/admin_server.h
index 2a5aa163352c..606442c19f5f 100644
--- a/daemon/admin_server.h
+++ b/daemon/admin_server.h
@@ -26,9 +26,8 @@

 # include "rpc/virnetdaemon.h"

-int
-adminDaemonListServers(virNetDaemonPtr dmn,
-   virAdmServerPtr **servers,
-   unsigned int flags);
+int adminDaemonListServers(virNetDaemonPtr dmn,
+   virNetServerPtr **servers,
+   unsigned int flags);

 #endif /* __LIBVIRTD_ADMIN_SERVER_H__ */
-- 
2.7.2

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

[libvirt] [PATCH 17/21] gendispatch: Accept server as an argument

2016-03-09 Thread Martin Kletzander 
Signed-off-by: Martin Kletzander 
---
 src/rpc/gendispatch.pl | 41 +
 1 file changed, 41 insertions(+)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index d6ce9b85e158..bd422842bc48 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -600,6 +600,16 @@ elsif ($mode eq "server") {
 } else {
 push(@args_list, "args->$arg_name");
 }
+} elsif ($args_member =~ m/^admin_nonnull_(server) (\S+);/) {
+my $type_name = name_to_TypeName($1);
+
+push(@vars_list, "virNet${type_name}Ptr $2 = NULL");
+push(@getters_list,
+ "if (!($2 = get_nonnull_$1(priv->dmn, 
args->$2)))\n" .
+ "goto cleanup;\n");
+push(@args_list, "$2");
+push(@free_list,
+ "virObjectUnref($2);");
 } elsif ($args_member =~ m/^(\/)?\*/) {
 # ignore comments
 } else {
@@ -819,6 +829,16 @@ elsif ($mode eq "server") {
 } elsif ($ret_member =~ m/^opaque (\S+)<\S+>;/) {
 # error out on unannotated arrays
 die "opaque array without insert@ annotation: 
$ret_member";
+} elsif ($ret_member =~ m/^admin_nonnull_(server) (\S+);/) {
+my $type_name = name_to_TypeName($1);
+
+push(@vars_list, "virNet${type_name}Ptr $2 = NULL");
+push(@ret_list, "make_nonnull_$1(>$2, $2);");
+push(@free_list,
+ "virObjectUnref($2);");
+$single_ret_var = $2;
+$single_ret_by_ref = 0;
+$single_ret_check = " == NULL";
 } elsif ($ret_member =~ m/^(\/)?\*/) {
 # ignore comments
 } else {
@@ -1317,6 +1337,17 @@ elsif ($mode eq "client") {

 push(@args_list, "$type_name $arg_name");
 push(@setters_list, "args.$arg_name = $arg_name;");
+} elsif ($args_member =~ m/^admin_nonnull_(server) (\S+);/) {
+my $name = $1;
+my $arg_name = $2;
+my $type_name = name_to_TypeName($name);
+
+if ($is_first_arg) {
+$priv_src = "$arg_name->conn";
+}
+
+push(@args_list, "virAdm${type_name}Ptr $arg_name");
+push(@setters_list, "make_nonnull_$1($arg_name, 
$arg_name);");
 } elsif ($args_member =~ m/^(\/)?\*/) {
 # ignore comments
 } else {
@@ -1513,6 +1544,16 @@ elsif ($mode eq "client") {
 $single_ret_var = "unsigned long long rv = 0";
 $single_ret_type = "unsigned long long";
 }
+} elsif ($ret_member =~ m/^admin_nonnull_(server) (\S+);/) {
+my $name = $1;
+my $arg_name = $2;
+my $type_name = name_to_TypeName($name);
+
+push(@ret_list, "rv = get_nonnull_$name($priv_src, 
ret.$arg_name);");
+push(@ret_list, "xdr_free((xdrproc_t)xdr_$rettype, (char 
*));");
+
+$single_ret_var = "virAdm${type_name}Ptr rv = NULL";
+$single_ret_type = "virAdm${type_name}Ptr";
 } elsif ($ret_member =~ m/^(\/)?\*/) {
 # ignore comments
 } else {
-- 
2.7.2

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

[libvirt] [PATCH 03/21] server: Store server name in server object

2016-03-09 Thread Martin Kletzander 
At first I did not want to do this, but after trying to implement some
newer feaures in the admin API I realized we need that to make our lives
easier.  On the other hand they are not saved redundantly and the
virNetServer objects are still kept in a hash table.

Signed-off-by: Martin Kletzander 
---
 daemon/libvirtd.c |  6 --
 src/locking/lock_daemon.c |  3 ++-
 src/logging/log_daemon.c  |  3 ++-
 src/lxc/lxc_controller.c  |  3 ++-
 src/rpc/virnetdaemon.c|  1 +
 src/rpc/virnetserver.c| 20 ++--
 src/rpc/virnetserver.h|  6 +-
 tests/virnetdaemontest.c  |  8 +---
 8 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index d1b98c9c3d02..beddec1bb852 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1382,7 +1382,8 @@ int main(int argc, char **argv) {
 goto cleanup;
 }

-if (!(srv = virNetServerNew(config->min_workers,
+if (!(srv = virNetServerNew("libvirtd",
+config->min_workers,
 config->max_workers,
 config->prio_workers,
 config->max_clients,
@@ -1456,7 +1457,8 @@ int main(int argc, char **argv) {
 goto cleanup;
 }

-if (!(srvAdm = virNetServerNew(config->admin_min_workers,
+if (!(srvAdm = virNetServerNew("admin",
+   config->admin_min_workers,
config->admin_max_workers,
0,
config->admin_max_clients,
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index cd70aa94d7a2..c89b842f0faf 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -160,7 +160,8 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool 
privileged)
 return NULL;
 }

-if (!(srv = virNetServerNew(1, 1, 0, config->max_clients,
+if (!(srv = virNetServerNew("virtlockd",
+1, 1, 0, config->max_clients,
 config->max_clients, -1, 0,
 NULL,
 virLockDaemonClientNew,
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 07a03c24199a..866e8a85f4fa 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -150,7 +150,8 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool 
privileged)
 return NULL;
 }

-if (!(logd->srv = virNetServerNew(1, 1, 0, config->max_clients,
+if (!(logd->srv = virNetServerNew("virtlogd",
+  1, 1, 0, config->max_clients,
   config->max_clients, -1, 0,
   NULL,
   virLogDaemonClientNew,
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 76bef82e709c..21cf71fa08a5 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -928,7 +928,8 @@ static int virLXCControllerSetupServer(virLXCControllerPtr 
ctrl)
 LXC_STATE_DIR, ctrl->name) < 0)
 return -1;

-if (!(srv = virNetServerNew(0, 0, 0, 1,
+if (!(srv = virNetServerNew("LXC",
+0, 0, 0, 1,
 0, -1, 0,
 NULL,
 virLXCControllerClientPrivateNew,
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 298fbf4ab086..7ae06dd38007 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -276,6 +276,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
 }

 srv = virNetServerNewPostExecRestart(object,
+ serverName,
  clientPrivNew,
  clientPrivNewPostExecRestart,
  clientPrivPreExecRestart,
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 547e52e409c0..cf48e50603e4 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -49,6 +49,8 @@ struct _virNetServerJob {
 struct _virNetServer {
 virObjectLockable parent;

+char *name;
+
 virThreadPoolPtr workers;

 char *mdnsGroupName;
@@ -304,7 +306,8 @@ static int 
virNetServerDispatchNewClient(virNetServerServicePtr svc,
 }


-virNetServerPtr virNetServerNew(size_t min_workers,
+virNetServerPtr virNetServerNew(const char *name,
+size_t min_workers,
 size_t max_workers,
 size_t priority_workers,
 size_t max_clients,
@@ -332,6 +335,9 @@ virNetServerPtr virNetServerNew(size_t min_workers,
   srv)))
 goto error;

+if (VIR_STRDUP(srv->name, name) < 0)

Re: [libvirt] [PATCH V2] libxl: support assignment from a pool of SRIOV VFs

2016-03-09 Thread Chun Yan Liu 


>>> On 3/10/2016 at 08:08 AM, in message <56e0bb0f.3000...@suse.com>, Jim Fehlig
 wrote: 
> Hi Chunyan, 
>  
> Sorry for the long delay... 
>  
> On 02/23/2016 01:07 AM, Chunyan Liu wrote: 
> > Add codes to support creating domain with network defition of assigning 
> > SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this 
> > kind of network defition. 
> > 
> > Signed-off-by: Chunyan Liu  
> > --- 
> > Changes: 
> > * move bug fix to another patch 
> > * use virDomainNetGetActualType instead of multiple times checking 
> > --- 
> >  src/libxl/libxl_conf.c   |  3 ++- 
> >  src/libxl/libxl_domain.c | 46  
> ++ 
> >  src/libxl/libxl_driver.c | 12  
> >  tests/Makefile.am|  3 +++ 
> >  4 files changed, 63 insertions(+), 1 deletion(-) 
> > 
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c 
> > index 48b8826..3cefbaa 100644 
> > --- a/src/libxl/libxl_conf.c 
> > +++ b/src/libxl/libxl_conf.c 
> > @@ -1453,7 +1453,8 @@ libxlMakeNicList(virDomainDefPtr def,   
> libxl_domain_config *d_config) 
> >  return -1; 
> >   
> >  for (i = 0; i < nnics; i++) { 
> > -if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) 
> > +if (virDomainNetGetActualType(l_nics[i]) 
> > +== VIR_DOMAIN_NET_TYPE_HOSTDEV) 
>  
> AFAICT this change is unrelated to the patch subject and should be done in a 
> separate patch. Also the patch no longer applies cleanly. Can you drop this 
> hunk, rebase, and send a V3? Thanks! 

Updated. V3 is posted.

Thanks,
Chunyan

>  
> Regards, 
> Jim 
>  
> >  continue; 
> >   
> >  if (libxlMakeNic(def, l_nics[i], _nics[nvnics])) 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index 50f7eed..88d1399 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -36,6 +36,7 @@ 
> >  #include "virtime.h" 
> >  #include "locking/domain_lock.h" 
> >  #include "xen_common.h" 
> > +#include "network/bridge_driver.h" 
> >   
> >  #define VIR_FROM_THIS VIR_FROM_LIBXL 
> >   
> > @@ -929,6 +930,48 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev,  
> void *for_callback) 
> >  libxl_event_free(ctx, ev); 
> >  } 
> >   
> > +static int 
> > +libxlNetworkPrepareDevices(virDomainDefPtr def) 
> > +{ 
> > +int ret = -1; 
> > +size_t i; 
> > + 
> > +for (i = 0; i < def->nnets; i++) { 
> > +virDomainNetDefPtr net = def->nets[i]; 
> > +int actualType; 
> > + 
> > +/* If appropriate, grab a physical device from the configured 
> > + * network's pool of devices, or resolve bridge device name 
> > + * to the one defined in the network definition. 
> > + */ 
> > +if (networkAllocateActualDevice(def, net) < 0) 
> > +goto cleanup; 
> > + 
> > +actualType = virDomainNetGetActualType(net); 
> > +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && 
> > +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { 
> > +/* Each type='hostdev' network device must also have a 
> > + * corresponding entry in the hostdevs array. For netdevs 
> > + * that are hardcoded as type='hostdev', this is already 
> > + * done by the parser, but for those allocated from a 
> > + * network / determined at runtime, we need to do it 
> > + * separately. 
> > + */ 
> > +virDomainHostdevDefPtr hostdev =  
> virDomainNetGetActualHostdev(net); 
> > +virDomainHostdevSubsysPCIPtr pcisrc =  
> >source.subsys.u.pci; 
> > + 
> > +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && 
> > +hostdev->source.subsys.type ==  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; 
> > + 
> > +if (virDomainHostdevInsert(def, hostdev) < 0) 
> > +goto cleanup; 
> > +} 
> > +} 
> > +ret = 0; 
> > + cleanup: 
> > +return ret; 
> > +} 
> >   
> >  /* 
> >   * Start a domain through libxenlight. 
> > @@ -1005,6 +1048,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >  vm, true) < 0) 
> >  goto cleanup; 
> >   
> > +if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +goto cleanup; 
> > + 
> >  if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > cfg->ctx, _config) < 0) 
> >  goto cleanup; 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 404016e..eef6651 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -3049,6 +3049,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> >  
> driver, 
> >   
> >  libxl_device_pci_init(); 
> >   
> > +/* For those allocated from a network

[libvirt] [PATCH V3 0/2] libxl: support assignment VF to guest from a pool of SRIOV VFs

2016-03-09 Thread Chunyan Liu 
This patch series is to support assigning VF to guest from a pool of SRIOV VFs
in libxl driver, detailed usage can be referred to:
http://wiki.libvirt.org/page/Networking#Assignment_from_a_pool_of_SRIOV_VFs_in_a_libvirt_.3Cnetwork.3E_definition

Chunyan Liu (2):
  libxl_conf: reuse virDomainNetGetActualtype in libxlMakeNicList
  libxl: support assignment from a pool of SRIOV VFs

 src/libxl/libxl_conf.c   |  3 ++-
 src/libxl/libxl_domain.c | 46 ++
 src/libxl/libxl_driver.c | 12 
 tests/Makefile.am|  3 +++
 4 files changed, 63 insertions(+), 1 deletion(-)

-- 
2.1.4

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

[libvirt] [PATCH V3 2/2] libxl: support assignment from a pool of SRIOV VFs

2016-03-09 Thread Chunyan Liu 
Add codes to support creating domain with network defition of assigning
SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this
kind of network defition.

Signed-off-by: Chunyan Liu 
---
Changes:
  * move a common change in libxl_conf.c into separate patch

 src/libxl/libxl_domain.c | 46 ++
 src/libxl/libxl_driver.c | 12 
 tests/Makefile.am|  3 +++
 3 files changed, 61 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c8d09b1..8191911 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -36,6 +36,7 @@
 #include "virtime.h"
 #include "locking/domain_lock.h"
 #include "xen_common.h"
+#include "network/bridge_driver.h"
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
 
@@ -960,6 +961,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, 
libxl_domain_config *d_config)
 }
 }
 
+static int
+libxlNetworkPrepareDevices(virDomainDefPtr def)
+{
+int ret = -1;
+size_t i;
+
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+int actualType;
+
+/* If appropriate, grab a physical device from the configured
+ * network's pool of devices, or resolve bridge device name
+ * to the one defined in the network definition.
+ */
+if (networkAllocateActualDevice(def, net) < 0)
+goto cleanup;
+
+actualType = virDomainNetGetActualType(net);
+if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
+net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+/* Each type='hostdev' network device must also have a
+ * corresponding entry in the hostdevs array. For netdevs
+ * that are hardcoded as type='hostdev', this is already
+ * done by the parser, but for those allocated from a
+ * network / determined at runtime, we need to do it
+ * separately.
+ */
+virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
+virDomainHostdevSubsysPCIPtr pcisrc = 
>source.subsys.u.pci;
+
+if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+hostdev->source.subsys.type == 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
+pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
+
+if (virDomainHostdevInsert(def, hostdev) < 0)
+goto cleanup;
+}
+}
+ret = 0;
+ cleanup:
+return ret;
+}
 
 /*
  * Start a domain through libxenlight.
@@ -1036,6 +1079,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 vm, true) < 0)
 goto cleanup;
 
+if (libxlNetworkPrepareDevices(vm->def) < 0)
+goto cleanup;
+
 if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
cfg->ctx, _config) < 0)
 goto cleanup;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 87ec5a5..a0b157b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2982,6 +2982,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
driver,
 
 libxl_device_pci_init();
 
+/* For those allocated from a network pool/ determined at runtime, it's
+ * not handled by libxlDomainDeviceDefPostParse as hostdev, we need to
+ * set backend correctly.
+ */
+pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
+
 if (virDomainHostdevFind(vm->def, hostdev, ) >= 0) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("target pci device %.4x:%.2x:%.2x.%.1x already 
exists"),
@@ -3323,6 +3329,12 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr 
driver,
 
 libxl_device_pci_init();
 
+/* For those allocated from a network pool/ determined at runtime, it's
+ * not handled by libxlDomainDeviceDefPostParse as hostdev, we need to
+ * set backend correctly.
+ */
+pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
+
 idx = virDomainHostdevFind(vm->def, hostdev, );
 if (idx < 0) {
 virReportError(VIR_ERR_OPERATION_FAILED,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 90981dc..b08cc7a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -530,6 +530,9 @@ endif ! WITH_XEN
 
 if WITH_LIBXL
 libxl_LDADDS = ../src/libvirt_driver_libxl_impl.la
+if WITH_NETWORK
+libxl_LDADDS += ../src/libvirt_driver_network_impl.la
+endif WITH_NETWORK
 libxl_LDADDS += $(LDADDS)
 
 xlconfigtest_SOURCES = \
-- 
2.1.4

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

[libvirt] [PATCH V3 1/2] libxl_conf: reuse virDomainNetGetActualtype in libxlMakeNicList

2016-03-09 Thread Chunyan Liu 
Reuse existing helper function virDomainNetGetActualtype.

Signed-off-by: Chunyan Liu 
---
 src/libxl/libxl_conf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 93c943b..2b77c59 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1445,7 +1445,8 @@ libxlMakeNicList(virDomainDefPtr def,  
libxl_domain_config *d_config)
 return -1;
 
 for (i = 0; i < nnics; i++) {
-if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
+if (virDomainNetGetActualType(l_nics[i])
+== VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
 
 if (libxlMakeNic(def, l_nics[i], _nics[nvnics]))
-- 
2.1.4

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

[libvirt] [PATCH 1/4] docs: website: Remove the et.redhat.com footer

2016-03-09 Thread Cole Robinson 
This is long since obsolete, just scrap it all
---
 docs/Makefile.am|   3 ---
 docs/et.png | Bin 9779 -> 0 bytes
 docs/footer_corner.png  | Bin 2359 -> 0 bytes
 docs/footer_pattern.png | Bin 817 -> 0 bytes
 docs/generic.css|   4 
 docs/libvirt.css|  34 --
 docs/page.xsl   |   6 --
 7 files changed, 47 deletions(-)
 delete mode 100644 docs/et.png
 delete mode 100644 docs/footer_corner.png
 delete mode 100644 docs/footer_pattern.png

diff --git a/docs/Makefile.am b/docs/Makefile.am
index 1b4353b..f4ce9ce 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -70,8 +70,6 @@ devhelpxsl = devhelp/devhelp.xsl devhelp/html.xsl
 
 png = \
   32favicon.png \
-  footer_corner.png \
-  footer_pattern.png \
   libvirt-header-bg.png \
   libvirt-header-logo.png \
   libvirtLogo.png \
@@ -81,7 +79,6 @@ png = \
   libvirt-driver-arch.png \
   libvirt-object-model.png \
   madeWith.png \
-  et.png \
   migration-managed-direct.png \
   migration-managed-p2p.png \
   migration-native.png \
diff --git a/docs/et.png b/docs/et.png
deleted file mode 100644
index 
115f8a140d74ee72e8d2c847d2bc43bddfb829a5..
GIT binary patch
literal 0
HcmV?d1

literal 9779
zcmW++2RxMjAHVF**4cZc3keA!
z<^Q>VuhTiNyXT(g`~7_0>+>WU8EDZXxsV71f?h{k-2|R<;75yw3jSYq>U4)EirW|+
zQyTaePUCzZUZ3>Uw!97RpCtX9z@Q6);g6gF8WsW9eOv>A9Q<7nK|w*19^Rg}og92!
zBz^qd3O1Fw5C{Q;j=HL8@Z$}e>D$~vyT`SGr|UK|&*^p!xAsjcq)p5ehU6`#w%8^3T*1wrmI+=W8xT|?{V>#ixJ@t#R@63+?Gs4ak_)Xe_9
ze6W8x+iM`BP}Wa3sPe+CbG(JKTfd7=WBIA!};A?9W+VUT$GwaaA-+RV*7L
znM;m9At{+%3Pj9|?WhdDtNbAm0IGW12!(pUdW@D^Vl>VgrfnC`1l

[libvirt] [PATCH 0/4] docs: website: improve readability

2016-03-09 Thread Cole Robinson 
Some css tweaks to improve test readability and be more consistent with
other sites (I was using en.wikipedia.org as a reference)

First 2 bits are cleanups, latter 2 bits are actual changes.

Front before: http://i.imgur.com/zvFqBa9.png
Front after:  http://i.imgur.com/gsHa5Pq.png
List before:  http://i.imgur.com/nh6hMsy.png
List after:   http://i.imgur.com/WxgE4y0.png
Text before:  http://i.imgur.com/8bvgEYg.png
Text after:   http://i.imgur.com/wg0hk4r.png

Cole Robinson (4):
  docs: website: Remove the et.redhat.com footer
  docs: generic.css: minor cleanups
  docs: generic.css: font size tweaks
  docs: generic.css: Indentation and spacing tweaks

 docs/Makefile.am|   3 ---
 docs/et.png | Bin 9779 -> 0 bytes
 docs/footer_corner.png  | Bin 2359 -> 0 bytes
 docs/footer_pattern.png | Bin 817 -> 0 bytes
 docs/generic.css|  57 
 docs/libvirt.css|  34 -
 docs/page.xsl   |   6 -
 7 files changed, 29 insertions(+), 71 deletions(-)
 delete mode 100644 docs/et.png
 delete mode 100644 docs/footer_corner.png
 delete mode 100644 docs/footer_pattern.png

-- 
2.5.0

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

[libvirt] [PATCH 3/4] docs: generic.css: font size tweaks

2016-03-09 Thread Cole Robinson 
- change font-family to just 'sans-serif' rather than hardcode a few
  font families. this means we abide the user's browser font setting,
  and makes us consistent with other sites like en.wikipedia.org
- raise font-size to 90%. this is what en.wikipedia.org uses.

With these two tweaks, libvirt.org text renders the same as
en.wikipedia.org with fedora firefox out of the box config. Previously
the font on libvirt.org was very small and difficult to read.
---
 docs/generic.css | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/generic.css b/docs/generic.css
index 52cd392..285e390 100644
--- a/docs/generic.css
+++ b/docs/generic.css
@@ -2,8 +2,8 @@ body {
   margin: 0em;
   padding: 0px;
   color: rgb(0,0,0);
-  font-family: Verdana, Arial, Helvetica, sans-serif;
-  font-size: smaller;
+  font-family: sans-serif;
+  font-size: 90%;
   background: #ff;
 }
 
-- 
2.5.0

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

[libvirt] [PATCH 4/4] docs: generic.css: Indentation and spacing tweaks

2016-03-09 Thread Cole Robinson 
- Add line-height:150% spacing for all text. This makes text lines far
  less cramped, and seems closer visually to what wikipedia uses.

- Remove bottom and top margin from lists: entries seemed needlessly
  spread out.

- Reduce sublist indentation a bit

- Add a bottom border after headings: IMO this greatly helps in break
  up the vertical flow of a big page of text. Doesn't look great on the
  front page, but helps a lot on dense pages like formatdomain
---
 docs/generic.css | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/docs/generic.css b/docs/generic.css
index 285e390..e862fe0 100644
--- a/docs/generic.css
+++ b/docs/generic.css
@@ -20,12 +20,16 @@ div.body p:first-letter {
 p, ul, ol, dl {
   padding: 0px;
   margin: 0px;
+  line-height: 150%;
+}
+
+p {
   margin-top: 1em;
   margin-bottom: 1em;
 }
 
 ul, ol {
-  margin-left: 3em;
+  margin-left: 2em;
 }
 
 dt {
@@ -45,6 +49,8 @@ h1, h2, h3, h4, h5, h6 {
   margin: 0px;
   padding: 0px;
   margin-top: 0.5em;
+  margin-bottom: 0.5em;
+  border-bottom:1px solid #aaa;
 }
 
 h1 {
-- 
2.5.0

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

[libvirt] [PATCH 2/4] docs: generic.css: minor cleanups

2016-03-09 Thread Cole Robinson 
- Drop some redundant bits
- Use consistent spacing
- Group similar blocks near each other

There should be no functional change
---
 docs/generic.css | 43 +--
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/docs/generic.css b/docs/generic.css
index de9f968..52cd392 100644
--- a/docs/generic.css
+++ b/docs/generic.css
@@ -7,30 +7,40 @@ body {
   background: #ff;
 }
 
+p:first-line {
+  margin-right: 1em;
+}
+
+div.body p:first-letter {
+  font-size: 1.2em;
+  font-weight: bold;
+}
+
+
 p, ul, ol, dl {
   padding: 0px;
   margin: 0px;
+  margin-top: 1em;
+  margin-bottom: 1em;
 }
 
-ol,ul {
+ul, ol {
   margin-left: 3em;
 }
 
-ol,ul,dl,p {
-  margin-top: 1em;
-  margin-bottom: 1em;
+dt {
+  margin-left: 1em;
+  margin-right: 2em;
 }
 
-p:first-line {
-  margin-right: 1em;
+dl dd {
+  margin-left: 2em;
+  margin-right: 2em;
+  margin-bottom: 0.5em;
 }
 
-div.body p:first-letter {
-  font-size: 1.2em;
-  font-weight: bold;
-}
 
-h1,h2,h3,h4,h5,h6 {
+h1, h2, h3, h4, h5, h6 {
   font-weight: bold;
   margin: 0px;
   padding: 0px;
@@ -55,14 +65,3 @@ h5 {
 h6 {
   font-size: 0.8em;
 }
-
-dl dt {
-  margin-left: 1em;
-  margin-right: 2em;
-}
-
-dl dd {
-  margin-left: 2em;
-  margin-right: 2em;
-  margin-bottom: 0.5em;
-}
-- 
2.5.0

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

Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion

2016-03-09 Thread Cole Robinson 
On 03/09/2016 08:20 PM, John Ferlan wrote:
> 
> [...]
> 
>>> OK - so a too long winded way to say - I don't think the following patch
>>> matters as anything (including libvirt) could have changed the file's
>>> ownership and/or protections, but not updated the volume XML. The
>>> refresh updates the volume XML to match the file's protection and
>>> furthermore only matters in the current virFileRemove if the change is
>>> back to root:root and only because we compare vs. gete{uid|gid}().
>>>
>>
>> I don't follow this conclusion really... refreshVol syncs the XML/volume 
>> cache
>> with the current uid/gid/mode on disk, which is then passed down to
>> virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the
>> internal cache is synced, virFileRemove will setuid(cole) and successfully
>> remove the disk image. Without this change, libvirt would setuid(qemu) and
>> fail to remove the disk.
>>
>> So I don't see how this patch is unneeded, or only works for root:root
>>
> 
> I was considering the checks:
> 
> +if (geteuid() != 0)
> +return false;
> 
> We're running as root...
> 
> +
> +/* uid/gid weren'd specified */
> +if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
> +return false;
> 
> We've provided qemu:qemu...
> 
> +
> +/* already running as proper uid/gid */
> +if (uid == geteuid() && gid == getegid())
> +return false;
> +
> 
> At this point geteuid() would return 0 (root)
> 
> Maybe I'm wrong...  I suppose it cannot hurt, but without patch 3 we'd
> go through the setuid path - I think. I could also be missing something
> really obvious.
> 

Right, we would go through the setuid path... but it would _succeed_, because
we would be setuid($correct-file-uid) and not setuid($out-of-date-file-uid)

This patch is infact still needed; consider the case when the cached uid is
out of date on NFS: we will do setuid($out-of-date-file-uid) and fail in the
same way as the original report.

- Cole

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

Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion

2016-03-09 Thread John Ferlan 

[...]

>> OK - so a too long winded way to say - I don't think the following patch
>> matters as anything (including libvirt) could have changed the file's
>> ownership and/or protections, but not updated the volume XML. The
>> refresh updates the volume XML to match the file's protection and
>> furthermore only matters in the current virFileRemove if the change is
>> back to root:root and only because we compare vs. gete{uid|gid}().
>>
> 
> I don't follow this conclusion really... refreshVol syncs the XML/volume cache
> with the current uid/gid/mode on disk, which is then passed down to
> virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the
> internal cache is synced, virFileRemove will setuid(cole) and successfully
> remove the disk image. Without this change, libvirt would setuid(qemu) and
> fail to remove the disk.
> 
> So I don't see how this patch is unneeded, or only works for root:root
> 

I was considering the checks:

+if (geteuid() != 0)
+return false;

We're running as root...

+
+/* uid/gid weren'd specified */
+if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
+return false;

We've provided qemu:qemu...

+
+/* already running as proper uid/gid */
+if (uid == geteuid() && gid == getegid())
+return false;
+

At this point geteuid() would return 0 (root)

Maybe I'm wrong...  I suppose it cannot hurt, but without patch 3 we'd
go through the setuid path - I think. I could also be missing something
really obvious.

John



> - Cole
> 
>> John
>>
>>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>>> index 1d96618..ded54c9 100644
>>> --- a/src/storage/storage_driver.c
>>> +++ b/src/storage/storage_driver.c
>>> @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
>>>  goto cleanup;
>>>  }
>>>  
>>> +/* Need to ensure we are using up to date uid/gid for deletion */
>>> +if (backend->refreshVol &&
>>> +backend->refreshVol(obj->conn, pool, vol) < 0) {
>>> +/* The file may have been removed behind libvirt's back. Don't
>>> +   error here, let the deletion fail or succeed instead */
>>> +VIR_INFO("Failed to refresh volume before deletion: %s",
>>> + virGetLastErrorMessage());
>>> +virResetLastError();
>>> +}
>>> +
>>>  if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0)
>>>  goto cleanup;
>>>  
>>>
> 

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

Re: [libvirt] [PATCH V2] libxl: support assignment from a pool of SRIOV VFs

2016-03-09 Thread Jim Fehlig 
Hi Chunyan,

Sorry for the long delay...

On 02/23/2016 01:07 AM, Chunyan Liu wrote:
> Add codes to support creating domain with network defition of assigning
> SRIOV VF from a pool. And fix hot plug and unplug SRIOV VF under this
> kind of network defition.
>
> Signed-off-by: Chunyan Liu 
> ---
> Changes:
> * move bug fix to another patch
> * use virDomainNetGetActualType instead of multiple times checking
> ---
>  src/libxl/libxl_conf.c   |  3 ++-
>  src/libxl/libxl_domain.c | 46 ++
>  src/libxl/libxl_driver.c | 12 
>  tests/Makefile.am|  3 +++
>  4 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 48b8826..3cefbaa 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1453,7 +1453,8 @@ libxlMakeNicList(virDomainDefPtr def,  
> libxl_domain_config *d_config)
>  return -1;
>  
>  for (i = 0; i < nnics; i++) {
> -if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
> +if (virDomainNetGetActualType(l_nics[i])
> +== VIR_DOMAIN_NET_TYPE_HOSTDEV)

AFAICT this change is unrelated to the patch subject and should be done in a
separate patch. Also the patch no longer applies cleanly. Can you drop this
hunk, rebase, and send a V3? Thanks!

Regards,
Jim

>  continue;
>  
>  if (libxlMakeNic(def, l_nics[i], _nics[nvnics]))
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 50f7eed..88d1399 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -36,6 +36,7 @@
>  #include "virtime.h"
>  #include "locking/domain_lock.h"
>  #include "xen_common.h"
> +#include "network/bridge_driver.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>  
> @@ -929,6 +930,48 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, 
> void *for_callback)
>  libxl_event_free(ctx, ev);
>  }
>  
> +static int
> +libxlNetworkPrepareDevices(virDomainDefPtr def)
> +{
> +int ret = -1;
> +size_t i;
> +
> +for (i = 0; i < def->nnets; i++) {
> +virDomainNetDefPtr net = def->nets[i];
> +int actualType;
> +
> +/* If appropriate, grab a physical device from the configured
> + * network's pool of devices, or resolve bridge device name
> + * to the one defined in the network definition.
> + */
> +if (networkAllocateActualDevice(def, net) < 0)
> +goto cleanup;
> +
> +actualType = virDomainNetGetActualType(net);
> +if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +/* Each type='hostdev' network device must also have a
> + * corresponding entry in the hostdevs array. For netdevs
> + * that are hardcoded as type='hostdev', this is already
> + * done by the parser, but for those allocated from a
> + * network / determined at runtime, we need to do it
> + * separately.
> + */
> +virDomainHostdevDefPtr hostdev = 
> virDomainNetGetActualHostdev(net);
> +virDomainHostdevSubsysPCIPtr pcisrc = 
> >source.subsys.u.pci;
> +
> +if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> +hostdev->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
> +
> +if (virDomainHostdevInsert(def, hostdev) < 0)
> +goto cleanup;
> +}
> +}
> +ret = 0;
> + cleanup:
> +return ret;
> +}
>  
>  /*
>   * Start a domain through libxenlight.
> @@ -1005,6 +1048,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
>  vm, true) < 0)
>  goto cleanup;
>  
> +if (libxlNetworkPrepareDevices(vm->def) < 0)
> +goto cleanup;
> +
>  if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
> cfg->ctx, _config) < 0)
>  goto cleanup;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 404016e..eef6651 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3049,6 +3049,12 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> driver,
>  
>  libxl_device_pci_init();
>  
> +/* For those allocated from a network pool/ determined at runtime, it's
> + * not handled by libxlDomainDeviceDefPostParse as hostdev, we need to
> + * set backend correctly.
> + */
> +pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
> +
>  if (virDomainHostdevFind(vm->def, hostdev, ) >= 0) {
>  virReportError(VIR_ERR_OPERATION_FAILED,
> _("target pci device %.4x:%.2x:%.2x.%.1x already 
> exists"),
> @@ -3390,6 +3396,12 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr 
> driver,
>  
>

Re: [libvirt] [PATCH 3/3] util: virfile: Only setuid for virFileRemove if on NFS

2016-03-09 Thread Cole Robinson 
On 03/09/2016 03:34 PM, John Ferlan wrote:
> 
> 
> On 03/09/2016 12:39 PM, Cole Robinson wrote:
>> NFS with root-squash is the only reason we need to do setuid/setgid
>> crazyness in virFileRemove, so limit that behavior to the NFS case.
>> ---
>> I'm not sure though if NFS is the only case we care about this here,
>> or if we want to conditionalize this path on NFS since that makes it
>> more of a pain to test... It's not required to fix the initial bug
>>
>>  src/util/virfile.c | 10 --
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index cea2674..3d1b118 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t 
>> mode,
>>   * owned by the passed uid/gid pair. Needed for NFS with root-squash
>>   */
>>  static bool
>> -virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
>> +virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid)
> 
> You added a new parameter to document...
> 
> ACK with that adjustment.
> 

Thanks, I pushed #2 and #3 with your adjustments

- Cole

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

Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion

2016-03-09 Thread Cole Robinson 
On 03/09/2016 03:28 PM, John Ferlan wrote:
> 
> 
> On 03/09/2016 12:38 PM, Cole Robinson wrote:
>> file volume deletion, via virFileRemove, has some logic that depends
>> on uid/gid owner of the path. This info is cached in the volume XML.
>> We need to be sure we are using up to date uid/gid before attempting
>> to delete the volume, otherwise we can hit a case like this:
>>
>> - test.img created with uid=root
>> - VM starts up using test.img, owner changed to uid=qemu
>> - test.img pool is refreshed, uid=qemu is cached in the volume XML
>> - VM shuts down, volume owner changed to gid=root
>> - vol-delete test.img thinks uid=qemu, virFileRemove does setuid(qemu),
>>   fails to delete test.img since it is actually uid=root
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1289327
>> ---
>>  src/storage/storage_driver.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
> 
> Coincidentally I was thinking about this one today... Still not
> convinced this is the only "root" (ahem) cause...
> 
> For the startup/stop path, I assume your reference is the path through
> virSecurityDACSetOwnershipInternal from virSecurityDACSetOwnership and
> virSecurityDACRestoreFileLabelInternal...
> 
> But what if the mode, owner, group are provided in the XML when the
> volume is created:
> 
> # cat vol.xml
> 
>   test.img
>   
>   
>   10485760
>   10485760
>   
> /home/vm-images/test.img
> 
> 
>   0600
>   107
>   107
>   system_u:object_r:unlabeled_t:s0
> 
>   
> 
> 
> 
> # virsh vol-create default vol.xml
> Vol test.img created from vol.xml
> 
> # virsh vol-delete test.img default
> error: Failed to delete vol test.img
> error: cannot unlink file '/home/vm-images/test.img': Permission denied
> 
> #
> 
> Yes, I know the following two patches resolve this case...  What's
> interesting is the extents we've gone to on creation to set things up,
> but only a few bread crumbs are left for deletion. The assumption at
> deletion being that no one changed anything from creation, but we see
> that's not true.
> 
> I've been under the assumption that the owner gets set/changed during
> virStorageBackendCreateRaw, virStorageFileBackendFileCreate, or
> virStorageBackendCreateExecCommand.  The setting of the owner for the
> CreateRaw path would occur because the flags had FORCE_OWNER and
> FORCE_MODE set.  The setting of the owner for the CreateExecCommand in
> the non POOL_NETFS path was because they were supplied in the XML and
> not taken as the default from the running of the command to create the
> file (eg qemu-img) and taking the default file/directory ownership.
> Since we run as root, this can all happen without issues.
> 
> Then after any of the create processing happens, we can refresh the pool
> which calls virStorageBackendUpdateVolTargetInfoFD and changes the
> volume uid, gid, mode based on what the lstat(path, ) has found...
> This perhaps helps in the event
> 
> But when we get to delete, we have no idea how things could have been
> originally created or perhaps (likely) updated, so we only check what we
> can... If we really wanted to be elaborate - save a provided uid, gid,
> mode at creation... That would help at deletion to determine if
> something changed.  Doesn't work well for existing files (daemon start,
> restart, reload).  We'd need to save volume xml's somewhere. Probably
> far more effort...
> 

This description may be another manifestation of the issue, however
virt-install/virt-manager _never_ specify UID/GID when creating a volume, so
it's unlikely to be the case for any of the bug reporters so far.

> OK - so a too long winded way to say - I don't think the following patch
> matters as anything (including libvirt) could have changed the file's
> ownership and/or protections, but not updated the volume XML. The
> refresh updates the volume XML to match the file's protection and
> furthermore only matters in the current virFileRemove if the change is
> back to root:root and only because we compare vs. gete{uid|gid}().
> 

I don't follow this conclusion really... refreshVol syncs the XML/volume cache
with the current uid/gid/mode on disk, which is then passed down to
virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the
internal cache is synced, virFileRemove will setuid(cole) and successfully
remove the disk image. Without this change, libvirt would setuid(qemu) and
fail to remove the disk.

So I don't see how this patch is unneeded, or only works for root:root

- Cole

> John
> 
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 1d96618..ded54c9 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
>>  goto cleanup;
>>  }
>>  
>> +/* Need to ensure we are using up to date uid/gid for deletion */
>> +if (backend->refreshVol &&
>> +backend->refreshVol(obj->conn, pool, vol) < 0) {
>> +

Re: [libvirt] [PATCH 3/3] util: virfile: Only setuid for virFileRemove if on NFS

2016-03-09 Thread John Ferlan 


On 03/09/2016 12:39 PM, Cole Robinson wrote:
> NFS with root-squash is the only reason we need to do setuid/setgid
> crazyness in virFileRemove, so limit that behavior to the NFS case.
> ---
> I'm not sure though if NFS is the only case we care about this here,
> or if we want to conditionalize this path on NFS since that makes it
> more of a pain to test... It's not required to fix the initial bug
> 
>  src/util/virfile.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index cea2674..3d1b118 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t 
> mode,
>   * owned by the passed uid/gid pair. Needed for NFS with root-squash
>   */
>  static bool
> -virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
> +virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid)

You added a new parameter to document...

ACK with that adjustment.

John

More outloud typing follows...

>  {
>  /* If running unprivileged, setuid isn't going to work */
>  if (geteuid() != 0)
> @@ -2336,6 +2336,12 @@ virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
>  if (uid == geteuid() && gid == getegid())
>  return false;
>  
> +/* Only perform the setuid stuff for NFS, which is the only case
> +   that may actually need it. This can error, but just be safe and
> +   only check for a clear negative result. */
> +if (virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 0)
> +return false;
> +

So at this point we know we're root, the provided uid/gid isn't 0:0 and
it's not -1:-1.  So it's something else...

Now if the target isn't a volume on an NFS server, then since we're root
we can delete it regardless of what uid/gid is set on the file under the
premise that we changed it to something, but the XML wasn't updated.  At
some future point in time perhaps we can track uid/gid changes such as this.

>  return true;
>  }
>  
> @@ -2361,7 +2367,7 @@ virFileRemove(const char *path,
>  gid_t *groups;
>  int ngroups;
>  
> -if (!virFileRemoveNeedsSetuid(uid, gid)) {
> +if (!virFileRemoveNeedsSetuid(path, uid, gid)) {
>  if (virFileIsDir(path))
>  return rmdir(path);
>  else
> 

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

Re: [libvirt] [PATCH 2/3] util: virfile: Clarify setuid usage for virFileRemove

2016-03-09 Thread John Ferlan 


On 03/09/2016 12:38 PM, Cole Robinson wrote:
> Break these checks out into their own function, and clearly document
> each one. This shouldn't change behavior
> ---
>  src/util/virfile.c | 33 +++--
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f45e18f..cea2674 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2314,6 +2314,32 @@ virFileOpenAs(const char *path, int openflags, mode_t 
> mode,
>  }
>  
>  
> +/* virFileRemoveNeedsSetuid:
> + * @uid: file uid to check
> + * @gid: file gid to check
> + *
> + * Return true if we should use setuid/setgid before deleting a file
> + * owned by the passed uid/gid pair. Needed for NFS with root-squash
> + */
> +static bool
> +virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
> +{
> +/* If running unprivileged, setuid isn't going to work */
> +if (geteuid() != 0)
> +return false;
> +
> +/* uid/gid weren'd specified */

weren't

ACK - with the nit fixed... The rest is me typing out my thoughts...

John

> +if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
> +return false;

This "should" only happen as failure path of
virStorageBackendCreateExecCommand when uid/gid not provided in the XML.
The virStorageBackendCreateRaw failure path expects creation to have
occurred where virFileOpenAs would have set the owner/mode due to the
operation_flags setting.

> +
> +/* already running as proper uid/gid */
> +if (uid == geteuid() && gid == getegid())
> +return false;
> +

If the XML provided uid/gid and it's root - that's what we want

> +return true;
> +}
> +
> +
>  /* virFileRemove:
>   * @path: file to unlink or directory to remove
>   * @uid: uid that was used to create the file (not required)
> @@ -2335,12 +2361,7 @@ virFileRemove(const char *path,
>  gid_t *groups;
>  int ngroups;
>  
> -/* If not running as root or if a non explicit uid/gid was being used for
> - * the file/volume or the explicit uid/gid matches, then use unlink 
> directly
> - */
> -if ((geteuid() != 0) ||
> -((uid == (uid_t) -1) && (gid == (gid_t) -1)) ||
> -(uid == geteuid() && gid == getegid())) {
> +if (!virFileRemoveNeedsSetuid(uid, gid)) {
>  if (virFileIsDir(path))
>  return rmdir(path);
>  else
> 

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

Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion

2016-03-09 Thread John Ferlan 


On 03/09/2016 12:38 PM, Cole Robinson wrote:
> file volume deletion, via virFileRemove, has some logic that depends
> on uid/gid owner of the path. This info is cached in the volume XML.
> We need to be sure we are using up to date uid/gid before attempting
> to delete the volume, otherwise we can hit a case like this:
> 
> - test.img created with uid=root
> - VM starts up using test.img, owner changed to uid=qemu
> - test.img pool is refreshed, uid=qemu is cached in the volume XML
> - VM shuts down, volume owner changed to gid=root
> - vol-delete test.img thinks uid=qemu, virFileRemove does setuid(qemu),
>   fails to delete test.img since it is actually uid=root
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1289327
> ---
>  src/storage/storage_driver.c | 10 ++
>  1 file changed, 10 insertions(+)
> 

Coincidentally I was thinking about this one today... Still not
convinced this is the only "root" (ahem) cause...

For the startup/stop path, I assume your reference is the path through
virSecurityDACSetOwnershipInternal from virSecurityDACSetOwnership and
virSecurityDACRestoreFileLabelInternal...

But what if the mode, owner, group are provided in the XML when the
volume is created:

# cat vol.xml

  test.img
  
  
  10485760
  10485760
  
/home/vm-images/test.img


  0600
  107
  107
  system_u:object_r:unlabeled_t:s0

  



# virsh vol-create default vol.xml
Vol test.img created from vol.xml

# virsh vol-delete test.img default
error: Failed to delete vol test.img
error: cannot unlink file '/home/vm-images/test.img': Permission denied

#

Yes, I know the following two patches resolve this case...  What's
interesting is the extents we've gone to on creation to set things up,
but only a few bread crumbs are left for deletion. The assumption at
deletion being that no one changed anything from creation, but we see
that's not true.

I've been under the assumption that the owner gets set/changed during
virStorageBackendCreateRaw, virStorageFileBackendFileCreate, or
virStorageBackendCreateExecCommand.  The setting of the owner for the
CreateRaw path would occur because the flags had FORCE_OWNER and
FORCE_MODE set.  The setting of the owner for the CreateExecCommand in
the non POOL_NETFS path was because they were supplied in the XML and
not taken as the default from the running of the command to create the
file (eg qemu-img) and taking the default file/directory ownership.
Since we run as root, this can all happen without issues.

Then after any of the create processing happens, we can refresh the pool
which calls virStorageBackendUpdateVolTargetInfoFD and changes the
volume uid, gid, mode based on what the lstat(path, ) has found...
This perhaps helps in the event

But when we get to delete, we have no idea how things could have been
originally created or perhaps (likely) updated, so we only check what we
can... If we really wanted to be elaborate - save a provided uid, gid,
mode at creation... That would help at deletion to determine if
something changed.  Doesn't work well for existing files (daemon start,
restart, reload).  We'd need to save volume xml's somewhere. Probably
far more effort...

OK - so a too long winded way to say - I don't think the following patch
matters as anything (including libvirt) could have changed the file's
ownership and/or protections, but not updated the volume XML. The
refresh updates the volume XML to match the file's protection and
furthermore only matters in the current virFileRemove if the change is
back to root:root and only because we compare vs. gete{uid|gid}().

John

> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 1d96618..ded54c9 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
>  goto cleanup;
>  }
>  
> +/* Need to ensure we are using up to date uid/gid for deletion */
> +if (backend->refreshVol &&
> +backend->refreshVol(obj->conn, pool, vol) < 0) {
> +/* The file may have been removed behind libvirt's back. Don't
> +   error here, let the deletion fail or succeed instead */
> +VIR_INFO("Failed to refresh volume before deletion: %s",
> + virGetLastErrorMessage());
> +virResetLastError();
> +}
> +
>  if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0)
>  goto cleanup;
>  
> 

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

Re: [libvirt] [PATCH 2/2] HACK: qemu: aarch64: Use virtio-pci if user specifies PCI controller

2016-03-09 Thread Laine Stump 

On 03/09/2016 09:54 AM, Daniel P. Berrange wrote:

On Wed, Mar 09, 2016 at 01:40:36PM +0100, Andrea Bolognani wrote:

On Fri, 2016-03-04 at 17:05 -0500, Laine Stump wrote:

I'm not sure I fully understand all of the above, but I'll pitch
in with my own proposal regardless :)
  
First, we make sure that
  
 
  
is always added automatically to the domain XML when using the

mach-virt machine type. Then, if
  
 

 
  
is present as well we default to virtio-pci, otherwise we use

the current default of virtio-mmio. This should allow management
applications, based on knowledge about the guest OS, to easily
pick between the two address schemes.
  
Does this sound like a good idea?

... or a variation of that, anyway :-)
  
What I think: If there are *any* pci controllers *beyond* pcie-root, or

if there are any devices that already have a PCI address, then assign
PCI addresses, else use mmio.

This sounds reasonable.

However, I'm wondering if we shouldn't be even more explicit about
this... From libvirt's point of view we just need to agree on some
sort of "trigger" that causes it to allocate PCI addresses instead
of MMIO addresses, and for that purpose "any PCI controller or any
device with a PCI address" is perfectly fine; looking at that from
the point of view of an user, though? Not sure about it.

What about adding something like

   
 
   

to the domain XML? AFAICT libvirt doesn't have any element that
could be used for this purpose at the moment, but maybe having a
generic way to set domain-wide preferences could be useful in
other situations as well?

[snip]

Looking at this mail and laine's before I really get the impression we
are over-thinking this all. The automatic address assignment by libvirt
was written such that it matched what QEMU would have done, so that we
could introduce the concept of device addressing without breaking
existing apps which didn't supply addresses. The automatic addressing
logic certainly wasn't ever intended to be able to implement arbitrary
policies.

As a general rule libvirt has always aimed to stay out of the policy
business, and focus on providing the mechanism only. So when we start
talking about adding   to the XML this is a sure sign
that we've gone too far into trying to implement policy in libvirt.

>From the POV of being able to tell libvirt to assign PCI instead of
MMIO addresses for the virt machine, I think it suffices to have
libvirt accept a simple   element (ie with no
bus/slot/func filled in).


That's just a more limited case of the same type of feature creep though 
- you're specifying a preference for what type of address you want, just 
in a different place. (The odd thing is that we're adding it only to 
remedy what is apparently a temporary problem, and even then it's a 
problem that wouldn't exist if we just said simply this:  "If 
virtio-mmio is specified, then use virtio-mmio; If no address is 
specified, use pci". This puts the burden on people insisting on the old 
/ soon-to-be-deprecated (?) virtio-mmio address type to add a little 
something to the XML (and a very simple little something at that!). For 
a short time, this could cause problems for people who create new 
domains using qemu binaries that don't have a pci bus on aarch64/virt 
yet, but that will be easily solvable (by adding "type='virtio-mmio'/>", which is nearly as simple as typing "type='pci'/>").



A downside of setting your preference with  vs. 
having the preference in a separate element is that you can no longer 
cause a "re-addressing" of all the devices by simply removing all the 
 elements (and it's really that that got me thinking about 
adding hints/preferences in the  element). I don't know how 
important that is; I suppose when the producer of the XML is some other 
software it's a non-issue, when the producer is a human using virsh edit 
it would make life much simpler once in awhile (and the suggestion in 
the previous paragraph would eliminate that problem anyway).


So is there a specific reason why we need to keep virtio-mmio as the 
default, and require explicitly saying "address type='pci'" in order to 
get a PCI address?



If applictions care about assigning devices to specify PCI buses,
ie to distinguish between PCI & PCIe, or to pick a different bus
when there is one bus per NUMA node, then really it is time for
the application to start specifying the full  element
with all details, and not try to invent ever more complex policies
inside libvirt which will never deal with all application use cases.


First, I agree with your vigilance against unnecessary feature creep. 
Both because keeping things simple makes it easier to maintain and use, 
and also because it's very difficult (or even impossible) to change 
something once it's gone in, in the case that you have second thoughts.


But, expanding beyond just the aarch64/virt mmio vs. pci problem (which 
I had already done in this thread, and which is what got us into this 
"feature

[libvirt] [PATCH 3/3] util: virfile: Only setuid for virFileRemove if on NFS

2016-03-09 Thread Cole Robinson 
NFS with root-squash is the only reason we need to do setuid/setgid
crazyness in virFileRemove, so limit that behavior to the NFS case.
---
I'm not sure though if NFS is the only case we care about this here,
or if we want to conditionalize this path on NFS since that makes it
more of a pain to test... It's not required to fix the initial bug

 src/util/virfile.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index cea2674..3d1b118 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t 
mode,
  * owned by the passed uid/gid pair. Needed for NFS with root-squash
  */
 static bool
-virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
+virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid)
 {
 /* If running unprivileged, setuid isn't going to work */
 if (geteuid() != 0)
@@ -2336,6 +2336,12 @@ virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
 if (uid == geteuid() && gid == getegid())
 return false;
 
+/* Only perform the setuid stuff for NFS, which is the only case
+   that may actually need it. This can error, but just be safe and
+   only check for a clear negative result. */
+if (virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 0)
+return false;
+
 return true;
 }
 
@@ -2361,7 +2367,7 @@ virFileRemove(const char *path,
 gid_t *groups;
 int ngroups;
 
-if (!virFileRemoveNeedsSetuid(uid, gid)) {
+if (!virFileRemoveNeedsSetuid(path, uid, gid)) {
 if (virFileIsDir(path))
 return rmdir(path);
 else
-- 
2.5.0

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

[libvirt] [PATCH 1/3] storage: refresh volume before deletion

2016-03-09 Thread Cole Robinson 
file volume deletion, via virFileRemove, has some logic that depends
on uid/gid owner of the path. This info is cached in the volume XML.
We need to be sure we are using up to date uid/gid before attempting
to delete the volume, otherwise we can hit a case like this:

- test.img created with uid=root
- VM starts up using test.img, owner changed to uid=qemu
- test.img pool is refreshed, uid=qemu is cached in the volume XML
- VM shuts down, volume owner changed to gid=root
- vol-delete test.img thinks uid=qemu, virFileRemove does setuid(qemu),
  fails to delete test.img since it is actually uid=root

https://bugzilla.redhat.com/show_bug.cgi?id=1289327
---
 src/storage/storage_driver.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 1d96618..ded54c9 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
 goto cleanup;
 }
 
+/* Need to ensure we are using up to date uid/gid for deletion */
+if (backend->refreshVol &&
+backend->refreshVol(obj->conn, pool, vol) < 0) {
+/* The file may have been removed behind libvirt's back. Don't
+   error here, let the deletion fail or succeed instead */
+VIR_INFO("Failed to refresh volume before deletion: %s",
+ virGetLastErrorMessage());
+virResetLastError();
+}
+
 if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0)
 goto cleanup;
 
-- 
2.5.0

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

[libvirt] [PATCH 2/3] util: virfile: Clarify setuid usage for virFileRemove

2016-03-09 Thread Cole Robinson 
Break these checks out into their own function, and clearly document
each one. This shouldn't change behavior
---
 src/util/virfile.c | 33 +++--
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index f45e18f..cea2674 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2314,6 +2314,32 @@ virFileOpenAs(const char *path, int openflags, mode_t 
mode,
 }
 
 
+/* virFileRemoveNeedsSetuid:
+ * @uid: file uid to check
+ * @gid: file gid to check
+ *
+ * Return true if we should use setuid/setgid before deleting a file
+ * owned by the passed uid/gid pair. Needed for NFS with root-squash
+ */
+static bool
+virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
+{
+/* If running unprivileged, setuid isn't going to work */
+if (geteuid() != 0)
+return false;
+
+/* uid/gid weren'd specified */
+if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
+return false;
+
+/* already running as proper uid/gid */
+if (uid == geteuid() && gid == getegid())
+return false;
+
+return true;
+}
+
+
 /* virFileRemove:
  * @path: file to unlink or directory to remove
  * @uid: uid that was used to create the file (not required)
@@ -2335,12 +2361,7 @@ virFileRemove(const char *path,
 gid_t *groups;
 int ngroups;
 
-/* If not running as root or if a non explicit uid/gid was being used for
- * the file/volume or the explicit uid/gid matches, then use unlink 
directly
- */
-if ((geteuid() != 0) ||
-((uid == (uid_t) -1) && (gid == (gid_t) -1)) ||
-(uid == geteuid() && gid == getegid())) {
+if (!virFileRemoveNeedsSetuid(uid, gid)) {
 if (virFileIsDir(path))
 return rmdir(path);
 else
-- 
2.5.0

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

[libvirt] [PATCH 0/3] storage: fix file unlink with old uid cache

2016-03-09 Thread Cole Robinson 
There's been several reports of issues deleting storage volumes
after creating a VM with virt-manager/virt-install. These patches
fix the reports, see patch descriptions for details.

Patch #1 is the main fix IMO
Patch #2 is a related cleanup that clarifies some remove logic
Patch #3 is optional and may not be exhaustive, it's mostly for discussion


Pavel's original patches and discussions:

https://www.redhat.com/archives/libvir-list/2016-February/msg01224.html
https://bugzilla.redhat.com/show_bug.cgi?id=1289327
https://bugzilla.redhat.com/show_bug.cgi?id=1293804

Cole Robinson (3):
  storage: refresh volume before deletion
  util: virfile: Clarify setuid usage for virFileRemove
  util: virfile: Only setuid for virFileRemove if on NFS

 src/storage/storage_driver.c | 10 ++
 src/util/virfile.c   | 39 +--
 2 files changed, 43 insertions(+), 6 deletions(-)

-- 
2.5.0

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

Re: [libvirt] [PATCH] Report error when both live and offline flags are used for migration.

2016-03-09 Thread Nitesh Konkar 
Hello Jiri,

On Tue, Mar 8, 2016 at 8:31 PM, Jiri Denemark  wrote:

> On Thu, Mar 03, 2016 at 06:08:20 -0500, Nitesh Konkar wrote:
> > ---
> >  src/libvirt-domain.c | 27 +++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index 9491845..dc11945 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -3617,6 +3617,15 @@ virDomainMigrate(virDomainPtr domain,
> >   error);
> >
> > +if (flags & VIR_MIGRATE_OFFLINE) {
>
> I asked you to move the following if () {...} block...
>
>
Sorry, I missed this out. Will rectify it in the next patch.


> > +if (flags & VIR_MIGRATE_LIVE) {
> > +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> > +   _("Live and offline migration flags are "
> > + "mutually exclusive"));
> > +goto error;
> > +}
> > +}
> > +
> >  if (flags & VIR_MIGRATE_OFFLINE) {
>
> ... here.
>
> >  if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
> domain->conn,
> >
> VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
> >  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>
> I wanted to do that and push the patch, but I realized the patch is
> incomplete (it doesn't cover MigrateToURI* APIs) and there is even a
> better place to add these checks...
>
> Something like the following (untested):
>
>
But, changes in qemuMigrationBeginPhase and qemuMigrationPrepareAny  would
necessitate changes to all drivers domainMigrate* specific implementations
right ?
I feel what we have is better. What is your opinion on it ?  :-)


> diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
> index 64cbffa..36a939d 100644
> --- i/src/qemu/qemu_migration.c
> +++ w/src/qemu/qemu_migration.c
> @@ -3081,6 +3081,12 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>  goto cleanup;
>
>  if (flags & VIR_MIGRATE_OFFLINE) {
> +if (flags & VIR_MIGRATE_LIVE) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("live and offline migration flags are "
> + "mutually exclusive"));
> +goto cleanup;
> +}
>  if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
>   VIR_MIGRATE_NON_SHARED_INC)) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -3335,6 +3341,12 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>  virNWFilterReadLockFilterUpdates();
>
>  if (flags & VIR_MIGRATE_OFFLINE) {
> +if (flags & VIR_MIGRATE_LIVE) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("live and offline migration flags are "
> + "mutually exclusive"));
> +goto cleanup;
> +}
>  if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
>   VIR_MIGRATE_NON_SHARED_INC)) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>
Looking forward to your reply.

Warm Regards,
Nitesh Konkar.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] HACK: qemu: aarch64: Use virtio-pci if user specifies PCI controller

2016-03-09 Thread Daniel P. Berrange 
On Wed, Mar 09, 2016 at 01:40:36PM +0100, Andrea Bolognani wrote:
> On Fri, 2016-03-04 at 17:05 -0500, Laine Stump wrote:
> > > I'm not sure I fully understand all of the above, but I'll pitch
> > > in with my own proposal regardless :)
> > > 
> > > First, we make sure that
> > > 
> > >
> > > 
> > > is always added automatically to the domain XML when using the
> > > mach-virt machine type. Then, if
> > > 
> > >
> > >
> > > 
> > > is present as well we default to virtio-pci, otherwise we use
> > > the current default of virtio-mmio. This should allow management
> > > applications, based on knowledge about the guest OS, to easily
> > > pick between the two address schemes.
> > > 
> > > Does this sound like a good idea?
> > ... or a variation of that, anyway :-)
> > 
> > What I think: If there are *any* pci controllers *beyond* pcie-root, or 
> > if there are any devices that already have a PCI address, then assign 
> > PCI addresses, else use mmio.
> 
> This sounds reasonable.
> 
> However, I'm wondering if we shouldn't be even more explicit about
> this... From libvirt's point of view we just need to agree on some
> sort of "trigger" that causes it to allocate PCI addresses instead
> of MMIO addresses, and for that purpose "any PCI controller or any
> device with a PCI address" is perfectly fine; looking at that from
> the point of view of an user, though? Not sure about it.
> 
> What about adding something like
> 
>   
> 
>   
> 
> to the domain XML? AFAICT libvirt doesn't have any element that
> could be used for this purpose at the moment, but maybe having a
> generic way to set domain-wide preferences could be useful in
> other situations as well?

[snip]

Looking at this mail and laine's before I really get the impression we
are over-thinking this all. The automatic address assignment by libvirt
was written such that it matched what QEMU would have done, so that we
could introduce the concept of device addressing without breaking
existing apps which didn't supply addresses. The automatic addressing
logic certainly wasn't ever intended to be able to implement arbitrary
policies.

As a general rule libvirt has always aimed to stay out of the policy
business, and focus on providing the mechanism only. So when we start
talking about adding   to the XML this is a sure sign
that we've gone too far into trying to implement policy in libvirt.

>From the POV of being able to tell libvirt to assign PCI instead of
MMIO addresses for the virt machine, I think it suffices to have
libvirt accept a simple   element (ie with no
bus/slot/func filled in).

If applictions care about assigning devices to specify PCI buses,
ie to distinguish between PCI & PCIe, or to pick a different bus
when there is one bus per NUMA node, then really it is time for
the application to start specifying the full  element
with all details, and not try to invent ever more complex policies
inside libvirt which will never deal with all application use cases.

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

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

Re: [libvirt] [PATCH 2/2] HACK: qemu: aarch64: Use virtio-pci if user specifies PCI controller

2016-03-09 Thread Andrea Bolognani 
On Fri, 2016-03-04 at 17:05 -0500, Laine Stump wrote:
> > I'm not sure I fully understand all of the above, but I'll pitch
> > in with my own proposal regardless :)
> > 
> > First, we make sure that
> > 
> >
> > 
> > is always added automatically to the domain XML when using the
> > mach-virt machine type. Then, if
> > 
> >
> >
> > 
> > is present as well we default to virtio-pci, otherwise we use
> > the current default of virtio-mmio. This should allow management
> > applications, based on knowledge about the guest OS, to easily
> > pick between the two address schemes.
> > 
> > Does this sound like a good idea?
> ... or a variation of that, anyway :-)
> 
> What I think: If there are *any* pci controllers *beyond* pcie-root, or 
> if there are any devices that already have a PCI address, then assign 
> PCI addresses, else use mmio.

This sounds reasonable.

However, I'm wondering if we shouldn't be even more explicit about
this... From libvirt's point of view we just need to agree on some
sort of "trigger" that causes it to allocate PCI addresses instead
of MMIO addresses, and for that purpose "any PCI controller or any
device with a PCI address" is perfectly fine; looking at that from
the point of view of an user, though? Not sure about it.

What about adding something like

  

  

to the domain XML? AFAICT libvirt doesn't have any element that
could be used for this purpose at the moment, but maybe having a
generic way to set domain-wide preferences could be useful in
other situations as well?

> To make this more useful, we will want some enhancements:
> 
> 1) add a "hotpluggable" attribute to  for every device (this 
> will be a bit ugly, because there isn't a unified parser/formatter for 
> the  element :-( )
> 
> 2) add a "busHint" ("busType", "preferredBus", ??) attribute too, to 
> allow specifying pci vs pcie (is it worth adding "mmio" here? Seems it 
> will be deprecated before long anyway...). It will default to pcie 
> rather than pci on platforms that support it.
> 
> 3) change the auto-assign to pay attention to hotpluggable and 
> preferredBus (damn! I need to think of a better name!)
> 
> This way someone can define a new aarch64 domain and just toss a bunch 
> of pcie-root-ports into it:
> 
>
>
>
>
>...
> 
> then add a bunch of devices. The pcie-root-ports will be auto-assigned 
> to ports on pcie.0, and the devices will be auto-assigned to root-ports. 
> (alternately, they could toss in a bunch of 
> pcie-downstream-switch-ports. These would trigger an auto-add of a 
> pcie-upstream-switch-port, which would trigger an auto-add of a 
> pcie-root-port, and those would all be connected).
> 
> (Ooh! Or instead of hotpluggable and preferredBus in the devices' 
> , just "preferredConnection" which would be exactly the model of 
> pci controller preferred. So you'd do something like this:
> 
> 
>   
>   ...
> 
> 
> This would look for an existing pcie-switch-downstream-port. If it 
> couldn't find one available, it would add one (and all the underlying 
> controllers necessary).

I'm assuming these examples refer to the case of a guest starting
up and not device hotplug, because neither pcie-root-port nor
pcie-switch-{up,down}stream-port can be added at runtime.

Looks to me like having both 'hotpluggable' and 'preferredBus'
would be more user-friendly, ie. looking at the XML tells you
whether you'll be able to hot-unplug a device without having to
remember what a pcie-switch-downstream-port is.

This still doesn't help when it comes to providing PCI or PCIe
slots suitable for hotplugging: if you want to have three PCIe
devices at startup and still be able to hotplug up to two PCIe
devices later on, my understanding is that you would need to
have something like

  
  

or

  
  
  

Could this make use of a preferences mechanism such as the one
hinted at above? For example

  


  

would generate either of the above PCIe topology, while

  


  

would add a dmi-to-pci-bridge and a pci-bridge in order to
present the hotplug-capable PCI slots to the guest.

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 v2 1/2] qemu: vcpupin: Extract live vcpupin setting into a separate function

2016-03-09 Thread Peter Krempa 
The function was now beyond maintainability.
---
 src/qemu/qemu_driver.c | 134 -
 1 file changed, 78 insertions(+), 56 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 72ed3c0..3d45ce3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4973,6 +4973,82 @@ qemuDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus)


 static int
+qemuDomainPinVcpuLive(virDomainObjPtr vm,
+  virDomainDefPtr def,
+  int vcpu,
+  virQEMUDriverPtr driver,
+  virQEMUDriverConfigPtr cfg,
+  virBitmapPtr cpumap)
+{
+virDomainVcpuInfoPtr vcpuinfo;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virCgroupPtr cgroup_vcpu = NULL;
+char *str = NULL;
+virObjectEventPtr event = NULL;
+char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = "";
+virTypedParameterPtr eventParams = NULL;
+int eventNparams = 0;
+int eventMaxparams = 0;
+int ret = -1;
+
+if (!qemuDomainHasVcpuPids(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("cpu affinity is not supported"));
+goto cleanup;
+}
+
+if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu))) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("vcpu %d is out of range of live cpu count %d"),
+   vcpu, virDomainDefGetVcpusMax(def));
+goto cleanup;
+}
+
+if (vcpuinfo->online) {
+/* Configure the corresponding cpuset cgroup before set affinity. */
+if (virCgroupHasController(priv->cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
+if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu,
+   false, _vcpu) < 0)
+goto cleanup;
+if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0)
+goto cleanup;
+}
+
+if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) < 0)
+goto cleanup;
+}
+
+virBitmapFree(vcpuinfo->cpumask);
+vcpuinfo->cpumask = cpumap;
+cpumap = NULL;
+
+if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 
0)
+goto cleanup;
+
+if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH,
+ VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) {
+goto cleanup;
+}
+
+str = virBitmapFormat(vcpuinfo->cpumask);
+if (virTypedParamsAddString(, ,
+, paramField, str) < 0)
+goto cleanup;
+
+event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);
+
+ret = 0;
+
+ cleanup:
+virBitmapFree(cpumap);
+virCgroupFree(_vcpu);
+VIR_FREE(str);
+qemuDomainEventQueue(driver, event);
+return ret;
+}
+
+
+static int
 qemuDomainPinVcpuFlags(virDomainPtr dom,
unsigned int vcpu,
unsigned char *cpumap,
@@ -4984,21 +5060,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virDomainObjPtr vm;
 virDomainDefPtr def;
 virDomainDefPtr persistentDef;
-virCgroupPtr cgroup_vcpu = NULL;
 int ret = -1;
-qemuDomainObjPrivatePtr priv;
 virBitmapPtr pcpumap = NULL;
 virBitmapPtr pcpumaplive = NULL;
 virBitmapPtr pcpumappersist = NULL;
-virDomainVcpuInfoPtr vcpuinfolive = NULL;
 virDomainVcpuInfoPtr vcpuinfopersist = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
-virObjectEventPtr event = NULL;
-char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = "";
-char *str = NULL;
-virTypedParameterPtr eventParams = NULL;
-int eventNparams = 0;
-int eventMaxparams = 0;

 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -5017,15 +5084,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 if (virDomainObjGetDefs(vm, flags, , ) < 0)
 goto endjob;

-priv = vm->privateData;
-
-if (def && !(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("vcpu %d is out of range of live cpu count %d"),
-   vcpu, virDomainDefGetVcpus(def));
-goto endjob;
-}
-
 if (persistentDef &&
 !(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) {
 virReportError(VIR_ERR_INVALID_ARG,
@@ -5048,44 +5106,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 goto endjob;

 if (def) {
-if (!qemuDomainHasVcpuPids(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   "%s", _("cpu affinity is not supported"));
+if (qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumaplive) < 
0) {
+pcpumaplive = NULL;
 goto endjob;
 }

-if (vcpuinfolive->online) {
-/* Configure the corresponding cpuset cgroup before set affinity. 
*/
-if

[libvirt] [PATCH v2 2/2] qemu: Refactor bitmap handling in qemuDomainPinVcpuFlags

2016-03-09 Thread Peter Krempa 
Now that the function was extracted we can get rid of some temp
variables. Additionally formatting of the bitmap string for the event
code should be checked.
---
 src/qemu/qemu_driver.c | 41 +
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3d45ce3..e9d1d39 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4980,6 +4980,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
   virQEMUDriverConfigPtr cfg,
   virBitmapPtr cpumap)
 {
+virBitmapPtr tmpmap = NULL;
 virDomainVcpuInfoPtr vcpuinfo;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virCgroupPtr cgroup_vcpu = NULL;
@@ -5004,6 +5005,12 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 goto cleanup;
 }

+if (!(tmpmap = virBitmapNewCopy(cpumap)))
+goto cleanup;
+
+if (!(str = virBitmapFormat(cpumap)))
+goto cleanup;
+
 if (vcpuinfo->online) {
 /* Configure the corresponding cpuset cgroup before set affinity. */
 if (virCgroupHasController(priv->cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
@@ -5019,8 +5026,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 }

 virBitmapFree(vcpuinfo->cpumask);
-vcpuinfo->cpumask = cpumap;
-cpumap = NULL;
+vcpuinfo->cpumask = tmpmap;
+tmpmap = NULL;

 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 
0)
 goto cleanup;
@@ -5030,7 +5037,6 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 goto cleanup;
 }

-str = virBitmapFormat(vcpuinfo->cpumask);
 if (virTypedParamsAddString(, ,
 , paramField, str) < 0)
 goto cleanup;
@@ -5040,7 +5046,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 ret = 0;

  cleanup:
-virBitmapFree(cpumap);
+virBitmapFree(tmpmap);
 virCgroupFree(_vcpu);
 VIR_FREE(str);
 qemuDomainEventQueue(driver, event);
@@ -5062,9 +5068,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virDomainDefPtr persistentDef;
 int ret = -1;
 virBitmapPtr pcpumap = NULL;
-virBitmapPtr pcpumaplive = NULL;
-virBitmapPtr pcpumappersist = NULL;
-virDomainVcpuInfoPtr vcpuinfopersist = NULL;
+virDomainVcpuInfoPtr vcpuinfo = NULL;
 virQEMUDriverConfigPtr cfg = NULL;

 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -5085,7 +5089,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 goto endjob;

 if (persistentDef &&
-!(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) {
+!(vcpuinfo = virDomainDefGetVcpu(persistentDef, vcpu))) {
 virReportError(VIR_ERR_INVALID_ARG,
_("vcpu %d is out of range of persistent cpu count %d"),
vcpu, virDomainDefGetVcpus(persistentDef));
@@ -5101,23 +5105,14 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 goto endjob;
 }

-if ((def && !(pcpumaplive = virBitmapNewCopy(pcpumap))) ||
-(persistentDef && !(pcpumappersist = virBitmapNewCopy(pcpumap
+if (def &&
+qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap) < 0)
 goto endjob;

-if (def) {
-if (qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumaplive) < 
0) {
-pcpumaplive = NULL;
-goto endjob;
-}
-
-pcpumaplive = NULL;
-}
-
 if (persistentDef) {
-virBitmapFree(vcpuinfopersist->cpumask);
-vcpuinfopersist->cpumask = pcpumappersist;
-pcpumappersist = NULL;
+virBitmapFree(vcpuinfo->cpumask);
+vcpuinfo->cpumask = pcpumap;
+pcpumap = NULL;

 ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef);
 goto endjob;
@@ -5131,8 +5126,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
  cleanup:
 virDomainObjEndAPI();
 virBitmapFree(pcpumap);
-virBitmapFree(pcpumaplive);
-virBitmapFree(pcpumappersist);
 virObjectUnref(cfg);
 return ret;
 }
-- 
2.6.2

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

[libvirt] [PATCH v2 0/2] vcpu info refactors - part 3b

2016-03-09 Thread Peter Krempa 
Peter Krempa (2):
  qemu: vcpupin: Extract live vcpupin setting into a separate function
  qemu: Refactor bitmap handling in qemuDomainPinVcpuFlags

 src/qemu/qemu_driver.c | 163 +++--
 1 file changed, 89 insertions(+), 74 deletions(-)

-- 
2.6.2

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

Re: [libvirt] [PATCH 6/9] conf: extract ignoring of inactive vcpu pinning information

2016-03-09 Thread Peter Krempa 
On Fri, Mar 04, 2016 at 07:30:39 -0500, John Ferlan wrote:
> 
> 
> On 02/24/2016 09:22 AM, Peter Krempa wrote:
> > Introduce VIR_DOMAIN_DEF_FEATURE_OFFLINE_CPUPIN domain feature flag
> 
> Should it be VCPUPIN ?

Yep.

> 
> > whcih will allow to skip ignoring of the pinning information for
> > hypervisor drivers which will want to implement forward-pinning of
> > vcpus.
> > ---
> >  src/conf/domain_conf.c | 28 +++-
> >  src/conf/domain_conf.h |  1 +
> >  2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 101fae2..4220448 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -4215,6 +4215,25 @@ 
> > virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
> >  }
> > 
> > 
> 
> A little intro would be nice...
> 
> > +static void
> > +virDomainDefRemoveOfflineVcpuPin(virDomainDefPtr def)
> > +{
> > +size_t i;
> > +virDomainVcpuInfoPtr vcpu;
> > +
> > +for (i = 0; i < virDomainDefGetVcpusMax(def); i++) {
> > +vcpu = virDomainDefGetVcpu(def, i);
> > +
> > +if (!vcpu->online && vcpu->cpumask) {
> > +virBitmapFree(vcpu->cpumask);
> > +vcpu->cpumask = NULL;
> > +
> > +VIR_WARN("Ignoring unsupported vcpupin for offline vcpu 
> > '%zu'", i);
> 
> Is/was this for debugging?  Do we really want to WARN or just go with INFO?

No, this is basically stolen from the original place where this
operation was done just with a more specific message. I did not attempt
to do anything different.

I'll push this with the message and we can get rid of it with a patch
specifically describing the semantic change.

Peter


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

Re: [libvirt] [python PATCH] Add support for JOB_COMPLETED event

2016-03-09 Thread Pavel Hrdina 
On Tue, Mar 08, 2016 at 04:49:11PM +0100, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark 
> ---
>  examples/event-test.py |  3 ++
>  libvirt-override-virConnect.py |  9 ++
>  libvirt-override.c | 64 
> ++
>  3 files changed, 76 insertions(+)

ACK

Pavel

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

Re: [libvirt] [PATCH 0/9] vcpu info refactors - part 3b

2016-03-09 Thread Peter Krempa 
On Fri, Mar 04, 2016 at 07:31:42 -0500, John Ferlan wrote:
> 
> 
> On 02/24/2016 09:22 AM, Peter Krempa wrote:
> > Yet another continuation of the saga. In this episode we add pinning for
> > inactive cpus.
> > 
> 
> ACK series modulo a couple of nits pointed out - most importantly is to
> combine patch 8 and 9.

I can combine them, but we usualy prefer code moves being separated from
refactors. But it's really your call in this case.

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 8/9] qemu: vcpupin: Extract live vcpupin setting into a separate function

2016-03-09 Thread Peter Krempa 
On Fri, Mar 04, 2016 at 07:31:23 -0500, John Ferlan wrote:
> 
> 
> On 02/24/2016 09:22 AM, Peter Krempa wrote:
> > The function was now beyond maintainability.
> > ---
> >  src/qemu/qemu_driver.c | 134 
> > -
> >  1 file changed, 77 insertions(+), 57 deletions(-)
> > 
> 
> hmmm... some comments below written before I viewed the patch 9
> pcpumaplive changes...  Perhaps there should be a merging of the two or
> even reverse the order with obvious logic adjustments... Without doing
> so leaves a 1-patch window with an issue.
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index bfabc53..cab69a5 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4967,6 +4967,82 @@ qemuDomainSetVcpus(virDomainPtr dom, unsigned int 
> > nvcpus)

[...]

> > +
> > +if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) 
> > < 0)
> > +goto cleanup;
> > +}
> > +
> > +virBitmapFree(vcpuinfo->cpumask);
> > +vcpuinfo->cpumask = cpumap;
> > +cpumap = NULL;
> 
> Because there's some goto cleanup's after this, I think cpumap should be
> passed by reference and not value.

No, see the explanation below ...

> > +
> > +if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, 
> > driver->caps) < 0)
> > +goto cleanup;
> > +
> > +if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH,
> > + VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) {
> > +goto cleanup;
> > +}
> > +
> > +str = virBitmapFormat(vcpuinfo->cpumask);
> > +if (virTypedParamsAddString(, ,
> > +, paramField, str) < 0)
> > +goto cleanup;
> > +
> > +event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);
> 
> Are there any timing or locking consequences of this being called before
> qemuDomainObjEndJob and/or virDomainObjEndAPI (I don't think so, but
> typing what my eyes see as 'change'...)
> 
> > +
> > +ret = 0;
> > +
> > + cleanup:
> > +virBitmapFree(cpumap);
> 
> This is duplicated in the caller (pcpumaplive) and this function doesn't
> own this, so the caller should be left to handle.

No, the caller needs to be fixed to clear the pointer, since this
function consumes the pointer regardless of the outcome.

[...]

> > @@ -5011,15 +5078,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> >  if (virDomainObjGetDefs(vm, flags, , ) < 0)
> >  goto endjob;
> > 
> > -priv = vm->privateData;
> > -
> > -if (def && !(vcpuinfolive = virDomainDefGetVcpu(def, vcpu))) {
> > -virReportError(VIR_ERR_INVALID_ARG,
> > -   _("vcpu %d is out of range of live cpu count %d"),
> > -   vcpu, virDomainDefGetVcpus(def));
> > -goto endjob;
> > -}
> > -
> >  if (persistentDef &&
> >  !(vcpuinfopersist = virDomainDefGetVcpu(persistentDef, vcpu))) {
> >  virReportError(VIR_ERR_INVALID_ARG,
> 
> This change in flow means persistent is checked before live.
> Theoretically, this hunk could move inside the if (persistentDef) below,
> unless of course it's important to error out before the live def would
> be handled. Although could it even be possible to have a vcpu be invalid
> in persistent, but valid in live?
> 
> > @@ -5042,44 +5100,10 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> >  goto endjob;
> > 
> >  if (def) {
> > -if (!qemuDomainHasVcpuPids(vm)) {
> > -virReportError(VIR_ERR_OPERATION_INVALID,
> > -   "%s", _("cpu affinity is not supported"));
> > +if (qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumaplive) 
> > < 0)
> 
> Why not just put:
> 
> if (!def)
> return 0;
> 
> in qemuDomainPinVcpuLive and then just call it...

I think it makes it less obvious. The function is designed to set the
desired pinning and not to decide when we should set it.

> 
> >  goto endjob;
> > -}
> > 
> > -if (vcpuinfolive->online) {
> > -/* Configure the corresponding cpuset cgroup before set 
> > affinity. */
> > -if (virCgroupHasController(priv->cgroup, 
> > VIR_CGROUP_CONTROLLER_CPUSET)) {
> > -if (virCgroupNewThread(priv->cgroup, 
> > VIR_CGROUP_THREAD_VCPU, vcpu,
> > -   false, _vcpu) < 0)
> > -goto endjob;
> > -if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, pcpumap) < 0)
> > -goto endjob;
> > -}
> > -
> > -if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), 
> > pcpumap) < 0)
> > -goto endjob;
> > -}
> > -
> > -virBitmapFree(vcpuinfolive->cpumask);
> > -vcpuinfolive->cpumask = pcpumaplive;
> >  pcpumaplive = NULL;
> 
> Note how you set pcpumaplive to NULL only on success; however, cleanup
> after setting into vcpuinfo->vcpumask means a success to