API design

[Jef Driesen]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);

/* device.c */

struct device {
/* some data here */
};

int
device_open (device **dev, const char *name)
{
if (dev == NULL)
return -1;

struct device *out = malloc (sizeof (struct device));
if (out == NULL)
return -1;

/* Do some stuff here */

*dev = out;
return 0;
}

As you can see, I made the internal data structure opaque by means of
the typedef and pointers to an incomplete data structure. Now my
questions are:

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

2. I want to add support for a second type of device, which happens to
be very similar to the first one. For instance the read/write functions
are different, but the open/close functions and the contents of the
struct itself are the same. How can I implement this without copying the
code to a new pair of header and source files?

I was thinking to move the shared code to a common header and source
file and make the device-specific functions call the common functions.
But what should I do with the typdefs below, because both structs would
be the same in my case. But I would like to hide that from the user of
the library.

/* common.h (for internal use only) */

struct device {
/* some data here */
};

int device_open (device **dev, const char *name);

/* device1.h (public) */
typedef struct device1 device1;
int device1_open (device1 **dev, const char *name);
....

/* device2.h (public) */
typedef struct device2 device2;
int device1_open (device1 **dev, const char *name);
....

 

[Malcolm McLean]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);

Take a book out of the standard library.

FILE *fp = fopen("filename.txt", "w");
if(!fp)
/* it failed */
fprintf(fp, "This file works\n");
fclose(fp);

Similarly

typedef struct device DEVICE;

DEVICE *dev = device_open("Fred");
if(!dev)
/* your device didn't open */
device_write(dev, "my name is Fred", 15);
device_close(dev);

 

[cr88192]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a header
and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);

device_open is ugly, IMO:
device *device_open (const char *name);
would be a little less ugly.

/* device.c */

struct device {
/* some data here */
};

int
device_open (device **dev, const char *name)
{
if (dev == NULL)
return -1;

struct device *out = malloc (sizeof (struct device));
if (out == NULL)
return -1;

/* Do some stuff here */

*dev = out;
return 0;
}

As you can see, I made the internal data structure opaque by means of the
typedef and pointers to an incomplete data structure. Now my questions
are:

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

this may be acceptable (I actually do similar with a void pointer at one
point):
typedef void *dyt;

where dyt hides that it is a pointer.
this is a personal preference though.

in my case, 'dyt' is a dynamic type for a specific 'dynamic type' API of
mine...

2. I want to add support for a second type of device, which happens to be
very similar to the first one. For instance the read/write functions are
different, but the open/close functions and the contents of the struct
itself are the same. How can I implement this without copying the code to
a new pair of header and source files?

a typical approach is to make use of structs filled with function pointers.
these exist per-type, and are referenced by the specific open instances.

struct device_info_s {
int (*close)(device *dev);
int (*read)(device *dev, void *data, unsigned int size);
int (*write)(device *dev, const void *data, unsigned int size);
};

the frontend functions then dispatch through this struct.

int device_close (device *dev)
{
if(dev->info->close)
return(dev->info->close(dev));
return(-1);
}

this allows plugging lots of different subsystems into the same interface,
and is especially useful, for example, in things like custom VFS subsystems,
....

for example, I did this at one point for a VFS API I called 'VFILE', and,
apart from the fact that I rewrote it at one point (the internals, the
frontend API remained more or less unchanged), this API has remained in use
for at least around a decade (this is pretty much some of the oldest code in
my projects, and one of my first 'formalized' APIs).

from what I remember, I originally wrote it long ago for an RDBMS type
project (this was back when I was in middle school or so I think...). (damn,
middle school was a long time ago...).

I was thinking to move the shared code to a common header and source file
and make the device-specific functions call the common functions. But what
should I do with the typdefs below, because both structs would be the same
in my case. But I would like to hide that from the user of the library.

/* common.h (for internal use only) */

struct device {
/* some data here */
};

int device_open (device **dev, const char *name);

/* device1.h (public) */
typedef struct device1 device1;
int device1_open (device1 **dev, const char *name);
...

/* device2.h (public) */
typedef struct device2 device2;
int device1_open (device1 **dev, const char *name);
...

unnecessary API fragmentation is ugly, especially if you might expect more
of it in the future...

 

[Jef Driesen]

Take a book out of the standard library.

FILE *fp = fopen("filename.txt", "w");
if(!fp)
/* it failed */
fprintf(fp, "This file works\n");
fclose(fp);

Similarly

typedef struct device DEVICE;

DEVICE *dev = device_open("Fred");
if(!dev)
/* your device didn't open */
device_write(dev, "my name is Fred", 15);
device_close(dev);

My question was not about the discussion to return the pointer with the
return value or with an output parameter. I did choose the last option
to be able to return an error code directly. This also makes the error
handling of the open function similar to the other functions.

 

[Eric Sosman]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);
[...]

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

"Moving the star" from the parameter lists to the typedef
is perhaps an improvement, but a pretty small one. I think a
better course might be to leave the typedef as it is, and use
`device *device_open(const char *name)' with the convention that
NULL is returned on a failure. This idiom will be familiar to
all C programmers, following the existing model of fopen() and
the like -- but in order to follow it, you need to reveal the
pointer-nature of the returned value.

Keep in mind that hiding implementation details is a means,
not an end in itself. When you're wondering whether to hide
something, think about why you want to keep it hidden.

2. I want to add support for a second type of device, which happens to
be very similar to the first one. For instance the read/write functions
are different, but the open/close functions and the contents of the
struct itself are the same. How can I implement this without copying the
code to a new pair of header and source files?

You could maintain a type indicator in the `struct device'.
One particularly flexible indicator is a set of function pointers:

struct device {
...
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
...
};

If there are a lot of such pointers, you might want to use one
more level of indirection:

struct funcs {
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
int (*scribble)(device*, void*, unsigned int);
int (*doodle)(device*, void*, unsigned int);
int (*spray)(device*, PaintColor);
};

static const struct funcs type1 = {
read1, write1, scribble1, doodle1, sprayany };
static const struct funcs type2 = {
read2, write2, scribble2, doodle2, sprayany };

struct device {
...
const struct funcs *funcs; /* to type1 or type2 */
...
};

As an aside, why use `unsigned int' for what appear to be
counts, instead of `size_t'? That's what it's for, after all.

 

[Roland Pibinger]

device_open is ugly, IMO:
device *device_open (const char *name);
would be a little less ugly.

alternatively:

struct device_handle {
device* imp;
} device_handle;
typedef struct device_handle device_handle;

device_handle device_open (const char *name);

a typical approach is to make use of structs filled with function pointers.
these exist per-type, and are referenced by the specific open instances.

or just a 'switch on type' which invisible for the user, too.

 

[Roland Pibinger]

alternatively:

struct device_handle {
device* imp;
} device_handle;
typedef struct device_handle device_handle;

Should have been:

struct device_handle {
device* imp;
};
typedef struct device_handle device_handle;

 

[Chris Thomasson]

a typical approach is to make use of structs filled with function
pointers.
these exist per-type, and are referenced by the specific open instances.

struct device_info_s {
int (*close)(device *dev);
int (*read)(device *dev, void *data, unsigned int size);
int (*write)(device *dev, const void *data, unsigned int size);
};

the frontend functions then dispatch through this struct.

int device_close (device *dev)
{
if(dev->info->close)
return(dev->info->close(dev));
return(-1);
}

this allows plugging lots of different subsystems into the same interface,
and is especially useful, for example, in things like custom VFS
subsystems, ...

[...]

FWIW, here is a way to do minimalist interfaces in C:

http://groups.google.com/group/comp.lang.c/browse_frm/thread/1b106926ba5db19f

 

[cr88192]

a typical approach is to make use of structs filled with function
pointers.
these exist per-type, and are referenced by the specific open instances.

struct device_info_s {
int (*close)(device *dev);
int (*read)(device *dev, void *data, unsigned int size);
int (*write)(device *dev, const void *data, unsigned int size);
};

the frontend functions then dispatch through this struct.

int device_close (device *dev)
{
if(dev->info->close)
return(dev->info->close(dev));
return(-1);
}

this allows plugging lots of different subsystems into the same
interface, and is especially useful, for example, in things like custom
VFS subsystems, ...

[...]

FWIW, here is a way to do minimalist interfaces in C:

http://groups.google.com/group/comp.lang.c/browse_frm/thread/1b106926ba5db19f

yes.
this does not seem too much different from what I was suggesting here.


this is one way to do OO/abstract APIs.
actually, often in such APIs (my case), one can recognize that there are
often multiple paths to the same result, so, trying to use one method, if it
is not present, the wrapper may look into a few other possibilities
(fallback cases), before finally returning error.

an example in my 'VFILE' API (ok, actually for the most part a shameless rip
of stdio, as nearly everything is the same except for a 'V' or 'v' prefix on
everything, though in a few cases I used 'vf' as well, or used 'vf' as a
suffix...).

this was done so that it would be really easy to convert code to/from the
stdio file interface (though slightly limited, as this idea was developed
before I really discovered search-and-replace code-conversion, which would
mandate a slightly different naming convention to make work well, but
search-replace followed by more search-replace to deal with conversion
issues often works good enough...).

or such...

 

[Barry Schwarz]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);

/* device.c */

struct device {
/* some data here */
};

int
device_open (device **dev, const char *name)
{
if (dev == NULL)
return -1;

struct device *out = malloc (sizeof (struct device));
if (out == NULL)
return -1;

/* Do some stuff here */

*dev = out;
return 0;
}

As you can see, I made the internal data structure opaque by means of
the typedef and pointers to an incomplete data structure. Now my
questions are:

1. Is there any advantage to change the typedef to

typedef struct device *device;

IMO, no advantage and a major disadvantage. If you make the change,
then code declaring a pointer will look like
device name;
which will inevitably at some point in the future lead some
unsuspecting maintenance programmer to believe he needs to add an &
when he passes name to some function expecting a pointer.

If you still decide to do it, change the typedef name from *device to
something like *device_ptr_t to give this unfortunate heir a hint.

and thus hiding the fact that the "device" type is actually a pointer.

Hide only the details that are worth hiding. As has already been
suggested, FILE is a precedent worth following.

2. I want to add support for a second type of device, which happens to
be very similar to the first one. For instance the read/write functions
are different, but the open/close functions and the contents of the
struct itself are the same. How can I implement this without copying the
code to a new pair of header and source files?

Again, FILE and the standard library show a reasonable approach. For
the things that are common, such as the struct declaration and the
open and close functions, you only need one. For the things that are
specific to a particular device (read and write functions), you need
unique versions. But the prototypes for these unique functions can
(not must) still be in a common header.

On my system, the standard headers include both the public and the
opaque information. You can't really hide the opaque data from the
programmer since he has to be able to read the headers to perform the
compile. It becomes a question of how much he has to wade through to
find the information.

If one program is capable of dealing with two device types, you have
the risk of a common structure associated with the second device being
passed to a function dedicated to the first device. Two typedef's
aliasing the same struct type will not solve this problem. If you
include in the structure a member indicating which of the possible
devices this instance of the structure applies to, your device
dependent routines can check before they do any real work.

I was thinking to move the shared code to a common header and source
file and make the device-specific functions call the common functions.

Putting code in a header is almost always a mistake. Maybe that's not
what you meant.

But what should I do with the typdefs below, because both structs would
be the same in my case. But I would like to hide that from the user of
the library.

/* common.h (for internal use only) */

struct device {
/* some data here */
};

int device_open (device **dev, const char *name);

/* device1.h (public) */
typedef struct device1 device1;
int device1_open (device1 **dev, const char *name);
...

/* device2.h (public) */
typedef struct device2 device2;
int device1_open (device1 **dev, const char *name);
...

A single header: device.h

typedef struct device
{
...
enum type {device_1, device_2, ...};
...
} device;
device* device_open(...);
device* device_close(...);
int device_1_read(...);
...

A source file for each function, such as device_open.c

#include <device.h>
#include <stdlib.h>
device* device_open(...)
{
device *ptr = malloc(...);
if (ptr != NULL)
{
...
}
return ptr;
{

In device dependent functions, such as device_1_read

int device_1_read(device *ptr, ......)
{
if (ptr->type != device_1)
/*error handler*/
...
}

Whenever a device is opened

dev = device_open(...);
if (dev == NULL)
/* error handler */
dev_type = device_1; /*or _2 or whatever */


Remove del for email

 

[CBFalconer]

device_open is ugly, IMO:
device *device_open (const char *name);
would be a little less ugly.

Only if the only error indication needed is failure vs success.

 

[Barry Schwarz]

alternatively:

struct device_handle {
device* imp;
} device_handle;

Why would you want a struct with only a single member? It just adds a
bunch of unneeded verbiage to both writing and reading the code.

<snip>


Remove del for email

 

[Richard]

Why would you want a struct with only a single member? It just adds a
bunch of unneeded verbiage to both writing and reading the code.

Plain common sense. Its either (a) stripped down for this example or (b) a
place holder for when the struct expands.

 

[Jef Driesen]

a typical approach is to make use of structs filled with function pointers.
these exist per-type, and are referenced by the specific open instances.

struct device_info_s {
int (*close)(device *dev);
int (*read)(device *dev, void *data, unsigned int size);
int (*write)(device *dev, const void *data, unsigned int size);
};

the frontend functions then dispatch through this struct.

int device_close (device *dev)
{
if(dev->info->close)
return(dev->info->close(dev));
return(-1);
}

this allows plugging lots of different subsystems into the same interface,
and is especially useful, for example, in things like custom VFS subsystems,
...

I think the solution you propose is useful if both devices share the
same interface, but with a different underlying implementation. And in
that case your solution allows to access both devices through a common
api. However in my situation, that is not entirely the case. Only some
functions of the devices are similar enough to be accessed through a
common api (like open/close/read/write), but not all functions on one
device have an equivalent on the other device.

So I do not necessary need a common interface. But there is some code
that is the same for both devices (like setting baudrate, timeouts,...)
and I want to re-use it, rather than duplicating it and thus needing to
maintain the same code it in two different locations.

unnecessary API fragmentation is ugly, especially if you might expect more
of it in the future...

What do you mean with "API fragmentation"?

 

[Jef Driesen]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);
[...]

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

"Moving the star" from the parameter lists to the typedef
is perhaps an improvement, but a pretty small one. I think a
better course might be to leave the typedef as it is, and use
`device *device_open(const char *name)' with the convention that
NULL is returned on a failure. This idiom will be familiar to
all C programmers, following the existing model of fopen() and
the like -- but in order to follow it, you need to reveal the
pointer-nature of the returned value.

Returning NULL on failure makes it more difficult to return the reason
of the failure. With the output parameter I can return an error code to
indicate the reason of the failure.

Keep in mind that hiding implementation details is a means,
not an end in itself. When you're wondering whether to hide
something, think about why you want to keep it hidden.

One of the reasons is to prevent the user of the library to mess with
the internals of the library. And the other reasons is compatibility. If
the contents of the struct is not part of the public api, I can change
it without breaking existing programs.

You could maintain a type indicator in the `struct device'.
One particularly flexible indicator is a set of function pointers:

struct device {
...
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
...
};

If there are a lot of such pointers, you might want to use one
more level of indirection:

struct funcs {
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
int (*scribble)(device*, void*, unsigned int);
int (*doodle)(device*, void*, unsigned int);
int (*spray)(device*, PaintColor);
};

static const struct funcs type1 = {
read1, write1, scribble1, doodle1, sprayany };
static const struct funcs type2 = {
read2, write2, scribble2, doodle2, sprayany };

struct device {
...
const struct funcs *funcs; /* to type1 or type2 */
...
};

This is the same idea as posted by cr88192. See my comments there.

As an aside, why use `unsigned int' for what appear to be
counts, instead of `size_t'? That's what it's for, after all.

No particular reason. I didn't need a datatype that supports large
counts, since the devices have only limited memory to read/write (only a
few KB).

 

[Roland Pibinger]

Why would you want a struct with only a single member? It just adds a
bunch of unneeded verbiage to both writing and reading the code.

For more type safety and for making clear your intentions. You cannot
accidentally convert the handle to another type (pointers silently
convert from/to void*). Simple casts to another type don't work, too.

 

[Eric Sosman]

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);
[...]

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

"Moving the star" from the parameter lists to the typedef
is perhaps an improvement, but a pretty small one. I think a
better course might be to leave the typedef as it is, and use
`device *device_open(const char *name)' with the convention that
NULL is returned on a failure. This idiom will be familiar to
all C programmers, following the existing model of fopen() and
the like -- but in order to follow it, you need to reveal the
pointer-nature of the returned value.

Returning NULL on failure makes it more difficult to return the reason
of the failure. With the output parameter I can return an error code to
indicate the reason of the failure.

If that's an important concern, then I'd suggest you go ahead
and "cloak" the pointer-nature of the type. Callers will be using
other channels to learn about errors, and so will not need to test
returned values against NULL.

One of the reasons is to prevent the user of the library to mess with
the internals of the library. And the other reasons is compatibility. If
the contents of the struct is not part of the public api, I can change
it without breaking existing programs.

Right. But a pointer to an incomplete type doesn't divulge
the details of the type's internals; you can still rearrange
things at will.

No particular reason. I didn't need a datatype that supports large
counts, since the devices have only limited memory to read/write (only a
few KB).

If you're sure you'll never need to support the FrammisVogel
MaxiFlex 4200 in hyper-extended multi-blivet mode, then go ahead.
True, the programming details of the 4200 are not yet available --
heck, the device itself isn't due to be invented until late next
year -- but since Halo 5 requires it, it'll be all over the
marketplace before you can blink ...

If the history of computing teaches us anything, it's that
Things Get Bigger. The original IBM S/360 used a 24-bit address,
enough to point at more memory than you could stuff into half
a dozen of the very largest supercomputers IBM built. With an
eye to economy, lots of code stored addresses in a 32-bit word's
low-order three bytes and stuffed flags and things into the high-
order byte. Can you *imagine* how expensive that "economy" move
turned out to be?

The only reasons I know of for avoiding `size_t' are (1) you
can't co-opt negative values as in-band status indicators, and
(2) it may use too much memory if you're storing billyuns and
billyuns of small counts. In your case (1) apparently doesn't
apply (you're already using an unsigned type), so unless (2) is
a concern I'd strongly recommend `size_t'. When multi-blivet
mode becomes de rigeur you will thank yourself.

 

[cr88192]

cr88192 wrote:

I think the solution you propose is useful if both devices share the same
interface, but with a different underlying implementation. And in that
case your solution allows to access both devices through a common api.
However in my situation, that is not entirely the case. Only some
functions of the devices are similar enough to be accessed through a
common api (like open/close/read/write), but not all functions on one
device have an equivalent on the other device.

So I do not necessary need a common interface. But there is some code that
is the same for both devices (like setting baudrate, timeouts,...) and I
want to re-use it, rather than duplicating it and thus needing to maintain
the same code it in two different locations.

ok.


What do you mean with "API fragmentation"?

this is where you end up with multiple incompatible APIs doing essentially
the same thing.

in non-trivial cases, this can make a horrible mess of things.

one has to keep track of all sorts of stuff, like different datatypes, API
functions, needing to implement "converters" to move data from one part of
the codebase to another, or even worse, to try to share the data and keep it
in sync, ...

IMO, it is far better to try to keep everything clean and unified.


as a codebase gets larger, this kind of thing can become very important, as,
say, changing some fundamental piece of the architecture, when you have,
say, several hundred kloc depending on it, can become very expensive (or, if
one is especially unlucky, break the entire project).

also, uniform APIs also make it a lot easier to reorganize things, as one
can (maybe even drastically) change the workings of one subsystem, while
still maintaining the internal integrity of others (wheras, with a much more
ad-hoc project, even minor changes can break things in an entirely different
subsystem).

so, not only should APIs be uniform, but they should also be opaque (aka: no
allowing "front end" code to dig around in the internals, instead it is
given a specific set of abstract types and functions, with each type and
function having a specific and well-defined task).

as such, we maintain a system of "invariants" that an API is held to, and
are free to do pretty much anything we want "under the hood" so long as
these basic rules are upheld...


so, it is better to try to adapt some semblance of "clean" practices early
on, rather than setting out to implement some horrible ad-hoc monstrosity...

it may make things slower, but IMO it pays off...


that is not to say though that sometimes fragmentation is not inevitable, or
some kind of holy grail above all other concerns, but it should at least be
tried for in general.