[RFC] [PATCH v2 1/12] [resourceproto] Added protocol description and records for XRes v1.2

[Daniel Stone]

Hi,
Mainly nitpicks, I'm afraid ...

On Fri, Dec 31, 2010 at 03:32:39PM +0200, Erkki Seppälä wrote:
> +#define X_XResClientXidMask      0x01
> +#define X_XResLocalClientPidMask 0x02

Maybe make these XID and PID?

> +typedef struct _XResClientIdSpec {
> +   CARD32  client B32;
> +   CARD32  mask B32;
> +} xXResClientIdSpec;
> +#define sz_xXResClientIdSpec 8
> +
> +typedef struct _XResClientIdValue {
> +   xXResClientIdSpec spec;
> +   CARD32  length B32;
> +   // followed by length CARD32s
> +} xXResClientIdValue;
> +#define sz_xResClientIdValue (sz_xXResClientIdSpec + 4)

Please don't use C++ comments (// instead of /* */).

> +typedef struct _XResQueryClientIds {
> +   CARD8   reqType;
> +   CARD8   XResReqType;
> +   CARD16  length B16;
> +   CARD32  numSpecs B32;
> +   // followed by numSpecs times XResClientIdSpec
> +} xXResQueryClientIdsReq;
> +#define sz_xXResQueryClientIdsReq 8
> 
> +typedef struct {
> +   CARD8   type;
> +   CARD8   pad1;
> +   CARD16  sequenceNumber B16;
> +   CARD32  length B32;
> +   CARD32  numIds B32;
> +   CARD32  pad2 B32;
> +   CARD32  pad3 B32;
> +   CARD32  pad4 B32;
> +   CARD32  pad5 B32;
> +   CARD32  pad6 B32;
> +   // followed by numIds times XResClientIdValue
> +} xXResQueryClientIdsReply;
> +#define sz_xXResQueryClientIdsReply  32

I think the nested approach (reply contains n * XResClientIdValue, which
contains m * CARD32) makes it more difficult to write an XCB binding for
this, though maybe that's not the case these days.

> +typedef struct _XResResourceSizeSpec {
> +   xXResResourceIdSpec spec;
> +   CARD32  bytes B32;
> +   CARD32  refCount B32;
> +   CARD32  useCount B32;
> +} xXResResourceSizeSpec;
> +#define sz_xXResResourceSizeSpec (sz_xXResResourceIdSpec + 12)
> +
> +typedef struct _XResResourceSizeValue {
> +   xXResResourceSizeSpec size;
> +   CARD32  numCrossReferences B32;
> +   // followed by numCrossReferences times XResResourceSizeSpec
> +} xXResResourceSizeValue;
> +#define sz_xXResResourceSizeValue (sz_xXResResourceSizeSpec + 4)
> +
> +typedef struct {
> +   CARD8   type;
> +   CARD8   pad1;
> +   CARD16  sequenceNumber B16;
> +   CARD32  length B32;
> +   CARD32  numSizes B32;
> +   CARD32  pad2 B32;
> +   CARD32  pad3 B32;
> +   CARD32  pad4 B32;
> +   CARD32  pad5 B32;
> +   CARD32  pad6 B32;
> +   // followed by numSizes times XResResourceSizeValue
> +} xXResQueryResourceBytesReply;
> +#define sz_xXResQueryResourceBytesReply  32

And same here.

> +ref_count
> +    Number of total users of the resource. Typically the reference
> +    count is 1 but for pixmaps the count may be larger.

Only for pixmaps?

> +This request is used to get the pixmap usage of some client. The
> +returned number is a sum of memory usage of each pixmap that can be
> +attributed to the given client. Ideally the server goes through all
> +pixmaps and divides each pixmap size by the pixmap reference count to
> +get a pixmap reference size. The reference size is then added to the
> +returned sum if the client happens to be referencing that pixmap. In
> +practice some pixmap references may be missed, because it would be too
> +difficult to keep track of all pixmap references. However, the server
> +will check the most important client resources that are using pixmaps
> +and tries to estimate the pixmap usage as well as is possible.

Most of the last half could just be written as 'The server need only
make a best-effort attempt to calculate resource source, so actual
resource size may differ from that reported in practice.'

> +The CLIENTIDSPEC of request and CLIENTIDSPEC of CLIENTIDVALUE in reply
> +match each other. [...]
> +
> +However, the CLIENTIDSPEC of returned CLIENTIDVALUE never contains any
> +wildcards. If the request used a wildcard to specify all clients in a
> +single CLIENTIDSPEC, then the reply has expanded the wildcard and
> +returns separate CLIENTIDVALUE records for each client. [...]

These sentences are somewhat contradictory; maybe add an 'In general',
or 'Usually', or such to the first paragraph, so it's clear that there
is an exception.

> +client
> +    An ID of a client can be given to limit the query to resources of
> +    that client. Just like in CLIENTIDSPEC, any resource ID can be
> +    given to identify a client and None can be used if the query
> +    shouldn't be limited to a specific client. Note that in some cases
> +    this field is redundant because resource_specs already fully
> +    determines which resources are selected.

What happens if a CLIENTIDSPEC specifies a client different to the owner
of a fully-specified resource in resource_specs? Is that resource just
ignored and not reported, or does an error result?

Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101231/6ab7571b/attachment-0001.pgp>