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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list