4:29 p.m.
On 12/01/2010 10:26 AM, Daniel P. Berrange wrote:
Introduces a set of generic objects which are to be used in
building RPC servers/clients based on XDR.
- virNetMessageHeader - standardize the XDR format for any
RPC program. Copied from remote protocol for back compat
- virNetMessage - Provides a buffer for (de-)serializing
messages, and a copy of the decoded virNetMessageHeader.
Provides APIs for encoding/decoding message headers and
payloads, thus isolating all the XDR api calls in one
file. Callers no longer need to use XDR themselves.
- virNetSocket - a wrapper around a socket file descriptor,
to simplify creation of new sockets, both for clients and
services. Encapsulates all the hairy getaddrinfo code
and sockaddr manipulation. Will eventually include
transparent support for TLS and SASL encoding of data
- virNetTLSContext - encapsulates the credentials required
to setup TLS sessions. eg the set of x509 certificates
and keys, optional DH parameters and x509 DName whitelist
Provides APIs for easily validating certificates from a
TLS session
- virNetTLSSession - encapsulates the TLS session handling,
so that callers no longer have a direct dependancy on
gnutls. This will facilitate adding alternate TLS impls.
Makes the read/write TLS functions work with same
semantics as the native socket read/write functions. ie
they set errno, instead of a gnutls specific error code.
This code is taken from either the daemon/libvirtd.c,
daemon/dispatch.c or src/remote/remote_driver.c files,
which all duplicated alot of functionality.
Whether or not you decide to break this into multiple patches (one per
API addition) or keep it as one (since this is all pretty much pure
addition), here's my first round of review:
@@ -1098,6 +1116,28 @@ libvirt_qemu_la_CFLAGS = $(AM_CFLAGS)
libvirt_qemu_la_LIBADD = libvirt.la $(CYGWIN_EXTRA_LIBADD)
EXTRA_DIST += $(LIBVIRT_QEMU_SYMBOL_FILE)
+
+noinst_LTLIBRARIES += libvirt-net-rpc.la
+
+libvirt_net_rpc_la_SOURCES = \
+ ../daemon/event.c \
+ rpc/virnetprotocol.h rpc/virnetprotocol.c \
+ rpc/virnetmessage.h rpc/virnetmessage.c \
+ rpc/virnettlscontext.h rpc/virnettlscontext.c \
+ rpc/virnetsocket.h rpc/virnetsocket.c
+libvirt_net_rpc_la_CFLAGS = \
+ $(GNUTLS_CFLAGS) \
+ $(SASL_CFLAGS) \
+ $(AM_CFLAGS)
If my cygwin patch is approved first, this will need $(XDR_CFLAGS).
https://www.redhat.com/archives/libvir-list/2010-December/msg00404.html
+++ b/src/rpc/virnetmessage.c
@@ -0,0 +1,215 @@
+#include <config.h>
+
+#include "virnetmessage.h"
+
+#include "virterror_internal.h"
+#include "logging.h"
+
+#define virNetError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_RPC, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
+
Does this need #define VIR_FROM_THIS VIR_FROM_RPC?
Should virNetError be added to the list of error() functions in cfg.mk?
+++ b/src/rpc/virnetmessage.h
@@ -0,0 +1,31 @@
+#ifndef __VIR_NET_MESSAGE_H__
+#define __VIR_NET_MESSAGE_H__
+
+#include "virnetprotocol.h"
+
+typedef struct virNetMessageHeader *virNetMessageHeaderPtr;
+
+typedef struct _virNetMessage virNetMessage;
+typedef virNetMessage *virNetMessagePtr;
+
+struct _virNetMessage {
+ char buffer[VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX];
+ unsigned int bufferLength;
+ unsigned int bufferOffset;
+
+ virNetMessageHeader header;
+};
That's a big struct; where lots of space will typically be unused. It
should never be stack-allocated. Should we rearrange the fields to put
buffer last, and then use variable-sized array (or something similar) to
trim the size down to what is needed, rather than always allocating the
largest possible message?
+
+static int virNetSocketForkDaemon(const char *binary)
+{
+ const char *const daemonargs[] = { binary, "--timeout=30", NULL };
+ pid_t pid;
+
+ if (virExecDaemonize(daemonargs, NULL, NULL,
+ &pid, -1, NULL, NULL,
+ VIR_EXEC_CLEAR_CAPS,
+ NULL, NULL, NULL) < 0)
+ return -1;
Should this use virCommand instead?
+
+#if 0
+ /* There is no meaningful local addr for UNIX sockets,
+ * and getsockname() returns something with AF_INET
+ * in sa_family when run against AF_JUNIX sockets !
Is it worth copying this typo around?
+
+int virNetSocketNewConnectSSH(const char *nodename,
+ const char *service,
+ const char *binary,
+ const char *username,
+ bool noTTY,
+ const char *netcat,
+ const char *path,
+ virNetSocketPtr *retsock)
+{
+ const char **cmdargv = NULL;
+ int ncmdargv;
+
+ *retsock = NULL;
+
+ ncmdargv = 6;
+ if (username) ncmdargv += 2; /* For -l username */
+ if (noTTY) ncmdargv += 5; /* For -T -o BatchMode=yes -e none */
+ if (service) ncmdargv += 2; /* For -p port */
Wow - this would be much easier to write with virCommand.
+
+int virNetSocketNewConnectCommand(const char **cmdargv,
+ const char **cmdenv,
+ virNetSocketPtr *retsock)
+{
Which means this should take a virCommandPtr, rather than a cmdargv and
cmdenv.
+void virNetSocketFree(virNetSocketPtr sock)
+{
+ VIR_DEBUG("sock=%p", sock);
+
+ if (!sock)
+ return;
+
+ if (sock->watch) {
+ virEventRemoveHandle(sock->watch);
+ sock->watch = -1;
+ }
Should this be sock->watch = 0; after removing the handle, or should the
condition be if (sock->watch > 0)?
+
+int virNetSocketListen(virNetSocketPtr sock)
+{
+ if (listen(sock->fd, 30) < 0) {
Why 30 and not some other magic number? Why not let the caller pass in
their desired backlog parameter?
+int virNetSocketAddIOCallback(virNetSocketPtr sock,
+ int events,
+ virNetSocketIOFunc func,
+ void *opaque)
+{
+ if (sock->watch) {
Again, should this be sock->watch > 0?
+
+/* XXX bad */
+int virNetSocketFD(virNetSocketPtr sock);
Is your intent to remove just this one function, to force all fd usage
to go through the wrappers instead?
+
+#endif /* __VIR_NET_SOCKET_H__ */
Should you be marking some parameters ATTRIBUTE_NONNULL in this header?
+
+ssize_t virNetTLSSessionWrite(virNetTLSSessionPtr sess,
+ const char *buf, size_t len)
+{
+ int ret;
s/int/ssize_t/
+ssize_t virNetTLSSessionRead(virNetTLSSessionPtr sess,
+ char *buf, size_t len)
+{
+ int ret;
s/int/ssize_t/
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org