Mailing List Archive

Wow, that's an incredible amount of code written already ! I managed to
compile & run the test programs without any significant trouble. The only
two very minor issues I had were likely just related to differing versions
of the dependant libraries from our respective OS platforms:

1. The Makefile sets -Werror but the xen_common.c spews a tonne of warning
messages resulting in a failed compile (I removed the -Werror to complete
the compile). FYI the warnings were all along the lines of:

cc -Iinclude -I/usr/include/libxml2 -W -Wall -Wextra -std=c99 -O2 -fPIC -c -o src/xen_common.o src/xen_common.c
src/xen_common.c: In function ?xen_init?:
src/xen_common.c:102: warning: pointer targets in passing argument 1 of ?xmlXPathCompile? differ in signedness
src/xen_common.c:105: warning: pointer targets in passing argument 1 of ?xmlXPathCompile? differ in signedness
src/xen_common.c: In function ?call_raw?:
src/xen_common.c:243: warning: pointer targets in passing argument 2 of ?parse_result? differ in signedness
src/xen_common.c: In function ?is_container_node?:
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?strlen? differ in signedness
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?__builtin_strcmp? differ in signedness
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?strlen? differ in signedness
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?__builtin_strcmp? differ in signedness
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?__builtin_strcmp? differ in signedness
src/xen_common.c:293: warning: pointer targets in passing argument 1 of ?__builtin_strcmp? differ in signedness

2. The test program failed to link because the libcurl -l linker flags
were surrounded in -Wl,--as-needed. The fix was simply to re-arrange
the linker flags from

$(CC) $(LDFLAGS) -o $@ $<O-L src -lxen

To:

$(CC) -o $@ $< $(LDFLAGS) -L src -lxen

On Tue, Aug 08, 2006 at 05:00:23PM +0100, Ewan Mellor wrote:
> I've added a record type to each of the classes, such as xen_vm_record, which
> will be used for client-side storage of data fields. I've added getters for
> these records, such as xen_vm_get_record, which return one of these.

The design of the API feels a little odd to me - there are now basically two
ways of getting at every piece of information. Either fetch a record and then
access the struct members, or call each xen_vm_get_XXX method as required. The
latter methods in their current impl result in an XML-RPC roundtrip on every
call which seems rather undesirable. Given this performance overhead, under
what circumstances would you anticipate one wanting to use the xen_vm_get_XXX
methods at all - accessing struct members is just as easy & less overhead?

Following on from this can you clarify how the apps are supposed to approach
error handling. The xen_session struct has an 'error_description' array of
strings, but its unclear to me when one is supposed to check this? All the
setter methods have a 'void' return type, and there is no explicit return
code indicating errors for the getters (although you could check for NULL
in some of them, others returning an enum aren't so clear), so how do youy
tell whether a method succeeded or failed ? From a cursory glance at the impl
of the xen_vm.c class it looks like every method can at least have a failure
from malloc() returning NULL, but this is never checked / propagated back to
the caller.

Finally, on the much discussed topic of resource utilization stats. From
previous discussions I was under the impression the APIs for fetching
these stats were going to be separated out from the APIs for fetching info
about an object's configuration / hardware properties. The xen_vm_record
xen_vbd_record and xen_vif_record structs all still contain the members
for utilization stats. What is the plan for providing APIs to efficiently
retrieve these stats in a single call ? eg a xen_vm_stats() or some such
API call.

Oh actually, one further question - where/how is information about the
virtual display adapters / serial console represented ? eg VNC server
port, SDL, and xenconsoled Pseudo TTY associated with the domain ?

Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

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

On Tue, Aug 08, 2006 at 08:11:07PM +0100, Daniel P. Berrange wrote:
> Wow, that's an incredible amount of code written already ! I managed to
> compile & run the test programs without any significant trouble. The only
> two very minor issues I had were likely just related to differing versions
> of the dependant libraries from our respective OS platforms:
>
> 1. The Makefile sets -Werror but the xen_common.c spews a tonne of warning
> messages resulting in a failed compile (I removed the -Werror to complete
> the compile). FYI the warnings were all along the lines of:
>
> cc -Iinclude -I/usr/include/libxml2 -W -Wall -Wextra -std=c99 -O2 -fPIC -c -o src/xen_common.o src/xen_common.c
> src/xen_common.c: In function ?xen_init?:
> src/xen_common.c:102: warning: pointer targets in passing argument 1 of ?xmlXPathCompile? differ in signedness

Classic case of using libxml2 xmlChar * where a char * is expected and
vice-versa, you need to double check that the string is UTF-8 and then just
add a cast (http://xmlsoft.org/FAQ.html#Developer item 12)

I also noted at least a few places where xmlChar * were freed using
free() instead of xmlFree(), that usually work on Unix, tend to fail on
Windows very easilly, and will break if a program using this library
also uses libxml2 and changes the allocator (usually done only on embedded
systems but apache modules do that too).

When using XPath with constant string, there is a serious optimization
consisting of compiling first the XPath, and then reusing the compiled
XPath instead for the repetive queries. But it's probably a bit early for
optimization.

> On Tue, Aug 08, 2006 at 05:00:23PM +0100, Ewan Mellor wrote:
> > I've added a record type to each of the classes, such as xen_vm_record, which
> > will be used for client-side storage of data fields. I've added getters for
> > these records, such as xen_vm_get_record, which return one of these.
>
> The design of the API feels a little odd to me - there are now basically two
> ways of getting at every piece of information. Either fetch a record and then
> access the struct members, or call each xen_vm_get_XXX method as required. The
> latter methods in their current impl result in an XML-RPC roundtrip on every
> call which seems rather undesirable. Given this performance overhead, under
> what circumstances would you anticipate one wanting to use the xen_vm_get_XXX
> methods at all - accessing struct members is just as easy & less overhead?

I suggested in July adding client side storage precisely to avoid many round
trip. But I'm not sure how you can update a locally modified structure, that
still seems to require the accessors.

[...]

I agree with the other points raised by Dan. I will add another one,
the use of enum within public structures and parameters or return of
functions, this is a very weak point of C, you don't have a good garantee
of allocation size, this may vary from one compiler to another, or
if you grow the number of items in the enum, I would avoid that and
use int (but document that it's supposed to be an enum), you loose a bit
of typechecking by compiler but gain ABI stability.
Oh, I like my code very commented, usually that's something impossible
to add after the initial write of the code better do it early than late.

Daniel

--
Daniel Veillard | Red Hat http://redhat.com/
veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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

On Tue, Aug 08, 2006 at 08:11:07PM +0100, Daniel P. Berrange wrote:

> Wow, that's an incredible amount of code written already !

*grin* I just wanted to do enough to be able to make a create_vm call, but
that turned out to be just about all of it!

> I managed to
> compile & run the test programs without any significant trouble. The only
> two very minor issues I had were likely just related to differing versions
> of the dependant libraries from our respective OS platforms:
>
> 1. The Makefile sets -Werror but the xen_common.c spews a tonne of warning
> messages resulting in a failed compile (I removed the -Werror to complete
> the compile). FYI the warnings were all along the lines of:

Thanks. My older GCC obviously doesn't flag those warnings, but they look
pretty easily solved -- I think they're all where I'm using char * rather than
xmlChar * or vice versa. My intention was for all strings to be UTF-8, like
libxml2, but without exposing xmlChar at the interface, so I just need to
sprinkle some casts around.

> 2. The test program failed to link because the libcurl -l linker flags
> were surrounded in -Wl,--as-needed. The fix was simply to re-arrange
> the linker flags from
>
> $(CC) $(LDFLAGS) -o $@ $<O-L src -lxen
>
> To:
>
> $(CC) -o $@ $< $(LDFLAGS) -L src -lxen

Cool, thanks, I'll fix that.

> On Tue, Aug 08, 2006 at 05:00:23PM +0100, Ewan Mellor wrote:
> > I've added a record type to each of the classes, such as xen_vm_record, which
> > will be used for client-side storage of data fields. I've added getters for
> > these records, such as xen_vm_get_record, which return one of these.
>
> The design of the API feels a little odd to me - there are now basically two
> ways of getting at every piece of information. Either fetch a record and then
> access the struct members, or call each xen_vm_get_XXX method as required. The
> latter methods in their current impl result in an XML-RPC roundtrip on every
> call which seems rather undesirable. Given this performance overhead, under
> what circumstances would you anticipate one wanting to use the xen_vm_get_XXX
> methods at all - accessing struct members is just as easy & less overhead?

I can imagine you wanting to access some of the fields certainly --
Vm.memory_actual, for example, would let you cheaply monitor changes in VM
memory consumption. Of course, no every field is going to be useful as an
individual get call, but it's not clear precisely which ones they are, and
they aren't expensive to support, so I thought that we may as well throw them
all in, for simplicity's sake.

> Following on from this can you clarify how the apps are supposed to approach
> error handling. The xen_session struct has an 'error_description' array of
> strings, but its unclear to me when one is supposed to check this? All the
> setter methods have a 'void' return type, and there is no explicit return
> code indicating errors for the getters (although you could check for NULL
> in some of them, others returning an enum aren't so clear), so how do youy
> tell whether a method succeeded or failed ?

The intention is that you check xen_session->ok before you use any of the
values returned by the library in your application, or before you make any
state-modifying calls on the server. You can chain together a few calls into
the library if necessary, as once ok is set to false, subsequent calls won't
actually take place. In many cases, this will degenerate to checking
xen_session->ok after each call into the library.

On that subject, the intention with the error_description array is that the
first element is a code, and any subsequent elements are the parameters to the
error message. You would look up the code in your internationalised message
database, and then substitute the parameters in appropriately. We still need
to define those codes and the parameters that match -- I've just done this on
an ad-hoc basis in the library at the moment.

> From a cursory glance at the impl
> of the xen_vm.c class it looks like every method can at least have a failure
> from malloc() returning NULL, but this is never checked / propagated back to
> the caller.

Yes, there will certainly be some failures that I haven't checked for, in
particular malloc calls. I'll be going through and checking for these kind of
things as part of the code audit that I'm going to do when they drop into Xen
pre-3.0.4.

> Finally, on the much discussed topic of resource utilization stats. From
> previous discussions I was under the impression the APIs for fetching
> these stats were going to be separated out from the APIs for fetching info
> about an object's configuration / hardware properties. The xen_vm_record
> xen_vbd_record and xen_vif_record structs all still contain the members
> for utilization stats. What is the plan for providing APIs to efficiently
> retrieve these stats in a single call ? eg a xen_vm_stats() or some such
> API call.

Yes, something like a xen_vm_stats() call is what I had in mind, though there
might be more than one, depending upon the kind of monitoring that you want to
do. Any ideas on that front?

> Oh actually, one further question - where/how is information about the
> virtual display adapters / serial console represented ? eg VNC server
> port, SDL, and xenconsoled Pseudo TTY associated with the domain ?

It looks like those are missing -- I'll put them on the TODO list.

Thanks,

Ewan.

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

On Tue, Aug 08, 2006 at 04:06:47PM -0400, Daniel Veillard wrote:

> On Tue, Aug 08, 2006 at 08:11:07PM +0100, Daniel P. Berrange wrote:
> > Wow, that's an incredible amount of code written already ! I managed to
> > compile & run the test programs without any significant trouble. The only
> > two very minor issues I had were likely just related to differing versions
> > of the dependant libraries from our respective OS platforms:
> >
> > 1. The Makefile sets -Werror but the xen_common.c spews a tonne of warning
> > messages resulting in a failed compile (I removed the -Werror to complete
> > the compile). FYI the warnings were all along the lines of:
> >
> > cc -Iinclude -I/usr/include/libxml2 -W -Wall -Wextra -std=c99 -O2 -fPIC -c -o src/xen_common.o src/xen_common.c
> > src/xen_common.c: In function ?xen_init?:
> > src/xen_common.c:102: warning: pointer targets in passing argument 1 of ?xmlXPathCompile? differ in signedness
>
> Classic case of using libxml2 xmlChar * where a char * is expected and
> vice-versa, you need to double check that the string is UTF-8 and then just
> add a cast (http://xmlsoft.org/FAQ.html#Developer item 12)
>
> I also noted at least a few places where xmlChar * were freed using
> free() instead of xmlFree(), that usually work on Unix, tend to fail on
> Windows very easilly, and will break if a program using this library
> also uses libxml2 and changes the allocator (usually done only on embedded
> systems but apache modules do that too).

Yes, I've obviously missed a few of those. I'll take Daniel B's warnings and
track them down.

> When using XPath with constant string, there is a serious optimization
> consisting of compiling first the XPath, and then reusing the compiled
> XPath instead for the repetive queries. But it's probably a bit early for
> optimization.

I'm doing this already, no? Or is there another optimisation that I don't
know about?

> [Snip]
>
> I agree with the other points raised by Dan. I will add another one,
> the use of enum within public structures and parameters or return of
> functions, this is a very weak point of C, you don't have a good garantee
> of allocation size, this may vary from one compiler to another, or
> if you grow the number of items in the enum, I would avoid that and
> use int (but document that it's supposed to be an enum), you loose a bit
> of typechecking by compiler but gain ABI stability.

That's an interesting point. I thought that enums were defined to be the same
size as int, but looking around it seems more complicated than this!

> Oh, I like my code very commented, usually that's something impossible
> to add after the initial write of the code better do it early than late.

More comments on the way, I promise!

Ewan.

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

On Tue, Aug 08, 2006 at 10:06:52PM +0100, Ewan Mellor wrote:
> On Tue, Aug 08, 2006 at 04:06:47PM -0400, Daniel Veillard wrote:
> > When using XPath with constant string, there is a serious optimization
> > consisting of compiling first the XPath, and then reusing the compiled
> > XPath instead for the repetive queries. But it's probably a bit early for
> > optimization.
>
> I'm doing this already, no?

heh, right, I misread :-)

> Or is there another optimisation that I don't
> know about?

If you can reuse the xmlXPathContext that could be non neglectible too
but thread safety becomes a concern. Only one thread at a time can use it
obviously.

> > I agree with the other points raised by Dan. I will add another one,
> > the use of enum within public structures and parameters or return of
> > functions, this is a very weak point of C, you don't have a good garantee
> > of allocation size, this may vary from one compiler to another, or
> > if you grow the number of items in the enum, I would avoid that and
> > use int (but document that it's supposed to be an enum), you loose a bit
> > of typechecking by compiler but gain ABI stability.
>
> That's an interesting point. I thought that enums were defined to be the same
> size as int, but looking around it seems more complicated than this!

I have seen ABI breakage because the number of values grew up and the
compiler decided to allocate more bytes, ouch ...

Daniel

--
Daniel Veillard | Red Hat http://redhat.com/
veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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

On Tue, Aug 08, 2006 at 09:50:14PM +0100, Ewan Mellor wrote:
> On Tue, Aug 08, 2006 at 08:11:07PM +0100, Daniel P. Berrange wrote:
> > On Tue, Aug 08, 2006 at 05:00:23PM +0100, Ewan Mellor wrote:
> > > I've added a record type to each of the classes, such as xen_vm_record, which
> > > will be used for client-side storage of data fields. I've added getters for
> > > these records, such as xen_vm_get_record, which return one of these.
> >
> > The design of the API feels a little odd to me - there are now basically two
> > ways of getting at every piece of information. Either fetch a record and then
> > access the struct members, or call each xen_vm_get_XXX method as required. The
> > latter methods in their current impl result in an XML-RPC roundtrip on every
> > call which seems rather undesirable. Given this performance overhead, under
> > what circumstances would you anticipate one wanting to use the xen_vm_get_XXX
> > methods at all - accessing struct members is just as easy & less overhead?
>
> I can imagine you wanting to access some of the fields certainly --
> Vm.memory_actual, for example, would let you cheaply monitor changes in VM
> memory consumption. Of course, no every field is going to be useful as an
> individual get call, but it's not clear precisely which ones they are, and
> they aren't expensive to support, so I thought that we may as well throw them
> all in, for simplicity's sake.

What I guess I was hinting at is that if getting a struct record for the VM
object is preferred general access method, why have the complexity of having
separate handle / struct objects? The 'xen_vm' handle could just be an opaque
pointer to the xen_vm_record struct in the first place. The xm_vm_get_XXX methods
could then simply be wrappers doing local access to the struct members instead
of doing RPC calls every time. It would make it clearer to app programmers
how to use the API - they wouldn't have to worry about 'should I use the
xm_vm handle & getters' vs 'should i use xen_vm_record' because they'd be one
and the same

> > Following on from this can you clarify how the apps are supposed to approach
> > error handling. The xen_session struct has an 'error_description' array of
> > strings, but its unclear to me when one is supposed to check this? All the
> > setter methods have a 'void' return type, and there is no explicit return
> > code indicating errors for the getters (although you could check for NULL
> > in some of them, others returning an enum aren't so clear), so how do youy
> > tell whether a method succeeded or failed ?
>
> The intention is that you check xen_session->ok before you use any of the
> values returned by the library in your application, or before you make any
> state-modifying calls on the server. You can chain together a few calls into
> the library if necessary, as once ok is set to false, subsequent calls won't
> actually take place. In many cases, this will degenerate to checking
> xen_session->ok after each call into the library.

This sounds like a very bad idea to me, because the error handling is totally
non-obvious to programmers using the API. Even if we document that you should
check 'xen_session->ok' after making any API call, the reality is that
people just don't read docs. They will look at the header file and see that
a method signature returns 'void' and assume that no errors will occur. It
will actively encourage bad pratice / sloppy error handling amongst users of
this API.

Its basically akin to removing all the return codes from libc functions and
saying you must check errno every time to see if something went wrong. Its
just not going to happen. You will also loose the benefit of compiler warnings
eg, the compiler can warn if you forget to check a return value - it can't
say anything about forgetting to check a completely unrelated struct member
like 'xen_session->ok'.

Given a choice between an API :

int memory;
memory = xen_vm_get_memory(vm);
if (!session->ok)
...do error stuff...

xen_vm_set_vcpus(vm, 3);
if (!session->ok)
...do errror stuff...

Or

int memory;
if (!(memory = xen_vm_get_memory(vm)))
...do error stuff...
if (!xen_vm_set_vcpus(vm))
...do error stuff...

The latter is a pretty obvious error handling approach to C, while the former
is just plain odd - you're checking a struct which isn't even the same as
the object you just operated on. By all means have the full error code / details
in the xen_session struct, but the methods really need to provide a boolean
success / fail return value. Looking at the APIs thus far this shouldn't be
all that difficult - those returning a pointer can provide NULL for failure,
while those returning an int can return '0' or '-1' as appropriate for their
semantics.

> On that subject, the intention with the error_description array is that the
> first element is a code, and any subsequent elements are the parameters to the
> error message. You would look up the code in your internationalised message
> database, and then substitute the parameters in appropriately. We still need
> to define those codes and the parameters that match -- I've just done this on
> an ad-hoc basis in the library at the moment.

If we're going to define formal error code as the first element, could we go
one step further and actually make it into a integer constant / enum element.
This would simplify checking by the client code allowing use of integer comparison,
switch statements, etc for error handling, rather than requiring the use of
strcmp.

> > Finally, on the much discussed topic of resource utilization stats. From
> > previous discussions I was under the impression the APIs for fetching
> > these stats were going to be separated out from the APIs for fetching info
> > about an object's configuration / hardware properties. The xen_vm_record
> > xen_vbd_record and xen_vif_record structs all still contain the members
> > for utilization stats. What is the plan for providing APIs to efficiently
> > retrieve these stats in a single call ? eg a xen_vm_stats() or some such
> > API call.
>
> Yes, something like a xen_vm_stats() call is what I had in mind, though there
> might be more than one, depending upon the kind of monitoring that you want to
> do. Any ideas on that front?

Well, run state (blocked, running, crashed, idle, etc) domain CPU time,
memory allocation, # of virt CPUs. Perhaps a per-CPU breakdown of run
state, time & host CPU scheduled.

Might want to have some flags to the xen_vm_stats() method to indicate whether
to also pull out disk & network device IO stats at the same time.

Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

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

On Wed, Aug 09, 2006 at 05:15:27AM +0100, Daniel P. Berrange wrote:

> On Tue, Aug 08, 2006 at 09:50:14PM +0100, Ewan Mellor wrote:
> > On Tue, Aug 08, 2006 at 08:11:07PM +0100, Daniel P. Berrange wrote:
> > > On Tue, Aug 08, 2006 at 05:00:23PM +0100, Ewan Mellor wrote:
> > > > I've added a record type to each of the classes, such as xen_vm_record, which
> > > > will be used for client-side storage of data fields. I've added getters for
> > > > these records, such as xen_vm_get_record, which return one of these.
> > >
> > > The design of the API feels a little odd to me - there are now basically two
> > > ways of getting at every piece of information. Either fetch a record and then
> > > access the struct members, or call each xen_vm_get_XXX method as required. The
> > > latter methods in their current impl result in an XML-RPC roundtrip on every
> > > call which seems rather undesirable. Given this performance overhead, under
> > > what circumstances would you anticipate one wanting to use the xen_vm_get_XXX
> > > methods at all - accessing struct members is just as easy & less overhead?
> >
> > I can imagine you wanting to access some of the fields certainly --
> > Vm.memory_actual, for example, would let you cheaply monitor changes in VM
> > memory consumption. Of course, no every field is going to be useful as an
> > individual get call, but it's not clear precisely which ones they are, and
> > they aren't expensive to support, so I thought that we may as well throw them
> > all in, for simplicity's sake.
>
> What I guess I was hinting at is that if getting a struct record for the VM
> object is preferred general access method, why have the complexity of having
> separate handle / struct objects? The 'xen_vm' handle could just be an opaque
> pointer to the xen_vm_record struct in the first place. The xm_vm_get_XXX methods
> could then simply be wrappers doing local access to the struct members instead
> of doing RPC calls every time. It would make it clearer to app programmers
> how to use the API - they wouldn't have to worry about 'should I use the
> xm_vm handle & getters' vs 'should i use xen_vm_record' because they'd be one
> and the same

The intention is that these _would_ be different things -- accessing a struct
accesses a potentially stale copy of the data, whereas hitting the server is
always up-to-date of course. It's up to the application to decide between
these options, given the cost of the latter.

> > > Following on from this can you clarify how the apps are supposed to approach
> > > error handling. The xen_session struct has an 'error_description' array of
> > > strings, but its unclear to me when one is supposed to check this? All the
> > > setter methods have a 'void' return type, and there is no explicit return
> > > code indicating errors for the getters (although you could check for NULL
> > > in some of them, others returning an enum aren't so clear), so how do youy
> > > tell whether a method succeeded or failed ?
> >
> > The intention is that you check xen_session->ok before you use any of the
> > values returned by the library in your application, or before you make any
> > state-modifying calls on the server. You can chain together a few calls into
> > the library if necessary, as once ok is set to false, subsequent calls won't
> > actually take place. In many cases, this will degenerate to checking
> > xen_session->ok after each call into the library.
>
> This sounds like a very bad idea to me, because the error handling is totally
> non-obvious to programmers using the API. Even if we document that you should
> check 'xen_session->ok' after making any API call, the reality is that
> people just don't read docs. They will look at the header file and see that
> a method signature returns 'void' and assume that no errors will occur. It
> will actively encourage bad pratice / sloppy error handling amongst users of
> this API.
>
> Its basically akin to removing all the return codes from libc functions and
> saying you must check errno every time to see if something went wrong. Its
> just not going to happen. You will also loose the benefit of compiler warnings
> eg, the compiler can warn if you forget to check a return value - it can't
> say anything about forgetting to check a completely unrelated struct member
> like 'xen_session->ok'.
>
> Given a choice between an API :
>
> int memory;
> memory = xen_vm_get_memory(vm);
> if (!session->ok)
> ...do error stuff...
>
> xen_vm_set_vcpus(vm, 3);
> if (!session->ok)
> ...do errror stuff...
>
> Or
>
> int memory;
> if (!(memory = xen_vm_get_memory(vm)))
> ...do error stuff...
> if (!xen_vm_set_vcpus(vm))
> ...do error stuff...
>
> The latter is a pretty obvious error handling approach to C, while the former
> is just plain odd - you're checking a struct which isn't even the same as
> the object you just operated on. By all means have the full error code / details
> in the xen_session struct, but the methods really need to provide a boolean
> success / fail return value. Looking at the APIs thus far this shouldn't be
> all that difficult - those returning a pointer can provide NULL for failure,
> while those returning an int can return '0' or '-1' as appropriate for their
> semantics.

It's not that odd -- it's an approach I've seen used in a number of libraries
before -- xmlrpc-c most recently, but also with CORBA bindings that I've used
in the past. This approach has two advantages: you can chain calls together
without having to clutter the code with error checking, and when you return a
value, you don't need to decide which value is to be your error code.

We already have to pass in a session object -- to give the session ID and
receive back the error message -- so it makes sense to me to use this for the
success/failure flag too. This way, you can guarantee that chained calls
following a failure don't do anything, because the binding can inspect the
session object.

> > On that subject, the intention with the error_description array is that the
> > first element is a code, and any subsequent elements are the parameters to the
> > error message. You would look up the code in your internationalised message
> > database, and then substitute the parameters in appropriately. We still need
> > to define those codes and the parameters that match -- I've just done this on
> > an ad-hoc basis in the library at the moment.
>
> If we're going to define formal error code as the first element, could we go
> one step further and actually make it into a integer constant / enum element.
> This would simplify checking by the client code allowing use of integer comparison,
> switch statements, etc for error handling, rather than requiring the use of
> strcmp.

Yes, we certainly should. I'll put that on the list.

> > > Finally, on the much discussed topic of resource utilization stats. From
> > > previous discussions I was under the impression the APIs for fetching
> > > these stats were going to be separated out from the APIs for fetching info
> > > about an object's configuration / hardware properties. The xen_vm_record
> > > xen_vbd_record and xen_vif_record structs all still contain the members
> > > for utilization stats. What is the plan for providing APIs to efficiently
> > > retrieve these stats in a single call ? eg a xen_vm_stats() or some such
> > > API call.
> >
> > Yes, something like a xen_vm_stats() call is what I had in mind, though there
> > might be more than one, depending upon the kind of monitoring that you want to
> > do. Any ideas on that front?
>
> Well, run state (blocked, running, crashed, idle, etc) domain CPU time,
> memory allocation, # of virt CPUs. Perhaps a per-CPU breakdown of run
> state, time & host CPU scheduled.
>
> Might want to have some flags to the xen_vm_stats() method to indicate whether
> to also pull out disk & network device IO stats at the same time.

Cool, thanks.

Ewan.

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

On Wed, Aug 09, 2006 at 04:04:40PM +0100, Ewan Mellor wrote:
> It's not that odd -- it's an approach I've seen used in a number of libraries
> before -- xmlrpc-c most recently, but also with CORBA bindings that I've used
> in the past. This approach has two advantages: you can chain calls together
> without having to clutter the code with error checking, and when you return a
> value, you don't need to decide which value is to be your error code.

Sorry, that's horrible, because you loose locality of the error.
Errors should be detected as soon as possible to be able to report
them as accurately as possible. Point in case I have a domain which
doesn't start, failure occurs within xend in XendDomainInfo::initDomain
you got a big try:/except: where most of the hard and prone to fail code
sit and basically errors are handled only at the end. Result: you just
get

in initDomain
raise VmError(str(exn))
VmError: (9, 'Bad file descriptor')

as the informations to work with. Sorry encouraging this kind of error
handling is a mistake, not an advantage.
I completely agree with Dan that the API should encourage proper
error handling by making test easy and following common convention which
is to return error code which can be easilly tested. Then also having
a more complete way to extract errors should be provided (including
more complete error code and optional context informations) and those
should use the context, yes.
IMHO doing error checking on each operation which can fail is not
cluttering code, it is writing good code. Writing the code in the first
place is a small effort in comparison of debugging and maintaining it,
optimizing for the first, cheapest operation at the expense of the others
is, I think, a bad decision.

Daniel

--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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

On Wed, Aug 09, 2006 at 12:12:59PM -0400, Daniel Veillard wrote:

> On Wed, Aug 09, 2006 at 04:04:40PM +0100, Ewan Mellor wrote:
> > It's not that odd -- it's an approach I've seen used in a number of libraries
> > before -- xmlrpc-c most recently, but also with CORBA bindings that I've used
> > in the past. This approach has two advantages: you can chain calls together
> > without having to clutter the code with error checking, and when you return a
> > value, you don't need to decide which value is to be your error code.
>
> Sorry, that's horrible, because you loose locality of the error.
> Errors should be detected as soon as possible to be able to report
> them as accurately as possible.

If you want to detect them as soon as possible, just have if (session->ok)
after every call. It's no harder than if (return_value == NULL/-1/0/false).
Errors should be reported well, because you've got the additional benefit of
parameterised, internationalisable error messages.

> Point in case I have a domain which
> doesn't start, failure occurs within xend in XendDomainInfo::initDomain
> you got a big try:/except: where most of the hard and prone to fail code
> sit and basically errors are handled only at the end. Result: you just
> get
>
> in initDomain
> raise VmError(str(exn))
> VmError: (9, 'Bad file descriptor')
>
> as the informations to work with. Sorry encouraging this kind of error
> handling is a mistake, not an advantage.

That's unfair. Firstly the error comes from the C binding, and is bounced
across into Python through an interface layer. It's the fact that we only
have errno available across that interface that the error reporting is so
poor. Secondly, the fact that you always get "Bad file descriptor" rather
than a more useful error code is a bug, one that was fixed by your colleague
Steven Rostedt yesterday.

Ewan.

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

On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:
> On Wed, Aug 09, 2006 at 12:12:59PM -0400, Daniel Veillard wrote:
>
> > On Wed, Aug 09, 2006 at 04:04:40PM +0100, Ewan Mellor wrote:
> > > It's not that odd -- it's an approach I've seen used in a number of libraries
> > > before -- xmlrpc-c most recently, but also with CORBA bindings that I've used
> > > in the past. This approach has two advantages: you can chain calls together
> > > without having to clutter the code with error checking, and when you return a
> > > value, you don't need to decide which value is to be your error code.
> >
> > Sorry, that's horrible, because you loose locality of the error.
> > Errors should be detected as soon as possible to be able to report
> > them as accurately as possible.
>
> If you want to detect them as soon as possible, just have if (session->ok)
> after every call. It's no harder than if (return_value == NULL/-1/0/false).

Well I think it's a big difference. In C a void call will always be
interpreted as a no side effect one. And as Dan pointed out with a mini
example the code really ain't the same. If would not inflict that coding style
on a python person, but that's the normal one for C. You ask for feedback
on C binding, well I tell you, sorry if we disagree.

> Errors should be reported well, because you've got the additional benefit of
> parameterised, internationalisable error messages.
>
> > Point in case I have a domain which
> > doesn't start, failure occurs within xend in XendDomainInfo::initDomain
> > you got a big try:/except: where most of the hard and prone to fail code
> > sit and basically errors are handled only at the end. Result: you just
> > get
> >
> > in initDomain
> > raise VmError(str(exn))
> > VmError: (9, 'Bad file descriptor')
> >
> > as the informations to work with. Sorry encouraging this kind of error
> > handling is a mistake, not an advantage.
>
> That's unfair. Firstly the error comes from the C binding, and is bounced
> across into Python through an interface layer. It's the fact that we only
> have errno available across that interface that the error reporting is so
> poor. Secondly, the fact that you always get "Bad file descriptor" rather
> than a more useful error code is a bug, one that was fixed by your colleague
> Steven Rostedt yesterday.

Okay, but I'm not arguing about this precise case but about the coding
practice in general, if I had a C example that would have been better, I
agree, Python practice is different. Still the 'check and catch fire only
at the end' makes things harder in my opinion.

Daniel

--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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

On 08/08/06 17:00 +0100, Ewan Mellor wrote:
>
>
>Attached is the latest version of the C bindings for the Xen Management API.

Apologize if this is an obvious question, but why the *_decl.h
headers?

More questions later. Ewan, thanks for all the work.

Mike

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

On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:

> > Sorry, that's horrible, because you loose locality of the error.
> > Errors should be detected as soon as possible to be able to report
> > them as accurately as possible.
>
> If you want to detect them as soon as possible, just have if (session->ok)
> after every call. It's no harder than if (return_value == NULL/-1/0/false).

I don't think the issue is whether it's possible to do so, but whether it's
natural. It's rather unnatural IMO to have this out-of-band error notification
in a C API. It's hard enough to get people to check return values when they
exist, never mind when they're implicit. (Yes, people should always strive to
get this right, but the perfect never happens, so it's really all about
minimising the error as much as possible...)

regards
john

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

On Wed, Aug 09, 2006 at 12:52:54PM -0400, Daniel Veillard wrote:
> On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:
> > On Wed, Aug 09, 2006 at 12:12:59PM -0400, Daniel Veillard wrote:
> > > Point in case I have a domain which
> > > doesn't start, failure occurs within xend in XendDomainInfo::initDomain
> > > you got a big try:/except: where most of the hard and prone to fail code
> > > sit and basically errors are handled only at the end. Result: you just
> > > get
> > >
> > > in initDomain
> > > raise VmError(str(exn))
> > > VmError: (9, 'Bad file descriptor')
> > >
> > > as the informations to work with. Sorry encouraging this kind of error
> > > handling is a mistake, not an advantage.
> >
> > That's unfair. Firstly the error comes from the C binding, and is bounced
> > across into Python through an interface layer. It's the fact that we only
> > have errno available across that interface that the error reporting is so
> > poor. Secondly, the fact that you always get "Bad file descriptor" rather
> > than a more useful error code is a bug, one that was fixed by your colleague
> > Steven Rostedt yesterday.
>
> Okay, but I'm not arguing about this precise case but about the coding
> practice in general, if I had a C example that would have been better, I
> agree, Python practice is different. Still the 'check and catch fire only
> at the end' makes things harder in my opinion.

In particular it makes debugging significantly harder. If you get an error
message a and want to correlate it back to the piece of code which triggered
it, you will only be able to correlate a stack trace back to a particular set
of calls. To correlate further back to a specific call will require re-running
the program and single stepping in a debugger. For example if you got say
a 'permissions denied' error from a particular location in the code, you've
no way of determining which of the preceeding 4-5 API calls it was caused
by. The 'clutter' of checking & dealing with errors at the location they
occurr is well worth it, being offset by greater clarity when later debugging
the code. This approach to error handling is best exemplified by the standard
C library, or POSIX system call APIs - checking each & every call you make
may require more code, but it is absolutely key to creating predictable,
reliable & clear to debug programs.

Having each method provide an explicit return status complies with the
'principle of least surprise' - if one sees a method which returns void,
the expected semantics are that is does not have failure conditions.

Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

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

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 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.

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)


#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.


More later, nice work!

Mike

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

On Wed, Aug 09, 2006 at 07:46:10PM -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.
>
> Apologize if this is an obvious question, but why the *_decl.h
> headers?

Many of the record declarations are mutually recursive -- a VM has a reference
to the host on which it resides, and a Host has references to the VMs on that
host, for example. I separated the struct declarations out so that they can
be #included without taking the whole record declaration.

Ewan.


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

On Thu, Aug 10, 2006 at 12:51:57AM +0100, John Levon wrote:

> On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:
>
> > > Sorry, that's horrible, because you loose locality of the error.
> > > Errors should be detected as soon as possible to be able to report
> > > them as accurately as possible.
> >
> > If you want to detect them as soon as possible, just have if (session->ok)
> > after every call. It's no harder than if (return_value == NULL/-1/0/false).
>
> I don't think the issue is whether it's possible to do so, but whether it's
> natural. It's rather unnatural IMO to have this out-of-band error notification
> in a C API. It's hard enough to get people to check return values when they
> exist, never mind when they're implicit. (Yes, people should always strive to
> get this right, but the perfect never happens, so it's really all about
> minimising the error as much as possible...)

OK, how's this:

I think that it's important to treat all return values the same, because I
want someone to be able to look at the Xen API document, the
language-independent one, and for them to be able to infer how the C API
should look. Further, I worry about the case where a value doesn't have a
reasonable sentinel, such as an integer that can reasonably take any value in
the range.

Therefore, rather than

uint64_t result = xen_vm_get_thingy(session, param1, ..., paramn);
if (!session->ok) {
// error handling
}

how about we have instead

uint64_t result;
if (!xen_vm_get_thingy(session, &result, param1, ..., paramn))
{
// error handling
}

Personally, my choice would be for the first, but if the weight of the world
is against me, I can yield ;-)

This second form is no more complicated, so I can change the bindings over.
In particular, this means that there will be no remote call that returns void,
which is the thing that the Daniels didn't like. Calls to the server that
return no result look like this instead:

if (!xen_vm_do_something(session, param1, ..., paramn))
{
// Call failed
}

How's that for an alternative?

Ewan.

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

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@lists.xensource.com
http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-api

On Thu, 2006-08-10 at 10:39 +0100, Ewan Mellor wrote:
> On Wed, Aug 09, 2006 at 07:46:10PM -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.
> >
> > Apologize if this is an obvious question, but why the *_decl.h
> > headers?
>
> Many of the record declarations are mutually recursive -- a VM has a reference
> to the host on which it resides, and a Host has references to the VMs on that
> host, for example. I separated the struct declarations out so that they can
> be #included without taking the whole record declaration.

Thank you! I wish more projects defined structs separately from other
code.

--
Hollis Blanchard
IBM Linux Technology Center


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

>Therefore, rather than
>
>uint64_t result = xen_vm_get_thingy(session, param1, ..., paramn);
>if (!session->ok) {
> // error handling
>}
>
>how about we have instead
>
>uint64_t result;
>if (!xen_vm_get_thingy(session, &result, param1, ..., paramn))
>{
> // error handling
>}
>
>Personally, my choice would be for the first, but if the weight of the
world
>is against me, I can yield ;-)

Without stepping into a 'religious' minefield here... :-) ignoring the
various pros and cons of
both approaches - which I'm not qualified to express an opinion about - I
think the reality is that
the first error handling approach probably wont fly in the broader Xen
devel community - its a bit too atypical C.
So unless there is a significant and obvious advantage to it (and I sense
its more personal preference
between here), I think the latter style that gets away from the session
will get more buyin from developers
and so get them onboard with the new API sooner; one less thing for them to
compain about. :-)

- G




Ewan Mellor
<ewan@xensource.c
Sent by: John Levon <john.levon@sun.com>
xen-api-bounces@l cc
ists.xensource.co Xen-API
m <xen-api@lists.xensource.com>
Subject
Re: [Xen-API] Xen-API C Bindings,
08/10/06 03:55 AM version 0.4.1










On Thu, Aug 10, 2006 at 12:51:57AM +0100, John Levon wrote:

> On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:
>
> > > Sorry, that's horrible, because you loose locality of the error.
> > > Errors should be detected as soon as possible to be able to report
> > > them as accurately as possible.
> >
> > If you want to detect them as soon as possible, just have if
(session->ok)
> > after every call. It's no harder than if (return_value ==
NULL/-1/0/false).
>
> I don't think the issue is whether it's possible to do so, but whether
it's
> natural. It's rather unnatural IMO to have this out-of-band error
notification
> in a C API. It's hard enough to get people to check return values when
they
> exist, never mind when they're implicit. (Yes, people should always
strive to
> get this right, but the perfect never happens, so it's really all about
> minimising the error as much as possible...)

OK, how's this:

I think that it's important to treat all return values the same, because I
want someone to be able to look at the Xen API document, the
language-independent one, and for them to be able to infer how the C API
should look. Further, I worry about the case where a value doesn't have a
reasonable sentinel, such as an integer that can reasonably take any value
in
the range.

Therefore, rather than

uint64_t result = xen_vm_get_thingy(session, param1, ..., paramn);
if (!session->ok) {
// error handling
}

how about we have instead

uint64_t result;
if (!xen_vm_get_thingy(session, &result, param1, ..., paramn))
{
// error handling
}

Personally, my choice would be for the first, but if the weight of the
world
is against me, I can yield ;-)

This second form is no more complicated, so I can change the bindings over.
In particular, this means that there will be no remote call that returns
void,
which is the thing that the Daniels didn't like. Calls to the server that
return no result look like this instead:

if (!xen_vm_do_something(session, param1, ..., paramn))
{
// Call failed
}

How's that for an alternative?

Ewan.

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

On Thu, Aug 10, 2006 at 11:55:16AM +0100, Ewan Mellor wrote:
> On Thu, Aug 10, 2006 at 12:51:57AM +0100, John Levon wrote:
>
> > On Wed, Aug 09, 2006 at 05:26:34PM +0100, Ewan Mellor wrote:
> >
> > > > Sorry, that's horrible, because you loose locality of the error.
> > > > Errors should be detected as soon as possible to be able to report
> > > > them as accurately as possible.
> > >
> > > If you want to detect them as soon as possible, just have if (session->ok)
> > > after every call. It's no harder than if (return_value == NULL/-1/0/false).
> >
> > I don't think the issue is whether it's possible to do so, but whether it's
> > natural. It's rather unnatural IMO to have this out-of-band error notification
> > in a C API. It's hard enough to get people to check return values when they
> > exist, never mind when they're implicit. (Yes, people should always strive to
> > get this right, but the perfect never happens, so it's really all about
> > minimising the error as much as possible...)
>
> OK, how's this:
>
> I think that it's important to treat all return values the same, because I
> want someone to be able to look at the Xen API document, the
> language-independent one, and for them to be able to infer how the C API
> should look. Further, I worry about the case where a value doesn't have a
> reasonable sentinel, such as an integer that can reasonably take any value in
> the range.
>
> Therefore, rather than
>
> uint64_t result = xen_vm_get_thingy(session, param1, ..., paramn);
> if (!session->ok) {
> // error handling
> }

Based solely on grossness, the above makes me sort of ill. It also means
there would need to be some interesting locking on the session object if it
is going to be thread safe. I don't like it.

> how about we have instead
>
> uint64_t result;
> if (!xen_vm_get_thingy(session, &result, param1, ..., paramn))
> {
> // error handling
> }

While a little awkward, it is the very Cish way to do things. It is
especially important that any input params require the caller to manage the
memory, otherwise things get nasty very quickly

> Personally, my choice would be for the first, but if the weight of the world
> is against me, I can yield ;-)
>
> This second form is no more complicated, so I can change the bindings over.
> In particular, this means that there will be no remote call that returns void,
> which is the thing that the Daniels didn't like. Calls to the server that
> return no result look like this instead:
>
> if (!xen_vm_do_something(session, param1, ..., paramn))
> {
> // Call failed
> }
>
> How's that for an alternative?

Yes, much better. :)

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

--
Sean Dague
IBM Linux Technology Center email: japh@us.ibm.com
Open Hypervisor Team alt: sldague@us.ibm.com

On Thu, Aug 10, 2006 at 10:10:56PM -0400, Sean Dague wrote:

> On Thu, Aug 10, 2006 at 11:55:16AM +0100, Ewan Mellor wrote:
>
> [Snip]
>
> > OK, how's this:
> >
> > I think that it's important to treat all return values the same, because I
> > want someone to be able to look at the Xen API document, the
> > language-independent one, and for them to be able to infer how the C API
> > should look. Further, I worry about the case where a value doesn't have a
> > reasonable sentinel, such as an integer that can reasonably take any value in
> > the range.
> >
> > Therefore, rather than
> >
> > uint64_t result = xen_vm_get_thingy(session, param1, ..., paramn);
> > if (!session->ok) {
> > // error handling
> > }
>
> Based solely on grossness, the above makes me sort of ill. It also means
> there would need to be some interesting locking on the session object if it
> is going to be thread safe.

Session objects _definitely_ aren't thread-safe. If you want a multithreaded
client with a session pool or suchlike, then you need to impose a
coarse-grained lock around the session, so that you only use it from one
thread at a time.

> I don't like it.

That seems to be a common theme ;-)

> > how about we have instead
> >
> > uint64_t result;
> > if (!xen_vm_get_thingy(session, &result, param1, ..., paramn))
> > {
> > // error handling
> > }
>
> While a little awkward, it is the very Cish way to do things. It is
> especially important that any input params require the caller to manage the
> memory, otherwise things get nasty very quickly
>
> > Personally, my choice would be for the first, but if the weight of the world
> > is against me, I can yield ;-)
> >
> > This second form is no more complicated, so I can change the bindings over.
> > In particular, this means that there will be no remote call that returns void,
> > which is the thing that the Daniels didn't like. Calls to the server that
> > return no result look like this instead:
> >
> > if (!xen_vm_do_something(session, param1, ..., paramn))
> > {
> > // Call failed
> > }
> >
> > How's that for an alternative?
>
> Yes, much better. :)

Consider it done!

Ewan.

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