[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Wed, Mar 16, 2016 at 12:47:53PM +0100, Erik Skultety wrote:
On 10/03/16 05:54, Martin Kletzander wrote:
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 <mkletzan redhat com>
---
 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(-)

...
+
+/**
+ * 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.
+ */

^^ This isn't quite describing what the function does, is it? Power of
copy-paste-edit I guess :).


It stayed rotting in one of my branches for a while and when I needed to
send it, I forgot that it's not fixed completely.  Not even Jan found
that out, so at least now I know somebody reads that ;)  Thanks for
spotting it.

+virAdmServerPtr
+virAdmConnectLookupServer(virAdmConnectPtr conn,
+                          const char *name,
+                          unsigned int flags)
+{
+    virAdmServerPtr ret = NULL;
+
+    VIR_DEBUG("conn=%p, name=%s, flags=%x", conn, NULLSTR(name), flags);
+    virResetLastError();
+
+    virCheckAdmConnectGoto(conn, cleanup);
+    virCheckNonNullArgGoto(name, cleanup);
+    virCheckFlagsGoto(0, cleanup);
+
+    ret = remoteAdminConnectLookupServer(conn, name, flags);
+ cleanup:
+    if (!ret)
+        virDispatchError(NULL);
+    return ret;
+}
diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms
index f22137bcc6af..268f1e6f2fdd 100644
--- a/src/libvirt_admin_private.syms
+++ b/src/libvirt_admin_private.syms
@@ -9,6 +9,8 @@
 xdr_admin_connect_get_lib_version_ret;
 xdr_admin_connect_list_servers_args;
 xdr_admin_connect_list_servers_ret;
+xdr_admin_connect_lookup_server_args;
+xdr_admin_connect_lookup_server_ret;
 xdr_admin_connect_open_args;

 # datatypes.h
diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms
index 52ff2dc1b411..58d085e1f118 100644
--- a/src/libvirt_admin_public.syms
+++ b/src/libvirt_admin_public.syms
@@ -24,4 +24,5 @@ LIBVIRT_ADMIN_1.3.0 {
         virAdmConnectListServers;
         virAdmServerGetName;
         virAdmServerFree;
+        virAdmConnectLookupServer;
 };

Just a reminder, not that we should do something about it right now, but
let's not forget about the fact, that all the symbols are introduced one
after another within different releases. It's been already 2 releases,
since we pushed first admin-related code. So, before we enable admin,
the library version will have to be changed from 1.3.0 to the
appropriate version of the exact release in which we finally enable it.



ACK with that commentary mentioned above changed.


Yes, that's another thing I'm keeping in my mind which I hope I don't
forget.  However, nobody knows if the same things as with the
description is not going to happen.  Thanks for the review.

Erik

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]