[libvirt] [PATCH 02/10] Provide a simple object for encoding/decoding RPC messages

[Eric Blake]

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 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/20110623/7c94aa8e/attachment-0001.sig>