Date: Thu, 30 Oct 2014 06:11:09 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r273129 - head/sys/kern Message-ID: <20141030054402.G2929@besplex.bde.org> In-Reply-To: <20141029165523.GF53947@kib.kiev.ua> References: <201410151238.s9FCcRMe018200@svn.freebsd.org> <20141028040935.M3114@besplex.bde.org> <20141029165523.GF53947@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 Oct 2014, Konstantin Belousov wrote:
> On Tue, Oct 28, 2014 at 04:28:37AM +1100, Bruce Evans wrote:
>> @diff -u2 dd.c~ dd.c
>> @--- dd.c~ Wed Apr 7 20:20:48 2004
>> @+++ dd.c Wed Apr 7 20:20:49 2004
>> @@@ -247,21 +245,18 @@
>> @ io->flags |= ISTRUNC;
>> @ if (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode)) {
>> @- if (ioctl(io->fd, FIODTYPE, &type) == -1) {
>> @+ if (ioctl(io->fd, FIODTYPE, &type) == -1)
>> @ err(1, "%s", io->name);
>> @- } else {
>> @+ else {
>> @ if (type & D_TAPE)
>> @ io->flags |= ISTAPE;
>> @ else if (type & (D_DISK | D_MEM))
>> @- io->flags |= ISSEEK;
>> @- if (S_ISCHR(sb.st_mode) && (type & D_TAPE) == 0)
>> @+ io->flags |= ISSEEKABLE;
>> @+ if (S_ISCHR(sb.st_mode))
>> @ io->flags |= ISCHR;
>> @ }
>> @- return;
>> @- }
>> @- errno = 0;
>> @- if (lseek(io->fd, (off_t)0, SEEK_CUR) == -1 && errno == ESPIPE)
>> @- io->flags |= ISPIPE;
>> @- else
>> @- io->flags |= ISSEEK;
>> @+ } else if (lseek(io->fd, (off_t)0, SEEK_CUR) == 0)
>> @+ io->flags |= ISSEEKABLE;
>> @+ else if (errno == ESPIPE)
>> @+ io->flags |= ISPIPE; /* XXX fixed in 4.4BSD */
>> @ }
>> @
>
> Ok, I tried to de-obfuscate and restore the patch above.
The patch came out not very readable for some reason.
> diff --git a/bin/dd/dd.c b/bin/dd/dd.c
> index 8ae11a7..aadc7da 100644
> --- a/bin/dd/dd.c
> +++ b/bin/dd/dd.c
> @@ -257,7 +257,7 @@ getfdtype(IO *io)
> err(1, "%s", io->name);
> if (S_ISREG(sb.st_mode))
> io->flags |= ISTRUNC;
> - if (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode)) {
> + if (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode)) {
> if (ioctl(io->fd, FIODTYPE, &type) == -1) {
> err(1, "%s", io->name);
> } else {
> @@ -265,16 +265,14 @@ getfdtype(IO *io)
> io->flags |= ISTAPE;
> else if (type & (D_DISK | D_MEM))
> io->flags |= ISSEEK;
> - if (S_ISCHR(sb.st_mode) && (type & D_TAPE) == 0)
> + if (S_ISCHR(sb.st_mode))
> io->flags |= ISCHR;
> }
> - return;
> - }
> - errno = 0;
> - if (lseek(io->fd, (off_t)0, SEEK_CUR) == -1 && errno == ESPIPE)
> - io->flags |= ISPIPE;
> - else
> + } else if (lseek(io->fd, (off_t)0, SEEK_CUR) == 0) {
> io->flags |= ISSEEK;
> + } else if (errno == ESPIPE) {
> + io->flags |= ISPIPE;
> + }
> }
>
> static void
That came out not very readable too. I don't like it much. Now I don't
see what I was doing with the lseek() changes, except to improve the
spelling (SEEK -> SEEKABLE). The XXX comment was in 4.4BSD. I restored
it, but now I think removing it was correct, and the other FreeBSD
changes near the lseek() call are improvements too.
Another try, starting with -current sources. It doesn't touch the lseek()
code, but changes more before that, to return early and reduce indentation
after that, and never fail:
% diff -u2 dd.c~ dd.c
% --- dd.c~ 2014-08-06 20:12:52.000000000 +0000
% +++ dd.c 2014-10-29 18:36:03.979133000 +0000
% @@ -255,18 +255,16 @@
%
% if (fstat(io->fd, &sb) == -1)
% - err(1, "%s", io->name);
% + return;
Unlikely error. Treat it is null type.
% if (S_ISREG(sb.st_mode))
% io->flags |= ISTRUNC;
% if (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode)) {
% + if (S_ISCHR(sb.st_mode))
% + io->flags |= ISCHR;
Default for next return.
Note that block devices aren't supported, so the ISBLK() check and the
second ISCHR() check are redundant. I keep them for compatibility.
% if (ioctl(io->fd, FIODTYPE, &type) == -1) {
% - err(1, "%s", io->name);
% - } else {
% - if (type & D_TAPE)
% - io->flags |= ISTAPE;
% - else if (type & (D_DISK | D_MEM))
% - io->flags |= ISSEEK;
% - if (S_ISCHR(sb.st_mode) && (type & D_TAPE) == 0)
% - io->flags |= ISCHR;
% - }
% + return;
Actually, ISBLK() is possible. Then since block devices aren't supported,
the ioctl should fail, and we again return with a null type.
% + if (type & D_TAPE)
% + io->flags |= ISTAPE;
% + else if (type & (D_DISK | D_MEM))
% + io->flags |= ISSEEK;
Re-indent and simplify a little.
The only change in the non-failure cases is to not use tangled logic to
avoid setting ISCHR for tape devices. So tape devices are ISCHR | ISTAPE
now, and still not ISSEEK. ISTAPE is only used in a couple of places,
and it is easy to see that having ISCHR also set doesn't affect the
behaviour of ISTAPE.
% return;
% }
After here there is a seek test. We get here for irregular regular files
in procfs. Hopefully the seek test will fail for them. The seek test
isn't very good, but it is better than unconditionally setting ISSEEK
for regular files. We could also try falling through to here to do the
seek test for cdevs, but it is know to give wrong results like succeeding
on keyboards so it is best to only set ISSEEK for devices that are known
to be seekable.
I now see a small improvement in the seek test. Just remove its errno
stuff, and trust the syscall to return -1 on error. This depends on -1
being an impossible seek offset. POSIX requires this, but we make it
possible so that /dev/mem can be seeked to address 0xFFFFFFFFFFFFFFFF
on 64-bit arches. We do this for any cdev but no other file types.
Since we returned earlier for cdevs, this case can't happen here.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141030054402.G2929>