[libvirt] [PATCH 4/8] Introduce generic objects for building XDR RPC servers/clients
Eric Blake
eblake at redhat.com
Thu Dec 9 22:29:20 UTC 2010
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101209/1d360a52/attachment-0001.sig>
More information about the libvir-list
mailing list