[libvirt] [PATCH 01/15] Defines the basics of a generic RPC protocol in XDR

[Daniel P. Berrange]

On Thu, Dec 16, 2010 at 08:27:23AM -0700, Eric Blake wrote:
> On 12/16/2010 04:21 AM, Daniel P. Berrange wrote:
> > +const VIR_NET_MESSAGE_HEADER_MAX = 24;
> > +
> > +/* Size of message payload */
> > +const VIR_NET_MESSAGE_PAYLOAD_MAX = 262120;
> 
> Would it be better to write VIR_NET_MESSAGE_PAYLOAD_MAX =
> VIR_NET_MESSAGE_MAX - VIR_NET_MESSAGE_HEADER_MAX?  But that's just cosmetic.

Unfortunately you can use expressions when defining
XDR constants.

> 
> > +
> > +/* Size of message length field. Not counted in VIR_NET_MESSAGE_MAX */
> > +const VIR_NET_MESSAGE_LEN_MAX = 4;
> > +
> > +const VIR_NET_MESSAGE_STRING_MAX = 65536;
> 
> You lost a useful comment from remote_protocol.h:
> 
> /* Length of long, but not unbounded, strings.
>  * This is an arbitrary limit designed to stop the decoder from trying
>  * to allocate unbounded amounts of memory when fed with a bad message.
>  */

Yeah, I didn't even really want these bits of the protocol,
but backcompat for the error struct forced me to copy them
in.

> > + * and the 'status' field varies according to:
> > + *
> > + *  - type == VIR_NET_CALL
> > + *     * VIR_NET_OK always
> > + *
> > + *  - type == VIR_NET_REPLY
> > + *     * VIR_NET_OK if RPC finished successfully
> > + *     * VIR_NET_ERROR if something failed
> > + *
> > + *  - type == VIR_NET_MESSAGE
> > + *     * VIR_NET_OK always
> > + *
> > + *  - type == VIR_NET_STREAM
> > + *     * VIR_NET_CONTINUE if more data is following
> > + *     * VIR_NET_OK if stream is complete
> > + *     * VIR_NET_ERROR if stream had an error
> > + *
> > + * Payload varies according to type and status:
> > + *
> > + *  - type == VIR_NET_CALL
> > + *          XXX_args  for procedure
> > + *
> > + *  - type == VIR_NET_REPLY
> > + *     * status == VIR_NET_OK
> > + *          XXX_ret         for procedure
> > + *     * status == VIR_NET_ERROR
> > + *          remote_error    Error information
> > + *
> > + *  - type == VIR_NET_MESSAGE
> > + *     * status == VIR_NET_OK
> > + *          XXX_args        for procedure
> > + *     * status == VIR_NET_ERROR
> > + *          remote_error    Error information
> 
> This is pure copy-and-paste from remote_protocol.x, but it doesnt' make
> sense to me.  Earlier, you said that VIR_NET_MESSAGE implies status is
> VIR_NET_OK always, and that messages are asynchronous, rather than in
> reply to a message.  Either this should be:
> 
> - type == VIR_NET_MESSAGE
>     XXX_msg      according to proc

Yeah that's the correct one. There are no errors associated
with async events.

> or you need to allow for VIR_NET_MESSAGE to pass status==VIR_NET_ERROR.


Daniel