Re: [libvirt] [PATCH 02/10] Provide a simple object for encoding/decoding RPC messages
On 06/22/2011 09:33 AM, Daniel P. Berrange wrote:
This provides a new struct that contains a buffer for the RPC
message header+payload, as well as a decoded copy of the message
header. There is an API for applying a XDR encoding & decoding
of the message headers and payloads. There are also APIs for
maintaining a simple FIFO queue of message instances.
Expected usage scenarios are:
To send a message
msg = virNetMessageNew()
...fill in msg->header fields..
virNetMessageEncodeHeader(msg)
...loook at msg->header fields to determine payload filter
virNetMessageEncodePayload(msg, xdrfilter, data)
...send msg->bufferLength worth of data from buffer
To receive a message
msg = virNetMessageNew()
...read VIR_NET_MESSAGE_LEN_MAX of data into buffer
virNetMessageDecodeLength(msg)
...read msg->bufferLength-msg->bufferOffset of data into buffer
virNetMessageDecodeHeader(msg)
...look at msg->header fields to determine payload filter
virNetMessageDecodePayload(msg, xdrfilter, data)
...run payload processor
'make check' fails:
virnetmessagetest-virnetmessagetest.o: In function
`testMessageHeaderEncode':
/home/remote/eblake/libvirt/tests/virnetmessagetest.c:57: undefined
reference to `virNetMessageEncodeHeader'
But that was easy enough to fix.
+void virNetMessageSaveError(virNetMessageErrorPtr rerr)
+{
+ /* This func may be called several times & the first
+ * error is the one we want because we don't want
+ * cleanup code overwriting the first one.
+ */
+ if (rerr->code != VIR_ERR_OK)
+ return;
+
+ virErrorPtr verr = virGetLastError();
+ if (verr) {
+ rerr->code = verr->code;
+ rerr->domain = verr->domain;
+ rerr->message = verr->message ? malloc(sizeof(char*)) : NULL;
Should we really be using raw malloc here, or is it any better to use
VIR_ALLOC? (Several times in this function).
+++ b/src/rpc/virnetmessage.h
@@ -0,0 +1,82 @@
+/*
+
+#ifndef __VIR_NET_MESSAGE_H__
+# define __VIR_NET_MESSAGE_H__
+
+# include <stdbool.h>
Is this include still necessary?
+int virtTestDifferenceBin(FILE *stream,
+ const char *expect,
+ const char *actual,
+ size_t length)
+{
+ size_t start = 0, end = length;
+ ssize_t i;
+
+ if (!virTestGetDebug())
+ return 0;
+
+ if (virTestGetDebug() < 2) {
+ /* Skip to first character where they differ */
+ for (i = 0 ; i < length ; i++) {
+ if (expect[i] != actual[i]) {
+ start = i;
+ break;
+ }
+ }
+
+ /* Work backwards to last character where they differ */
+ for (i = (length -1) ; i >= 0 ; i--) {
+ if (expect[i] != actual[i]) {
+ end = i;
+ break;
+ }
+ }
+ }
+ /* Round to nearest boundary of 4 */
+ start -= (start % 4);
+ end += 4 - (end % 4);
This could make end go beyond length, if the difference was in the last
byte or two on an unaligned length. And if expect or actual are also
unaligned, then this means that reading beyond bounds could segfault (at
any rate, valgrind will certainly call you on it).
ACK with this squashed in:
diff --git i/tests/Makefile.am w/tests/Makefile.am
index 2b1aa6b..4c3e51b 100644
--- i/tests/Makefile.am
+++ w/tests/Makefile.am
@@ -404,7 +404,7 @@ commandhelper_LDADD = $(LDADDS)
virnetmessagetest_SOURCES = \
virnetmessagetest.c testutils.h testutils.c
virnetmessagetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
-virnetmessagetest_LDADD = $(LDADDS)
+virnetmessagetest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS)
seclabeltest_SOURCES = \
diff --git i/tests/testutils.c w/tests/testutils.c
index d87347d..27a5435 100644
--- i/tests/testutils.c
+++ w/tests/testutils.c
@@ -406,9 +406,11 @@ int virtTestDifferenceBin(FILE *stream,
}
}
}
- /* Round to nearest boundary of 4 */
+ /* Round to nearest boundary of 4, except that last word can be
short */
start -= (start % 4);
end += 4 - (end % 4);
+ if (end >= length)
+ end = length - 1;
/* Show the trimmed differences */
fprintf(stream, "\nExpect [ Region %d-%d", (int)start, (int)end);
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)