Re: [Qemu-devel] [PATCH 5/9] qapi_sized_buffer

[mdroth <mdroth@linux.vnet.ibm.com>]

On Thu, Mar 14, 2013 at 11:24:03AM -0400, Stefan Berger wrote:
> On 03/14/2013 11:11 AM, mdroth wrote:
> >On Thu, Mar 14, 2013 at 10:51:49AM -0400, Stefan Berger wrote:
> >>On 03/14/2013 10:28 AM, mdroth wrote:
> >>>On Thu, Mar 14, 2013 at 09:39:14AM -0400, Stefan Berger wrote:
> >>>>On 03/14/2013 08:18 AM, mdroth wrote:
> >>>>>On Wed, Mar 13, 2013 at 09:48:11PM -0400, Stefan Berger wrote:
> >>>>>>On 03/13/2013 07:18 PM, mdroth wrote:
> >>>>>>>On Wed, Mar 13, 2013 at 06:00:24PM -0400, Stefan Berger wrote:
> >>>>>>>>On 03/13/2013 04:52 PM, mdroth wrote:
> >>>>>>>>
> >>>>>>>Visitors don't have any knowledge of the data structures they're visiting
> >>>>>>>outside of what we tell them via the visit_*() API.
> >>>>>>>
> >>>>>>>[...]
> >>>>>>>
> >>>>>>>For example, a visitor for a 16-element array of:
> >>>>>>>
> >>>>>>>typedef struct ComplexType {
> >>>>>>>     int32_t foo;
> >>>>>>>     char *bar;
> >>>>>>>} ComplexType;
> >>>>>>>
> >>>>>>>would look something like:
> >>>>>>>
> >>>>>>>visit_start_carray(v, ...); // instruct visitor how to calculate offsets
> >>>>>>>for (i = 0; i < 16; i++) {
> >>>>>>>     visit_type_ComplexType(v, ...) // instruct visitor how to handle elem
> >>>>>>>     visit_next_carray(v, ...); // instruct visitor to move to next offset
> >>>>>>>}
> >>>>>>>visit_end_carray(v, ...); // instruct visitor to finalize array
> >>>>>>Given this example above, I think we will need the sized buffer. The
> >>>>>>sized buffer targets  binary arrays and their encoding. If I was to
> >>>>>>encode an 'unsigned char[n]' (e.g., n=200) using n, or n/2 or n/4
> >>>>>>loops like above breaking it apart in u8, u16 or u32 respectively I
> >>>>>>think this would 'not bed good' also considering the 2 bytes for tag
> >>>>>>and length being added by ASN.1 for every such datatype
> >>>>>>(u8,u16,u32). The sized buffer allows you to for example take a
> >>>>>>memory page and write it out in one chunk adding a few bytes of
> >>>>>>ASN.1 'decoration' around the actual data.
> >>>>>You could do it with this interface as well actually. The Visitor will
> >>>>>need to maintain some internal state to differentiate what it does with
> >>>>>subsequent visit_type*/visit_next_carray/visit_end_carry. There's no
> >>>>>reason it couldn't also track the elem size so it could tag a buffer
> >>>>>"en masse" when visit_end_carray() gets called.
> >>>>It depends on what you pass into visit_start_carray. In your case if
> >>>>you pass in ComplexType you would pass in a sizeof(ComplexType) for
> >>>>the size of each element presumably. The problem is now you havechar *foo, a string pointer, hanging off of this structure. How
> >>>>would you handle that? Serializing ComplexType's foo and pointer
> >>>>obviously won't do it.
> >>>Why not?  visit_type_ComplexType() knows how to deal with
> >>>the individual fields, including the string pointer. I'm not sure
> >>>what's at issue here.
> >>>
> >>>In this case the handling for ComplexType would look something like:
> >>>
> >>>visit_type_Complex:
> >>>     visit_start_struct
> >>>     visit_type_uin32 //foo
> >>>     visit_type_str //bar
> >>>     visit_end_struct
> >>>
> >>>Granted, strings are easier to deal with. If char * was instead a plain
> >>>old uint8_t*, we'd need a nested call to start_carray for each element.
> >>>in this case it would look something like:
> >>>
> >>>visit_type_Complex:
> >>>     visit_start_struct
> >>>     visit_type_uin32 //foo field
> >>>     visit_start_carray //bar field
> >>>     for (i = 0; i < len_of_bar; i++):
> >>>         visit_type_uint8
> >>>         visit_next_carray
> >>>     visit_end_carray
> >>You really want to create a separate element for each element in
> >>this potentially large binary array? I guess it depends on the
> >>underlying data, but this has the potential of generating a lot of
> >>control code around each such byte... As said, for ASN.1 encoding,
> >>each such byte would be decorated with a tag and a length value,
> >>consuming 2 more bytes per byte.
> >I addressed this earlier. Your visitor doesn't have tag each
> >element: if it know it's handling an array (because we told it via
> >start_carray()), it can buffer them internally and tag the array en
> >masse when end_carray() is issued.
> 
> If we were to do this using carray on an array of structs of the
> following type
> 
> struct SimpleStruct {
>     uint8_t a;
>     uint8_t b;
>     uint32_t c;
> }
> 
> then the serialization of a and b would be buffered and flushed once
> the 32bit output visitor (or any other than uint8_t output visitor)
> would be called?

I don't quite understand. For a struct, we'd tag each field
individually, right?

It's avoiding the need to tag each element in a list, each SimpleStruct,
that's at issue, right? We have a special case for u8 arrays that is
currently handled by qapi_sized_buffer(), and now we're trying to
generalize this optimized handling for more complex data types via
visit_carray_*?

I guess my first question is whether or not it's possible for more complex
data types. For u8 arrays we seem to use a special OCTET_STRING encoding.

If we're not sure we can do this more generally, we do have the option
of only special-casing u8 arrays to use the OCTET_STRING encoding, and
handle the others with "non-optimized" encodings.

If you have an idea for what a generalized, optimized encoding that's
applicable for non-u8 types would look like, we can work through that if
you have an example optimized encoding for, say, and array of
SimpleStruct.

> 
>   Stefan
>