[libvirt] [PATCH 4/8] Introduce generic objects for building XDR RPC servers/clients

[Daniel P. Berrange]

On Thu, Dec 09, 2010 at 03:29:20PM -0700, Eric Blake wrote:
> > @@ -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

That patch isn't in yet I see.

> > +++ 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?

Only if it uses virReportSystemError or virReportOOMError(),
which it does now.

> Should virNetError be added to the list of error() functions in cfg.mk?

Probably, I forgot to make this change in the new series

> > +++ 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?

This struct is always allocated on the heap. Making it dynamically
sized is tricky. When serializing a message to XDR form, we need to
allocate enough data before serialization, but we've no idea how
much is big enough, so we have to allocate the full size. We could
re-alloc smaller afterwards I guess.  When de-serializing we could
do an incremental allocation, because we read the header and get
the total size and then read the payload.

> > +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?

This was just copying existing code, but I've changed it
now to use virCommand...not actually tested my new code
yet though.

> > +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)?

Yep, the latter.

> > +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?

The listen backlog is pretty arbitrary. None of our use cases have
such high connection rates that we'd really need to tune this any
higher, so I think the arbitrary limit is fine.

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

I had to keep this for the DTrace APIs and one other place in
the client code which needs an FD to poll() on. Ideally we
could address the latter, but the former is always going ot
be required. So I removed the XXX.

> 
> > +
> > +#endif /* __VIR_NET_SOCKET_H__ */
> 
> Should you be marking some parameters ATTRIBUTE_NONNULL in this header?

Possibly, but I've not considered it yet.

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

Done this and many many more

Daniel