Re: [PATCH] Minimize sysdeps code involved in defining major/minor/makedev.

[Mike Frysinger <vapier at gentoo dot org>]

On 28 Apr 2016 21:39, Zack Weinberg wrote:
> New ports henceforth need only provide bits/sysmacros.h defining
> macros __dev_makedev, __dev_major, and __dev_minor, and it's OK if
> these macros evaluate their arguments more than once.

as a general policy, i don't think we want that.

> 1) There seem to be generic "bits" headers in a top-level directory
> named bits/, as well as sysdeps/generic/bits/ which is where I would
> expect them to be.  What's up with that?

i suspect it's something like:
- bits/: The One True header that is used everywhere and overriding
  doesn't make sense.
- sysdeps/generic/bits/: We expect platforms to override it, so throw
  it in here to signal as much.

> 2) On all supported platforms except Hurd, dev_t is either __UQUAD_TYPE
> or __U64_TYPE; these are both 64-bit unsigned integer types (I don't
> fully understand the distinction; the comments in bits/types.h are
> inconsistent with the code).  The generic sysmacros.h, however, only
> supports what I'll call the "traditional" dev_t encoding, with 8 bits
> each for major and minor number.  Does it maybe make sense to promote
> the extended dev_t encoding glibc uses on all Linux-based
> configurations, which supports 32-bit major and minor numbers, to
> generic?  Hurd would of course need an override, but that's not
> difficult provided someone tells me what Hurd's dev_t encoding actually
> is (also, it's probably wrong as is).  (Note that Linux-the-kernel
> honors only 12-bit major and 20-bit minor numbers; glibc's encoding is
> an upward compatible extension of *that*.)

i think making it 64-bit as a GNU extesion/base makes sense

> --- /dev/null
> +++ b/misc/tst-makedev.c
> @@ -0,0 +1,87 @@
> +#include <sys/types.h>

all new files need a comment block at the top with:
- one line overview blurb
- copyright
- license

> +/* Confirm that makedev (major (d), minor (d)) == d. */

GNU style says two spaces after periods.  comes up a few times.

> +static int
> +do_test_split_combine (dev_t d1)
> +{
> +  unsigned int maj = major (d1);
> +  unsigned int min = minor (d1);
> +  dev_t d2 = makedev (maj, min);
> +  if (d1 != d2)
> +    {
> +      printf ("FAIL: %04lx != %04lx (maj %02x min %02x)\n",
> +              d2, d1, maj, min);

you noted that we can't rely on dev_t size, so you'll probably want to
cast to uint64_t and use PRIx64 in the format string.

also, you seem to be omitting the 8 spaces -> 1 tab conversion here and
in other places in this patch.

> --- /dev/null
> +++ b/sysdeps/generic/bits/sysmacros.h
>
> +#define __dev_makedev(major, minor) (((major) << 8) | (minor & 0xff))

parens around (minor) please
-mike

Attachment: signature.asc
Description: Digital signature