[Xen-API] Re: Xen-API C Bindings, version 0.4.1

[Ewan Mellor <ewan@xxxxxxxxxxxxx>]

On Wed, Aug 09, 2006 at 08:36:58PM -0400, Mike D. Day wrote:

> On 08/08/06 17:00 +0100, Ewan Mellor wrote:
> 
> >Attached is the latest version of the C bindings for the Xen Management 
> >API.
> >
> >A record that contains references to other records (Ref in the data model) 
> >has
> >fields of record_opt type.  This type is a tagged union -- either a handle
> >referring to the object, or a full copy of the object's fields.  The
> >get_record calls are only shallow -- they return handles as those 
> >references,
> >not full copies -- but I anticipate that we shall add some deep calls 
> >later.
> 
> 
> 
> 
> >I've added constructors, such as xen_vm_create, that take a xen_vm_record, 
> >and
> >use that to supply parameters to the construction.
> >
> >All items that are allocated by the library and passed to the user have a
> >corresponding call for deallocation (xen_vm_free, for example).
> >
> 
> Simple style question: should we typedef structs? I like the shorthand
> but linux style has some advantages. typedefs tend to hide information
> about the type that can be important. 

I like the shorthand too, but I've provided all of the struct names on the
public interfaces too in case people want to use them.  I've declared them
like

typedef struct foo
{
} foo;

so you can write either "struct foo" or "foo" in your code.

> I like the internal structure of the API. It might be nice for each
> type/value to have pluggable marshall/demarshall functions. The
> marshall/demarshall code in xen_common.c is great but usually there
> are cases where a user needs to roll his own.

I had intended that users would keep their sticky little fingers out of
xen_common.c ;-)

The intention is to provide a properly typed C call for every server-side
function available, so that users never need to worry about marshalling.  It's
essentially an implementation detail of the library.

In what sort of scenario would you see this becoming necessary?

> Memory management: 
> 
> Right now the user supplies a buffer with input values (which is
> copied by the API), and the API supplies a result buffer, which the
> user needs to free. This is always hard to get right. It might make
> sense for the caller to provide both. This would avoid a memcpy and
> would reduce memory consumption. Since the caller is also part of the
> API implementation I think it would be best to avoid the memcpy.
> 
> The current prototype should make params const:
> 
> void
> xen_call_(xen_session *s, const char *method_name,
> -       abstract_value params[], int param_count,
> +         const abstract_value params[], int param_count,
>          const abstract_type *result_type, void *value)
> 
> 
> But I would rather prefer this, with the caller supplying parameter
> memory:
> 
> void
> xen_call_(xen_session *s, const char *method_name,
> -       abstract_value params[], int param_count,
> +         const abstract_value *params[], int param_count,
>          const abstract_type *result_type, void *value)

I think this means that I couldn't do the inline declaration of the params
array, doesn't it?  Instead of this:

    const abstract_value param_values[] =
        {
            { .type = &abstract_type_string,
              .u.string_val = vm }
        };

I'd have to write 

    const abstract_value *param1 =
        { .type = &abstract_type_string,
          .u.string_val = vm };
    const abstract_value *param_values[] =
        {
           &param1
        };

That's right isn't it?  That's pretty horrible for me, even if it is an
internal detail of the library!

The memcpy is just there to slot the session parameter in the front of the
parameter list, so we could equally avoid it by doing:

    const abstract_value param_values[] =
        {
            { 0 },
            { .type = &abstract_type_string,
              .u.string_val = vm }
        };

In other words, we require callers of xen_call_ to provide a single slot for
the session parameter.  Again, this is an internal issue, so it's not a
decision that would be exposed to library users.  Would that be acceptable?

> #define XEN_CALL_(method_name__)                                \
>    xen_call_(session, method_name__, param_values,             \
>              sizeof(param_values) / sizeof(param_values[0]),   \
>              &result_type, &result)                            \
> 
> The XEN_CALL macro is kind of scary because it uses inferred
> variables. It's probably because I'm not used to this convention but I
> would prefer 
> 
> #define XEN_CALL_ xen_call_ 
> 
> In other words, it really isn't needed and we should just expand the
> macro ourselves becuase.

This is an internal macro, and is just there so that I don't have to write all
that code by hand!

Yes, it's a little bit skanky, but there are 133 uses of XEN_CALL -- expanding
them by hand would add double that number of lines of code to the library,
each of which could contain a subtle error (like getting the sizeof calls
wrong or missing an &) which the compiler wouldn't catch.  I prefer wrapping
it in a macro.

> More later, nice work!

Thank you!

Ewan.

_______________________________________________
xen-api mailing list
xen-api@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-api