On Fri, Jan 16, 2009 at 12:01:16PM +0000, Richard W.M. Jones wrote:
> On Tue, Jan 13, 2009 at 09:51:00PM +0000, Daniel P. Berrange wrote:
> [...]
>
> From a "peephole" view of the patch, it looks sane enough, and I think
> we should commit it and debug any fallout later. One tiny comment:
>
> > +struct qemud_client_message;
> > +
> > +struct qemud_client_message {
> > + char buffer [REMOTE_MESSAGE_MAX + REMOTE_MESSAGE_HEADER_XDR_LEN];
> > + unsigned int bufferLength;
> > + unsigned int bufferOffset;
> > +
> > + int async : 1;
> > +
> > + struct qemud_client_message *next;
> > +};
>
> (1) You don't need to predeclare the struct. (2) This means the
> struct is always the same size as the maximum-sized message.
> Shouldn't we malloc the buffer?
I did consider whether we should malloc the buffer, but this only
helps when receiving the RPC request. We know there what the size
of the incoming message is, from the length field in the header.
When we are generating the reply message though, we don't know how
big it'll be until we've made the XDR calls to serialize the struct
into the XDR. So we'd need to have the full sized buffer for filling
out the reply.
To avoid having to malloc() a 'struct qemud_client_message' for the
RPC reply, I currently just re-use the existing malloc()'d one we
had for the RPC request, since that's now unused. If we only malloc'd
the buffer that we needed for the request, then we'd need to realloc
it to the full size for the reply anyway.
So I think just declaring the struct to the full buffer size is an
acceptable approach - particularly since we never have any of these
structs on the stack - they're all on the heap.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list