[PATCH] 1/1 Better expose xenstore limits

[PATCH] 1/1 Better expose xenstore limits

[Tim Post]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

Hi,

The attached patch announces the limits regarding:

XENSTORE_PAYLOAD_MAX, XENSTORE_ABS_PATH_MAX and XENSTORE_REL_PATH_MAX in
the public xs.h.

These are already defined in xs_wire, however, not many people dig that
far when using xs.h.

Since xs.h already includes xs_wire, this patch duplicates those limits
as XS_PAYLOAD_MAX, XS_ABS_PATH_MAX and XS_REL_PATH_MAX. Moving these
would be a pain, but they really need to be advertised.

Such limits should be obvious in the headers that people actually use
outside of the kernel, saving them from wondering what went wrong.

Added comments direct people to xs_wire.h , in case those limits change.

Signed-off-by: Tim Post <echo@echoreply.us>



[-- Attachment #2: xenstore-expose-limits.patch --]
[-- Type: text/x-patch, Size: 486 bytes --]

diff -r dc5bd14a4675 tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Fri Apr 24 13:46:17 2009 +0100
+++ b/tools/xenstore/xs.h	Thu Apr 30 00:39:14 2009 +0800
@@ -23,6 +23,11 @@
 #include <xs_lib.h>
 
 #define XBT_NULL 0
+
+/* Please see xen/io/xs_wire.h for more explantion on these limits */
+#define XS_PAYLOAD_MAX XENSTORE_PAYLOAD_MAX
+#define XS_ABS_PATH_MAX XENSTORE_ABS_PATH_MAX
+#define XS_REL_PATH_MAX XENSTORE_REL_PATH_MAX
 
 struct xs_handle;
 typedef uint32_t xs_transaction_t;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] 1/1 Better expose xenstore limits

[Keir Fraser]

On 29/04/2009 17:43, "Tim Post" <echo@echoreply.us> wrote:

> Since xs.h already includes xs_wire, this patch duplicates those limits
> as XS_PAYLOAD_MAX, XS_ABS_PATH_MAX and XS_REL_PATH_MAX. Moving these
> would be a pain, but they really need to be advertised.
> 
> Such limits should be obvious in the headers that people actually use
> outside of the kernel, saving them from wondering what went wrong.
> 
> Added comments direct people to xs_wire.h , in case those limits change.

In terms of documentation, docs/misc/xenstore.txt already does a fine job.
And I think a xen/public/... header is the right place for the limits, since
it is a general documented protocol-level constraint, rather than a
constraint of the particular client library (which is the scope of xs.h
itself).

 -- Keir

Re: [PATCH] 1/1 Better expose xenstore limits

[Tim Post]

On Wed, 2009-04-29 at 19:01 +0100, Keir Fraser wrote:
> On 29/04/2009 17:43, "Tim Post" <echo@echoreply.us> wrote:
> 
> > Since xs.h already includes xs_wire, this patch duplicates those limits
> > as XS_PAYLOAD_MAX, XS_ABS_PATH_MAX and XS_REL_PATH_MAX. Moving these
> > would be a pain, but they really need to be advertised.
> > 
> > Such limits should be obvious in the headers that people actually use
> > outside of the kernel, saving them from wondering what went wrong.
> > 
> > Added comments direct people to xs_wire.h , in case those limits change.
> 
> In terms of documentation, docs/misc/xenstore.txt already does a fine job.

Provided such docs are actually packaged.. many install Xen (and
associated DSO's and headers) via yum, apt-get, etc.

> And I think a xen/public/... header is the right place for the limits, since
> it is a general documented protocol-level constraint, rather than a
> constraint of the particular client library (which is the scope of xs.h
> itself).

Yes, it is the right place for the limits, the patch does not suggest
moving them, only echoing them in the header that user space apps will
include.

Someone using xs.h is going to be entirely unaware of said limits.
Xenstore internals will relay an appropriate errno if the limits are
exceeded, but I think many people will not know such limits exist.

Xenwire is used when developing drivers.. not user space applications.

Why not assert the limits in both at the cost of bothering the
preprocessor three times?

Cheers,
--Tim

Re: [PATCH] 1/1 Better expose xenstore limits

[Keir Fraser]

On 29/04/2009 19:18, "Tim Post" <echo@echoreply.us> wrote:

> Someone using xs.h is going to be entirely unaware of said limits.
> Xenstore internals will relay an appropriate errno if the limits are
> exceeded, but I think many people will not know such limits exist.
> 
> Xenwire is used when developing drivers.. not user space applications.
> 
> Why not assert the limits in both at the cost of bothering the
> preprocessor three times?

Defining the same thing again but with a different name just for purposes of
documentation is pretty barking. I suppose it is perhaps worth a
documentation comment in xs.h, but no more than that.

 -- Keir

Re: [PATCH] 1/1 Better expose xenstore limits

[Tim Post]

On Wed, 2009-04-29 at 22:23 +0100, Keir Fraser wrote:
> On 29/04/2009 19:18, "Tim Post" <echo@echoreply.us> wrote:
> 
> > Someone using xs.h is going to be entirely unaware of said limits.
> > Xenstore internals will relay an appropriate errno if the limits are
> > exceeded, but I think many people will not know such limits exist.
> > 
> > Xenwire is used when developing drivers.. not user space applications.
> > 
> > Why not assert the limits in both at the cost of bothering the
> > preprocessor three times?
> 
> Defining the same thing again but with a different name just for purposes of
> documentation is pretty barking. I suppose it is perhaps worth a
> documentation comment in xs.h, but no more than that.

It is worth a mention... though it seems silly to send a patch that adds
comments. However, a patch that would convert the comments in xs to
doxygen format seems more useful, with the appropriate grouping, shorts,
etc.

How receptive would you be to that?

Cheers,
--Tim

Re: [PATCH] 1/1 Better expose xenstore limits

[Keir Fraser]

On 30/04/2009 09:54, "Tim Post" <echo@echoreply.us> wrote:

>> Defining the same thing again but with a different name just for purposes of
>> documentation is pretty barking. I suppose it is perhaps worth a
>> documentation comment in xs.h, but no more than that.
> 
> It is worth a mention... though it seems silly to send a patch that adds
> comments. However, a patch that would convert the comments in xs to
> doxygen format seems more useful, with the appropriate grouping, shorts,
> etc.
> 
> How receptive would you be to that?

I don't have a strong opinion. You can go wild on that if you like, assuming
noone else hates doxygen format for any reason. I added a limits-related
comment to xs.h by the way.

 -- Keir

Re: [PATCH] 1/1 Better expose xenstore limits

[Tim Post]

On Thu, 2009-04-30 at 10:35 +0100, Keir Fraser wrote:
> On 30/04/2009 09:54, "Tim Post" <echo@echoreply.us> wrote:
> 
> >> Defining the same thing again but with a different name just for purposes of
> >> documentation is pretty barking. I suppose it is perhaps worth a
> >> documentation comment in xs.h, but no more than that.
> > 
> > It is worth a mention... though it seems silly to send a patch that adds
> > comments. However, a patch that would convert the comments in xs to
> > doxygen format seems more useful, with the appropriate grouping, shorts,
> > etc.
> > 
> > How receptive would you be to that?
> 
> I don't have a strong opinion. You can go wild on that if you like, assuming
> noone else hates doxygen format for any reason. I added a limits-related
> comment to xs.h by the way.

Well, since we have doxygen in the build, and the generated html docs
are so easy to read .. seems sort of silly not to make it uniform :)

If nobody replies with a resounding PLEASE NO, I'll go ahead and do
it .. libxc needs some comment love anyway.

It is very frustrating and difficult for people to write apps that work
with Xen using the shipped (or wiki) documentation. All too often,
what's on the wiki is not in step with unstable, so each new release is
like an easter egg hunt.

I also have some autoconf macros that I'll post to help stop breakage
due to structures shifting members. from the earlier days of v3 (but
still in use).

Cheers,
--Tim