Re: [PATCH 06/14] DRBD: userspace_interface

[Bart Van Assche]

On Fri, Apr 10, 2009 at 2:12 PM, Philipp Reisner
<philipp.reisner@xxxxxxxxxx> wrote:
> diff -uNrp linux-2.6.30-rc1/include/linux/drbd.h linux-2.6.30-rc1-drbd/include/linux/drbd.h
> --- linux-2.6.30-rc1/include/linux/drbd.h       1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.30-rc1-drbd/include/linux/drbd.h  2009-03-26 15:53:46.520275000 +0100

...

> +#include <linux/drbd_config.h>

By including drbd_config.h in drbd.h all definitions in the former
header file become visible in user space. Several definitions in
drbd_config.h only make sense inside the kernel. Either the above
#include directive should be removed or drbd_config.h should be
cleaned up.

...

> +/* Altough the Linux source code makes a difference between
> +   generic endiness and the bitfields' endianess, there is no
> +   architecture as of Linux-2.6.24-rc4 where the bitfileds' endianess
> +   does not match the generic endianess. */

I assume this should have been four times "endianness", just like below ?

> +
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +#define __LITTLE_ENDIAN_BITFIELD
> +#elif __BYTE_ORDER == __BIG_ENDIAN
> +#define __BIG_ENDIAN_BITFIELD
> +#else
> +# error "sorry, weird endianness on this box"
> +#endif

...

> +/* KEEP the order, do not delete or insert!
> + * Or change the API_VERSION, too. */
> +enum ret_codes {
> +       RetCodeBase = 100,
> +       NoError,         /* 101 ... */
> +       LAAlreadyInUse,

How will backwards compatibility for return codes be ensured ? The
comment before the enum probably has to be changed to "KEEP the order,
do not delete or insert!" only ?

...

> +union drbd_state_t {
> +/* According to gcc's docs is the ...
> + * The order of allocation of bit-fields within a unit (C90 6.5.2.1, C99 6.7.2.1).
> + * Determined by ABI.
> + * pointed out by Maxim Uvarov q<muvarov@xxxxxxxxxxxxx>
> + * even though we transmit as "cpu_to_be32(state)",
> + * the offsets of the bitfields still need to be swapped
> + * on different endianess.
> + */

The above comment really looks strange. Is it still up to date ?

...

> +/* from drbd_strings.c */
> +extern const char *conns_to_name(enum drbd_conns);
> +extern const char *roles_to_name(enum drbd_role);
> +extern const char *disks_to_name(enum drbd_disk_state);
> +extern const char *set_st_err_name(enum set_st_err);

Should declarations of kernel functions really be present in a user
space interface header ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/