Date: Thu, 3 Aug 2017 19:34:56 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, Hans Petter Selasky <hps@selasky.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r321920 - head/sys/sys Message-ID: <20170803180419.R2314@besplex.bde.org> In-Reply-To: <20170803075747.GJ1700@kib.kiev.ua> References: <201708021014.v72AEHEk061037@repo.freebsd.org> <f7a4a90c-f1d8-b381-27fe-3cf76b574a29@selasky.org> <37abc595-c80e-a8da-04a8-815f42c634de@selasky.org> <20170802135455.GG1700@kib.kiev.ua> <20170803122015.Q1093@besplex.bde.org> <20170803075747.GJ1700@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 3 Aug 2017, Konstantin Belousov wrote: > On Thu, Aug 03, 2017 at 01:21:56PM +1000, Bruce Evans wrote: >> It would be better to remove the comments. >> >> makedev() actually has a man page (a FreeBSD addition makedev(3)). >> This could have been better, and is now out of date. The largest error >> is that major() is still documented to return a value between 0 and >> 255. This reduction prevented makedev() being the inverse of >> major()+minor() on arbitrary args. The man page doesn't claim that >> it is, and barely gives a hint that makedev() can be used to synthesize >> a dev_t from (x, y) provided 0 <= x <= 255. > > See below. > > diff --git a/share/man/man3/makedev.3 b/share/man/man3/makedev.3 > index 87ca953dc79..a54d0590bc5 100644 > --- a/share/man/man3/makedev.3 > +++ b/share/man/man3/makedev.3 > ... > @@ -54,11 +54,18 @@ and > .Fn minor > macros can be used to obtain the original numbers from the device number Problems with old wording: - "can be used" is too informal. Before this, makedev() is described as "allows" something. "allow" is almost never used in share/man3/*, even in the sense of giving permission. Only pthread man pages have many instances of this bug. - "original" is an anachronism. The major and minor used to be the original numbers, but now it is the dev_t that is original except when the dev_t is synthesized. "original" might also mean "original representation". Fixes: "can be used to obtain" is just a verbose spelling of "return". "allows" can be improved using "returns" too, and it would be good to shorten "a unique device number to be generated based on" to something that says less about the one to one correspondence. "generation" is just a special case of applying the correspondence to get an alternative representation. "original" is also related to the correspondence. Perhaps describe the correspondence first and then only say that the functions are the ones that implement it. > .Fa dev . > +In other words, for a value > +.Va dev > +of the type > +.Vt dev_t , > +the assertion > +.Dl dev == makedev(major(dev), minor(dev)) > +is valid. This describes the correspondence in another way. Pehaps delete all details about what the functions do in ther previous description and say that they are defined by this property. I think the inverse property also needs to be spelled out. It is that for (int ma, int mi) .Dl ma = major(makedev(ma, mi)) .Dl mi = minor(makedev(ma, mi)) > .Pp > In previous implementations of > .Fx > all block and character devices were uniquely identified by a pair of > -major and minor numbers. > +stable major and minor numbers. "stable" is clarified later, but is hard to understand in context. > The major number referred to a certain device class (e.g. disks, TTYs) > while the minor number identified an instance within the device class. > Later versions of > @@ -66,7 +73,8 @@ Later versions of > automatically generate a unique device number for each character device > visible in > .Pa /dev/ . > -These numbers are not divided in device classes. > +These numbers are not divided in device classes and are not guaranteed > +to be stable upon reboot or driver reload. They used to be stable across even more than reboot. They were kept in file systems, and that required them to be stable across OS versions in practice. > .Pp > On > .Fx > @@ -78,11 +86,9 @@ conventional way. > .Sh RETURN VALUES > The > .Fn major > -macro returns a device major number that has a value between 0 and 255. > -The > +and > .Fn minor > -macro returns a device minor number whose value can span the complete > -range of an > +macros return numbers whose value can span the complete range of an > .Vt int . > .Sh SEE ALSO > .Xr mknod 2 , > diff --git a/sys/sys/types.h b/sys/sys/types.h > index 30a08724443..8ea6e146e08 100644 > --- a/sys/sys/types.h > +++ b/sys/sys/types.h > @@ -364,9 +364,9 @@ __bitcount64(__uint64_t _x) > > #include <sys/select.h> > > -#define major(x) ((int)((dev_t)(x) >> 32)) /* major number */ > -#define minor(x) ((int)((x) & 0xffffffff)) /* minor number */ > -#define makedev(x, y) (((dev_t)(x) << 32) | (unsigned)(y)) /* create dev_t */ > +#define major(x) ((int)((dev_t)(x) >> 32)) > +#define minor(x) ((int)((x) & 0xffffffff)) > +#define makedev(x, y) (((dev_t)(x) << 32) | (unsigned)(y)) I see another problem. Masking with 0xfffffffff and casting to unsigned are gratuitously different spellings for extracting the low 32 bits. I prefer the cast. It only works if ints have 32 bits, but we assume that for other reasons. The mask is especially hard to understand for the 1's complement case. We avoid complications with dodgy args for major() by casting the arg to dev_t. For minor(), the behaviour is only clear if x is unsigned (it should be dev_t), or int or lower rank unsigned and typeof(0xffffffff) == unsigned (true with 32-bit ints). In the second case, the default promotions convert x to unsigned. -0 becomes 0U which probably breaks it on 1's complement systems, but -0L doesn't change, which is hard to understand. To avoid complications, we could cast to dev_t, but that is verbose, or to unsigned, but then the mask is redundant if ints are 32 bits. For makedev() we, do just cast to unsigned and assume 32 bits. > > /* > * These declarations belong elsewhere, but are repeated here and in > Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170803180419.R2314>