GNU bug report logs - #7362
dd strangeness

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 7362 in the body.
You can then email your comments to 7362 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Lucia Rotger <lucia <at> aircomp.aero>
To: bug-coreutils <at> gnu.org
Subject: dd strangeness
Date: Wed, 10 Nov 2010 11:22:25 +0100

I see this behavior in Solaris, Linux and BSD dd: if I send a big enough 
file they all read it short at the end of the stream.

This works as expected:

# cat /dev/zero | dd bs=512 count=293601280 | wc

I get the expected results, dd reads exactly 293601280 blocks and wc 
sees 150323855360 characters, 140 GB

Whereas substituting cat for zfs send doesn't:

# zfs send <backup> | dd bs=512 count=293601280 | wc

The output of one of the runs is

293590463+10817 records in
293590463+10817 records out

and the bytes counted by wc are < 140 GB. The zfs command sends 600 GB, 
so obviously dd should not run short.

BSD and Linux dd were used on BSD and Linux machines, respectively, 
piping the stream with nc.

Since this happens with three different implementations of dd I'm 
thinking of a design flaw but I've never ecountered it before. I'm 
testing sdd (a dd replacement) and will see what happens, but it'll take 
5 hours still. There seems to be something going on in dd with different 
input and output block sizes since both sdd and this 
https://bugs.launchpad.net/ubuntu/+source/rsyslog/+bug/517773 hint at 
it: "The dd process requires a ridiculous amount of CPU during startup, 
though, since it is running with bs=1 to not miss stuff". But I don't 
know if that's what's happening here. According to man dd, bs sets ibs 
and obs.

bs=512 is the last attempt I made but I've tried combinations of the bs 
and count parameters (always to make a size of 140 GB) to no avail, 
nothing seems to work with a big stream. I still haven't tried bs=1 as I 
think it would take weeks to go through but maybe I'm wrong. If I try 
with smaller files, up to hundreds of MBs dd works fine, but I can't 
tell at what size it breaks or under which circumstances or why.

Message #10 received at 7362-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Lucia Rotger <lucia <at> aircomp.aero>
Cc: 7362-done <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 15:29:41 +0000

On 10/11/10 10:22, Lucia Rotger wrote:
> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
> file they all read it short at the end of the stream.
> 
> This works as expected:
> 
> # cat /dev/zero | dd bs=512 count=293601280 | wc
> 
> I get the expected results, dd reads exactly 293601280 blocks and wc
> sees 150323855360 characters, 140 GB
> 
> Whereas substituting cat for zfs send doesn't:
> 
> # zfs send <backup> | dd bs=512 count=293601280 | wc

different write sizes to the pipe mean
in the later case, dd will get short reads.
IMHO dd is doing the wrong/most surprising thing here,
but it can't be changed for compatibility reasons.
You can get coreutils dd to do what you want with:

dd iflag=fullblock

cheers,
Pádraig.

Message #13 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 16:07:18 +0000

On 10/11/10 15:29, Pádraig Brady wrote:
> On 10/11/10 10:22, Lucia Rotger wrote:
>> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
>> file they all read it short at the end of the stream.
>>
>> This works as expected:
>>
>> # cat /dev/zero | dd bs=512 count=293601280 | wc
>>
>> I get the expected results, dd reads exactly 293601280 blocks and wc
>> sees 150323855360 characters, 140 GB
>>
>> Whereas substituting cat for zfs send doesn't:
>>
>> # zfs send <backup> | dd bs=512 count=293601280 | wc
> 
> different write sizes to the pipe mean
> in the later case, dd will get short reads.
> IMHO dd is doing the wrong/most surprising thing here,
> but it can't be changed for compatibility reasons.
> You can get coreutils dd to do what you want with:
> 
> dd iflag=fullblock

BTW here are my notes on some possible changes in this area:

  `dd conv=sync` seems to do the wrong thing with pipes.
  I.E. it pads out short reads. Why would one ever want that?

  Should sync for pipes imply fullblock?

  Should sync for pipes without fullblock give a warning?

  Should specifying a count (and bs I suppose) without fullblock
  when reading from pipes give a warning?

cheers,
Pádraig

Message #14 received at 7362-done <at> debbugs.gnu.org (full text, mbox):

From: Lucia Rotger <lucia <at> aircomp.aero>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362-done <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 18:47:48 +0100

Thanks, I'll try it, but it's not in the info or man pages.

Also, the section under FLAGS is badly formatted with the different 
options all in one line. You might want to fix that.

Lucia

On 10/11/2010 16:29, Pádraig Brady wrote:
> On 10/11/10 10:22, Lucia Rotger wrote:
>> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
>> file they all read it short at the end of the stream.
>>
>> This works as expected:
>>
>> # cat /dev/zero | dd bs=512 count=293601280 | wc
>>
>> I get the expected results, dd reads exactly 293601280 blocks and wc
>> sees 150323855360 characters, 140 GB
>>
>> Whereas substituting cat for zfs send doesn't:
>>
>> # zfs send<backup>  | dd bs=512 count=293601280 | wc
>
> different write sizes to the pipe mean
> in the later case, dd will get short reads.
> IMHO dd is doing the wrong/most surprising thing here,
> but it can't be changed for compatibility reasons.
> You can get coreutils dd to do what you want with:
>
> dd iflag=fullblock
>
> cheers,
> Pádraig.
>

Message #17 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 10:56:47 -0800

On 11/10/10 08:07, Pádraig Brady wrote:

> BTW here are my notes on some possible changes in this area:
> 
>   `dd conv=sync` seems to do the wrong thing with pipes.
>   I.E. it pads out short reads. Why would one ever want that?

Nobody ever wants it but (as you note) it's required by the
standard.

>   Should sync for pipes imply fullblock?

I think that would be a good idea, but it should be done only
if POSIXLY_CORRECT is not set, since it contradicts the standard.

>   Should sync for pipes without fullblock give a warning?

I would say "no".  If POSIXLY_CORRECT, the warning would arguably
violate the standard.  If not, there's no need for a warning.

>   Should specifying a count (and bs I suppose) without fullblock
>   when reading from pipes give a warning?

No, for the same reason.

Do you have time to implement this change?  If not, I suppose
I could look into it.  It shouldn't be that hard.  The hardest
part would be the documentation.

Regardless of whether we make such a change, we should document
the issue, as this is a common problem.

Message #18 received at 7362-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Lucia Rotger <lucia <at> aircomp.aero>
Cc: 7362-done <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 22:23:45 +0000

On 10/11/10 17:47, Lucia Rotger wrote:
> Thanks, I'll try it, but it's not in the info or man pages.
> 
> Also, the section under FLAGS is badly formatted with the different
> options all in one line. You might want to fix that.

I think all that was fixed up.
`dd iflag=fullblock` was implemented in coreutils-7.0

cheers,
Pádraig.

Message #21 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Thu, 11 Nov 2010 01:19:01 +0000

On 10/11/10 18:56, Paul Eggert wrote:
> On 11/10/10 08:07, Pádraig Brady wrote:
> 
>> BTW here are my notes on some possible changes in this area:
>>
>>   `dd conv=sync` seems to do the wrong thing with pipes.
>>   I.E. it pads out short reads. Why would one ever want that?
> 
> Nobody ever wants it but (as you note) it's required by the
> standard.
> 
>>   Should sync for pipes imply fullblock?
> 
> I think that would be a good idea, but it should be done only
> if POSIXLY_CORRECT is not set, since it contradicts the standard.
> 
>>   Should sync for pipes without fullblock give a warning?
> 
> I would say "no".  If POSIXLY_CORRECT, the warning would arguably
> violate the standard.  If not, there's no need for a warning.

Agreed on all the above.

> 
>>   Should specifying a count (and bs I suppose) without fullblock
>>   when reading from pipes give a warning?
> 
> No, for the same reason.

Or maybe give the warning when POSIXLY_CORRECT was not set?

> 
> Do you have time to implement this change?  If not, I suppose
> I could look into it.  It shouldn't be that hard.  The hardest
> part would be the documentation.
> 
> Regardless of whether we make such a change, we should document
> the issue, as this is a common problem.

I'll look into it at some stage but not soon.
Feel free do it if you like.

cheers,
Pádraig.

Message #24 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 10 Nov 2010 22:01:05 -0800

On 11/10/2010 05:19 PM, Pádraig Brady wrote:
>> > 
>>> >>   Should specifying a count (and bs I suppose) without fullblock
>>> >>   when reading from pipes give a warning?
>> > 
>> > No, for the same reason.
> Or maybe give the warning when POSIXLY_CORRECT was not set?

I wouldn't bother.  Let's just have dd do the right thing quietly.

I am a bit tied down too doing other things, but will send you email if I start
working on it.

Message #27 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Thu, 11 Nov 2010 09:32:48 +0000

On 11/11/10 06:01, Paul Eggert wrote:
> On 11/10/2010 05:19 PM, Pádraig Brady wrote:
>>>>
>>>>>>   Should specifying a count (and bs I suppose) without fullblock
>>>>>>   when reading from pipes give a warning?
>>>>
>>>> No, for the same reason.
>> Or maybe give the warning when POSIXLY_CORRECT was not set?
> 
> I wouldn't bother.  Let's just have dd do the right thing quietly.

Oh right, do the auto fullblock for that case also.
Agreed.

> I am a bit tied down too doing other things, but will send you email if I start
> working on it.

cheers,
Pádraig.

Message #28 received at 7362-done <at> debbugs.gnu.org (full text, mbox):

From: Lucia Rotger <lucia <at> aircomp.aero>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362-done <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 12 Nov 2010 15:53:19 +0100

On 10/11/2010 16:29, Pádraig Brady wrote:
> On 10/11/10 10:22, Lucia Rotger wrote:
>> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
>> file they all read it short at the end of the stream.
>>
>> This works as expected:
>>
>> # cat /dev/zero | dd bs=512 count=293601280 | wc
>>
>> I get the expected results, dd reads exactly 293601280 blocks and wc
>> sees 150323855360 characters, 140 GB
>>
>> Whereas substituting cat for zfs send doesn't:
>>
>> # zfs send<backup>  | dd bs=512 count=293601280 | wc
>
> different write sizes to the pipe mean
> in the later case, dd will get short reads.
> IMHO dd is doing the wrong/most surprising thing here,
> but it can't be changed for compatibility reasons.
> You can get coreutils dd to do what you want with:
>
> dd iflag=fullblock

Thanks for the explanation. Now, if I may abuse your time a bit more, 
how exactly is a block size determined when reading from a pipe? is it 
the size returned by each read()? And why does dd stop short at the last 
block?

Sorry about those problems in the man page being already fixed. I 
checked and I have dd version 6.1, these Ubuntu guys don't update very well.

I have somewhat solved it in a more portable way doing

# zfs send<backup> | dd obs=512 | dd bs=512 count=293601280 | wc

I guess as long as no writes from zfs send are smaller than 512 bytes. 
It works nicely now.

I suppose adding ibs=1 to the first dd would work with everything but it 
would probably take ages to finish.

Thanks

Message #29 received at 7362-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Lucia Rotger <lucia <at> aircomp.aero>
Cc: 7362-done <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 12 Nov 2010 15:20:07 +0000

On 12/11/10 14:53, Lucia Rotger wrote:
> On 10/11/2010 16:29, Pádraig Brady wrote:
>> On 10/11/10 10:22, Lucia Rotger wrote:
>>> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
>>> file they all read it short at the end of the stream.
>>>
>>> This works as expected:
>>>
>>> # cat /dev/zero | dd bs=512 count=293601280 | wc
>>>
>>> I get the expected results, dd reads exactly 293601280 blocks and wc
>>> sees 150323855360 characters, 140 GB
>>>
>>> Whereas substituting cat for zfs send doesn't:
>>>
>>> # zfs send<backup>  | dd bs=512 count=293601280 | wc
>>
>> different write sizes to the pipe mean
>> in the later case, dd will get short reads.
>> IMHO dd is doing the wrong/most surprising thing here,
>> but it can't be changed for compatibility reasons.
>> You can get coreutils dd to do what you want with:
>>
>> dd iflag=fullblock
> 
> Thanks for the explanation. Now, if I may abuse your time a bit more,
> how exactly is a block size determined when reading from a pipe? is it
> the size returned by each read()? And why does dd stop short at the last
> block?
> 
> Sorry about those problems in the man page being already fixed. I
> checked and I have dd version 6.1, these Ubuntu guys don't update very
> well.
> 
> I have somewhat solved it in a more portable way doing
> 
> # zfs send<backup> | dd obs=512 | dd bs=512 count=293601280 | wc

The above works because the middle dd coalesces the short reads,
and so writes 512 bytes at a time, and so the next dd will not
have short reads. This will not generally work as you increase the size.

To illustrate what the middle dd is doing:

$ (echo 1; sleep 1; echo 2) | strace -e read,write dd obs=512
read(0, "1\n", 512)                     = 2
read(0, "2\n", 512)                     = 2
read(0, "", 512)                        = 0
write(1, "1\n2\n", 4)                   = 4

> I suppose adding ibs=1 to the first dd would work with everything but it
> would probably take ages to finish.

right. The crux of your problem is specifying a count.
If you didn't the short reads wouldn't matter.
If you do need to specify a count, then the iflag=fullblock
available since 7.0 will work.

cheers,
Pádraig.

Message #36 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Feb 2011 13:23:46 +0000

On 10/11/10 16:07, Pádraig Brady wrote:
> On 10/11/10 15:29, Pádraig Brady wrote:
>> On 10/11/10 10:22, Lucia Rotger wrote:
>>> I see this behavior in Solaris, Linux and BSD dd: if I send a big enough
>>> file they all read it short at the end of the stream.
>>>
>>> This works as expected:
>>>
>>> # cat /dev/zero | dd bs=512 count=293601280 | wc
>>>
>>> I get the expected results, dd reads exactly 293601280 blocks and wc
>>> sees 150323855360 characters, 140 GB
>>>
>>> Whereas substituting cat for zfs send doesn't:
>>>
>>> # zfs send <backup> | dd bs=512 count=293601280 | wc
>>
>> different write sizes to the pipe mean
>> in the later case, dd will get short reads.
>> IMHO dd is doing the wrong/most surprising thing here,
>> but it can't be changed for compatibility reasons.
>> You can get coreutils dd to do what you want with:
>>
>> dd iflag=fullblock
> 
> BTW here are my notes on some possible changes in this area:
> 
>   `dd conv=sync` seems to do the wrong thing with pipes.
>   I.E. it pads out short reads. Why would one ever want that?
> 
>   Should sync for pipes imply fullblock?
> 
>   Should sync for pipes without fullblock give a warning?
> 
>   Should specifying a count (and bs I suppose) without fullblock
>   when reading from pipes give a warning?

This looks like another candidate to auto enable fullblock for.
https://bugzilla.redhat.com/show_bug.cgi?id=614605
I.E. oflag=direct

cheers,
Pádraig.

Message #39 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 25 Feb 2011 12:19:35 +0000

[Message part 1 (text/plain, inline)]

On 02/02/11 13:23, Pádraig Brady wrote:
> This looks like another candidate to auto enable fullblock for.
> https://bugzilla.redhat.com/show_bug.cgi?id=614605
> I.E. oflag=direct

Attached is a proposed solution to this.
I'm worried about the last condition though
where we enable 'fullblock' when both count and bs are specified.

For example this would still work:

# Output first 2 parts
$ (echo part1; sleep 1; echo part2; sleep 1; echo discard) |
  dd count=2 obs=1 2>/dev/null
part1
part2

However this would not:

# Output first 2 parts, each being up to 4096 bytes
$ (echo part1; sleep 1; echo part2; sleep 1; echo discard) |
  dd count=2 ibs=4096 obs=1 2>/dev/null
part1
part2
discard

So how contrived is the last example,
given how brittle such a construct is?

cheers,
Pádraig.

[dd-fullblock.diff (text/x-patch, attachment)]

Message #42 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 25 Feb 2011 09:29:24 -0800

On 02/25/2011 04:19 AM, Pádraig Brady wrote:

> Attached is a proposed solution to this.

My kneejerk reaction is that it tries to do too much inferring,
and ends up being more complicated than giving the user more control.

If we're going to change the default to be not compatible with POSIX,
we need to give the user a way to get the POSIX behavior, something
that's less subtle than POSIXLY_CORRECT.  I suggest that we add
a new option that is the inverse of "fullblock".  We can call it
"partblock", say.

Then, we can say that the default is "fullblock" normally, but it is
"partblock" if POSIXLY_CORRECT and if bs= is given and if no conversions
other than sync, noerror, and notrunk are given.

Anyway, I'm just thinking out loud to some extent, and further
comments are welcome.

Message #45 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Mon, 28 Feb 2011 09:41:11 +0000

[Message part 1 (text/plain, inline)]

On 25/02/11 17:29, Paul Eggert wrote:
> On 02/25/2011 04:19 AM, Pádraig Brady wrote:
> 
>> Attached is a proposed solution to this.
> 
> My kneejerk reaction is that it tries to do too much inferring,
> and ends up being more complicated than giving the user more control.
> 
> If we're going to change the default to be not compatible with POSIX,
> we need to give the user a way to get the POSIX behavior, something
> that's less subtle than POSIXLY_CORRECT.  I suggest that we add
> a new option that is the inverse of "fullblock".  We can call it
> "partblock", say.
> 
> Then, we can say that the default is "fullblock" normally, but it is
> "partblock" if POSIXLY_CORRECT and if bs= is given and if no conversions
> other than sync, noerror, and notrunk are given.
> 
> Anyway, I'm just thinking out loud to some extent, and further
> comments are welcome.

Hmm, it's better to be explicit but I think defaulting to "fullblock"
is too risky. As an interim step at least, how about just warning
as per the attached.

cheers,
Pádraig.

[dd-fullblock.diff (text/x-patch, attachment)]

Message #48 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 01:50:08 -0800

On 02/28/2011 01:41 AM, Pádraig Brady wrote:
> Hmm, it's better to be explicit but I think defaulting to "fullblock"
> is too risky. As an interim step at least, how about just warning
> as per the attached.

Ouch.  This is a pain to think about.  But here are some thoughts anyway:

 * I went back and reread POSIX, and "dd" is allowed to issue
   diagnostics to stderr whenever it likes.  So we don't need to
   worry about POSIXLY_CORRECT if all we want to do is issue diagnostics.
   We can issue them regardless of POSIXLY_CORRECT.

 * I don't understand the business with C_SYNC.  People who use
   conv=sync know what they're doing, or ought to; there's little
   point giving them a warning.

 * For (O_DIRECT | O_CIO), surely this matters only for output_flags.
   If these bits are set in input_flags then O_FULLBLOCK is irrelevant, no?

 * If we care about max_records we should also care about skip_records,
   since short reads matter when skipping in a pipe, too.

 * Since POSIX doesn't specify the direct or cio flags, we're free
   to have them silently enable iflag=fullblock.  But it doesn't sound
   right to do that.  Instead, we should set conversions_mask |= C_TWOBUFS,
   because the input and output blocksizes might differ.

 * If we suggest ibs=whatever rather than iflag=fullblock, our
   suggestions will be portable to other POSIX implementations, which
   is a plus.

 * Rather than warn about potential problems, how about diagnosing the
   problems only when they actually occur?  That would help us avoid
   crying wolf.

Here's a proposed patch that tries to embody all the above, except
that I haven't done the documentation (I figure we should get the
behavior right first....):


From 85e3716f918e8163695a85d40fe8c561634c9e2e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 1 Mar 2011 01:34:57 -0800
Subject: [PATCH] dd: avoid or diagnose some problems with short reads
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/dd.c (iread): Diagnose short reads when they mess up counts.
(scanargs): If oflags=direct or oflags=cio, use
C_TWOBUFS so that the output blocks are typically full.
Derived from a suggestion by Pádraig Brady in:
http://lists.gnu.org/archive/html/bug-coreutils/2011-02/msg00150.html
---
 src/dd.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index daddc1e..41ad7a3 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -802,7 +802,29 @@ iread (int fd, char *buf, size_t size)
       process_signals ();
       nread = read (fd, buf, size);
       if (! (nread < 0 && errno == EINTR))
-        return nread;
+        {
+          static ssize_t prev_nread;
+          static bool warned;
+
+          if (nread != 0 && 0 < prev_nread && prev_nread < size
+              && iread_fnc == iread
+              && ! (conversions_mask & C_TWOBUFS)
+              && (skip_records
+                  || (0 < max_records && max_records < (uintmax_t) -1))
+              && ! warned)
+            {
+              unsigned long int prev = prev_nread;
+              unsigned long int ibs = input_blocksize;
+              error (0, 0, _("warning: short read (%lu bytes)"), prev);
+              error (0, 0,
+                     _("Perhaps you wanted ibs=%lu rather than bs=%lu?"),
+                     ibs, ibs);
+              warned = true;
+            }
+
+          prev_nread = nread;
+          return nread;
+        }
     }
 }
 
@@ -1075,6 +1097,9 @@ scanargs (int argc, char *const *argv)
       conversions_mask |= C_TWOBUFS;
     }
 
+  if (output_flags & (O_DIRECT | O_CIO))
+    conversions_mask |= C_TWOBUFS;
+
   if (input_blocksize == 0)
     input_blocksize = DEFAULT_BLOCKSIZE;
   if (output_blocksize == 0)
-- 
1.7.4

Message #51 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 11:27:11 +0000

On 01/03/11 09:50, Paul Eggert wrote:
> On 02/28/2011 01:41 AM, Pádraig Brady wrote:
>> Hmm, it's better to be explicit but I think defaulting to "fullblock"
>> is too risky. As an interim step at least, how about just warning
>> as per the attached.
> 
> Ouch.  This is a pain to think about.  But here are some thoughts anyway:
> 
>  * I went back and reread POSIX, and "dd" is allowed to issue
>    diagnostics to stderr whenever it likes.  So we don't need to
>    worry about POSIXLY_CORRECT if all we want to do is issue diagnostics.
>    We can issue them regardless of POSIXLY_CORRECT.

Checking POSIXLY_CORRECT allows one to disable the warnings

> 
>  * I don't understand the business with C_SYNC.  People who use
>    conv=sync know what they're doing, or ought to; there's little
>    point giving them a warning.

They might think the short read might only apply to the end of the input?
But fair enough, it's not as important as the O_DIRECT issue.

>  * For (O_DIRECT | O_CIO), surely this matters only for output_flags.
>    If these bits are set in input_flags then O_FULLBLOCK is irrelevant, no?

True.

>  * If we care about max_records we should also care about skip_records,
>    since short reads matter when skipping in a pipe, too.

True.

>  * Since POSIX doesn't specify the direct or cio flags, we're free
>    to have them silently enable iflag=fullblock.  But it doesn't sound
>    right to do that.  Instead, we should set conversions_mask |= C_TWOBUFS,
>    because the input and output blocksizes might differ.
> 
>  * If we suggest ibs=whatever rather than iflag=fullblock, our
>    suggestions will be portable to other POSIX implementations, which
>    is a plus.

So the standard way to accumulate short reads to a full write,
is to specify separate ibs and obs (we'd probably want to prompt about
setting obs too for efficiency). However I think that would mess up
with a specific count (1 for each partial read) or with conv=sync,noerror

> 
>  * Rather than warn about potential problems, how about diagnosing the
>    problems only when they actually occur?  That would help us avoid
>    crying wolf.

I like that idea, except that users might only hit an issue
on particular runs, or when moving from a test file
to a pipe in production.

cheers,
Pádraig.

Message #54 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 12:01:55 +0000

On 01/03/11 11:27, Pádraig Brady wrote:
> On 01/03/11 09:50, Paul Eggert wrote:
>>  * Since POSIX doesn't specify the direct or cio flags, we're free
>>    to have them silently enable iflag=fullblock.  But it doesn't sound
>>    right to do that.  Instead, we should set conversions_mask |= C_TWOBUFS,
>>    because the input and output blocksizes might differ.
>>
>>  * If we suggest ibs=whatever rather than iflag=fullblock, our
>>    suggestions will be portable to other POSIX implementations, which
>>    is a plus.
> 
> So the standard way to accumulate short reads to a full write,
> is to specify separate ibs and obs (we'd probably want to prompt about
> setting obs too for efficiency). However I think that would mess up
> with a specific count (1 for each partial read) or with conv=sync,noerror

Oh right, you're warning for short reads with a specific count
and discounting the conv=sync,noerror case.

Message #57 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 09:45:42 -0800

On 03/01/2011 03:27 AM, Pádraig Brady wrote:

> So the standard way to accumulate short reads to a full write,
> is to specify separate ibs and obs (we'd probably want to prompt about
> setting obs too for efficiency)

Yes, good point, the diagnostic should suggest ibs=N obs=N
(instead of just ibs=N).

By the way, the relationship between fullblock and ibs=N obs=N is
a curious one, one that I don't fully understand.  If you have
ibs=N obs=N, why would you need fullblock?  This should probably
be documented (preferably by someone who understands it :-).

>>   * Rather than warn about potential problems, how about diagnosing the
>>     problems only when they actually occur?  That would help us avoid
>>     crying wolf.
>
> I like that idea, except that users might only hit an issue
> on particular runs, or when moving from a test file
> to a pipe in production.

True in both cases, but in practice these problems would
be pretty rare compared to the problem of dd crying wolf.
It's fairly common for people to use dd bs=N to extract sections
of regular files, e.g., "dd bs=1k count=1 seek=1 if=bigfile",
and issuing warnings in these cases (when dd operates perfectly
well) would cause unnecessary confusion.

> Checking POSIXLY_CORRECT allows one to disable the warnings

Yes, but POSIXLY_CORRECT should be used only to make a program conform
to POSIX when the default behavior does not conform.  It shouldn't
be used for auxiliary purposes such as suppressing warnings.  If we
want an option to suppress warnings, we should add one; but we
shouldn't overload POSIXLY_CORRECT.

Message #60 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 11:12:17 -0700

[Message part 1 (text/plain, inline)]

On 03/01/2011 10:45 AM, Paul Eggert wrote:
>> Checking POSIXLY_CORRECT allows one to disable the warnings
> 
> Yes, but POSIXLY_CORRECT should be used only to make a program conform
> to POSIX when the default behavior does not conform.  It shouldn't
> be used for auxiliary purposes such as suppressing warnings.  If we
> want an option to suppress warnings, we should add one; but we
> shouldn't overload POSIXLY_CORRECT.

Agreed - POSIXLY_CORRECT is not the right knob to use, since we are
already asserting that the warnings don't violate POSIX (indeed, POSIX
states under stderr that "Diagnostic messages may also be written to
standard error.", line 83304 of POSIX 2008).  We already have
status=noxfer as a way to suppress one class of output; we could add
status=nowarn to suppress this new warning.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #63 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 11:30:12 -0800

On 03/01/2011 09:45 AM, Paul Eggert wrote:
> By the way, the relationship between fullblock and ibs=N obs=N is
> a curious one, one that I don't fully understand.  If you have
> ibs=N obs=N, why would you need fullblock?  This should probably
> be documented (preferably by someone who understands it :-).

In looking into this some more, I did find one difference
(or at least, something that *should* be a difference).

POSIX says that when an input record has
an odd size, conv=swab should output the last byte as-is.
If dd obeyed POSIX, ibs=100 obs=100 conv=swab would differ
from bs=100 iflag=fullblock conv=swab in that the latter would
swap every pair of input bytes, regardless of how many bytes
are returned by individual 'read' system calls, whereas the
former would not swap the last byte of each input record that
has an odd size.

Except -- GNU dd doesn't conform to POSIX here!  It swaps
bytes in the ibs=100 obs=100 conv=swab case, even when an
input record has an odd number of bytes and POSIX says the
last byte shouldn't be swapped.

For example:

(echo ab; sleep 1; echo cd) | dd ibs=100 conv=swab 2>/dev/null

POSIX says the output should be:

ba
dc

(and Solaris dd agrees with POSIX here).  But GNU dd outputs:

bac


(with a blank line at the end).

I suspect that this incompatibility was put in before the fullblock
flag was added, because of the need to be able to swap bytes reliably.
However, this need is better served by the fullblock option, so we
should remove the incompatibility.

Message #66 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Tue, 01 Mar 2011 21:40:00 +0000

On 01/03/11 17:45, Paul Eggert wrote:
> On 03/01/2011 03:27 AM, Pádraig Brady wrote:
> 
>> So the standard way to accumulate short reads to a full write,
>> is to specify separate ibs and obs (we'd probably want to prompt about
>> setting obs too for efficiency)
> 
> Yes, good point, the diagnostic should suggest ibs=N obs=N
> (instead of just ibs=N).
> 
> By the way, the relationship between fullblock and ibs=N obs=N is
> a curious one, one that I don't fully understand.  If you have
> ibs=N obs=N, why would you need fullblock?  This should probably
> be documented (preferably by someone who understands it :-).

Well as I understand it, it's to do with 'count'.
count refers to the number of input reads,
both partial and full.

So the advice to use iflag=fullblock is probably safer,
especially when a count (or skip) is specified.

cheers,
Pádraig.

Message #69 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 12:53:21 +0000

[Message part 1 (text/plain, inline)]

On 01/03/11 21:40, Pádraig Brady wrote:
> On 01/03/11 17:45, Paul Eggert wrote:
>> On 03/01/2011 03:27 AM, Pádraig Brady wrote:
>>
>>> So the standard way to accumulate short reads to a full write,
>>> is to specify separate ibs and obs (we'd probably want to prompt about
>>> setting obs too for efficiency)
>>
>> Yes, good point, the diagnostic should suggest ibs=N obs=N
>> (instead of just ibs=N).
>>
>> By the way, the relationship between fullblock and ibs=N obs=N is
>> a curious one, one that I don't fully understand.  If you have
>> ibs=N obs=N, why would you need fullblock?  This should probably
>> be documented (preferably by someone who understands it :-).
> 
> Well as I understand it, it's to do with 'count'.
> count refers to the number of input reads,
> both partial and full.
> 
> So the advice to use iflag=fullblock is probably safer,
> especially when a count (or skip) is specified.

Thinking about it more, we should at least split up the patch.
So for the oflag=direct case the attached just enables fullblock
(as using C_TWOBUFS would require more mem, CPU, and also messes
up if the user specified a count).

I'm not sure we should try to be more clever than this,
and accept that dd is a low level tool that can be
used in a myriad of ways.

cheers,
Pádraig.

[dd-fullblock.diff (text/x-patch, attachment)]

Message #72 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 13:12:36 +0000

On 01/03/11 19:30, Paul Eggert wrote:
> On 03/01/2011 09:45 AM, Paul Eggert wrote:
>> By the way, the relationship between fullblock and ibs=N obs=N is
>> a curious one, one that I don't fully understand.  If you have
>> ibs=N obs=N, why would you need fullblock?  This should probably
>> be documented (preferably by someone who understands it :-).
> 
> In looking into this some more, I did find one difference
> (or at least, something that *should* be a difference).
> 
> POSIX says that when an input record has
> an odd size, conv=swab should output the last byte as-is.
> If dd obeyed POSIX, ibs=100 obs=100 conv=swab would differ
> from bs=100 iflag=fullblock conv=swab in that the latter would
> swap every pair of input bytes, regardless of how many bytes
> are returned by individual 'read' system calls, whereas the
> former would not swap the last byte of each input record that
> has an odd size.
> 
> Except -- GNU dd doesn't conform to POSIX here!  It swaps
> bytes in the ibs=100 obs=100 conv=swab case, even when an
> input record has an odd number of bytes and POSIX says the
> last byte shouldn't be swapped.
> 
> For example:
> 
> (echo ab; sleep 1; echo cd) | dd ibs=100 conv=swab 2>/dev/null
> 
> POSIX says the output should be:
> 
> ba
> dc
> 
> (and Solaris dd agrees with POSIX here).  But GNU dd outputs:
> 
> bac
> 
> 
> (with a blank line at the end).
> 
> I suspect that this incompatibility was put in before the fullblock
> flag was added, because of the need to be able to swap bytes reliably.
> However, this need is better served by the fullblock option, so we
> should remove the incompatibility.

Oh good spot.

One can use an odd block size to test directly:

[solaris-10]$ echo abcde | dd bs=3 conv=swab 2>/dev/null
baced
[solaris-10]$ printf "abcd\n" | dd bs=3 conv=swab 2>/dev/null
bac
[solaris-10]$ printf "abcd" | dd bs=3 conv=swab 2>/dev/null
bacSegmentation Fault (core dumped)

So solaris is not being POSIX compliant either :)

Seriously though, changing to output the trailing byte directly
might break existing scripts that are swapping from a pipe.
I suppose we could warn to use fullblock if
  (!input_seekable && C_SWAB && !ibs%2 && nread%2)
though that would warn for your example above.
So I'm not sure how to proceed here.

cheers,
Pádraig.

Message #75 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 14:36:33 +0100

Pádraig Brady wrote:
> On 01/03/11 21:40, Pádraig Brady wrote:
>> On 01/03/11 17:45, Paul Eggert wrote:
>>> On 03/01/2011 03:27 AM, Pádraig Brady wrote:
>>>
>>>> So the standard way to accumulate short reads to a full write,
>>>> is to specify separate ibs and obs (we'd probably want to prompt about
>>>> setting obs too for efficiency)
>>>
>>> Yes, good point, the diagnostic should suggest ibs=N obs=N
>>> (instead of just ibs=N).
>>>
>>> By the way, the relationship between fullblock and ibs=N obs=N is
>>> a curious one, one that I don't fully understand.  If you have
>>> ibs=N obs=N, why would you need fullblock?  This should probably
>>> be documented (preferably by someone who understands it :-).
>>
>> Well as I understand it, it's to do with 'count'.
>> count refers to the number of input reads,
>> both partial and full.
>>
>> So the advice to use iflag=fullblock is probably safer,
>> especially when a count (or skip) is specified.
>
> Thinking about it more, we should at least split up the patch.
> So for the oflag=direct case the attached just enables fullblock
> (as using C_TWOBUFS would require more mem, CPU, and also messes
> up if the user specified a count).
>
> I'm not sure we should try to be more clever than this,
> and accept that dd is a low level tool that can be
> used in a myriad of ways.
...
> Subject: [PATCH] dd: enable iflag=fullblock for oflag=direct or oflag=cio
>
> * NEWS: Mention the change in behavior.
> * doc/coreutils.texi: Document when iflag=fullblock is implied.
> * src/dd.c (scan_args): Enable O_FULLBLOCK when needed.
...
> +** Changes in behavior
> +
> +  dd now enables iflag=fullblock with oflag=direct or oflag=cio
> +  where short reads can have adverse effects.

Thanks.  This looks fine to me.
It is so targeted and affects dd only when a non-POSIX flag is specified,
that I can't imagine it would cause any trouble.

Message #78 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 09:55:51 -0800

On 03/02/2011 04:53 AM, Pádraig Brady wrote:
> +  /* Enable 'fullblock' with 'direct' or 'cio' as we don't want to
> +     write partial blocks to output, which would disable O_DIRECT. An
> +     alternative would be to enable C_TWOBUFS to accumulate full output
> +     blocks.  However that wouldn't work when a count is specified, and
> +     is also less efficient.  */
> +  if (output_flags & (O_DIRECT | O_CIO))
> +    input_flags |= O_FULLBLOCK;

I'm afraid this patch feels wrong somehow.  It's conflating four
issues: counting, efficiency, disabling O_DIRECT, and disabling O_CIO.
Some thoughts:

1.  Offhand I don't see why O_CIO has anything to do with blocking;
why is it mentioned here?

2.  If C_TWOBUFS is already in effect, then dd needn't set
O_FULLBLOCK, since C_TWOBUFS already prevents the disabling of
O_DIRECT.

3.  The counting issue is independent of oflag=direct or oflag=cio:
any solution for counting should work regardless of oflag settings.

The more I think about it, the more I suspect that this
O_FULLBLOCK-inferring code should be omitted.  It adds little benefit, as
it covers a rare combination of flags that is likely to be used only
by experts, who should know the gotchas in this area anyway.  And the
extra complexity in documentation will penalize everybody who reads
it, even the non-experts.

Message #81 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 23:06:43 +0000

On 02/03/11 17:55, Paul Eggert wrote:
> On 03/02/2011 04:53 AM, Pádraig Brady wrote:
>> +  /* Enable 'fullblock' with 'direct' or 'cio' as we don't want to
>> +     write partial blocks to output, which would disable O_DIRECT. An
>> +     alternative would be to enable C_TWOBUFS to accumulate full output
>> +     blocks.  However that wouldn't work when a count is specified, and
>> +     is also less efficient.  */
>> +  if (output_flags & (O_DIRECT | O_CIO))
>> +    input_flags |= O_FULLBLOCK;
>
> I'm afraid this patch feels wrong somehow.  It's conflating four
> issues: counting, efficiency, disabling O_DIRECT, and disabling O_CIO.
> Some thoughts:
>
> 1.  Offhand I don't see why O_CIO has anything to do with blocking;
> why is it mentioned here?

Well in my quick googling it seemed to be a superset
of O_DIRECT mode, with the same alignment constraints.
It seems to fall back to a standard (synchronous) write
if the alignment is not maintained (we should always be OK there),
so I thought it safer to add it given its close relationship
with O_DIRECT. I'll remove it until I can actually test this.

>
> 2.  If C_TWOBUFS is already in effect, then dd needn't set
> O_FULLBLOCK, since C_TWOBUFS already prevents the disabling of
> O_DIRECT.

True. But it shouldn't cause an issue.
I'll leave as is to simplify the code and docs.
I'll amend the comment.

> 3.  The counting issue is independent of oflag=direct or oflag=cio:
> any solution for counting should work regardless of oflag settings.

True. I'll amend the comment.

> The more I think about it, the more I suspect that this
> O_FULLBLOCK-inferring code should be omitted.  It adds little benefit, as
> it covers a rare combination of flags that is likely to be used only
> by experts, who should know the gotchas in this area anyway.  And the
> extra complexity in documentation will penalize everybody who reads
> it, even the non-experts.

Well it has caught people out recently:
https://bugzilla.redhat.com/show_bug.cgi?id=614605
https://lkml.org/lkml/2011/2/22/746

I'm still inclined to add this.

Thanks for all the insightful comments on this BTW.
dd really is hairy.

Pádraig.

Message #84 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 23:29:23 +0000

On 02/03/11 17:55, Paul Eggert wrote:
> On 03/02/2011 04:53 AM, Pádraig Brady wrote:
>> +  /* Enable 'fullblock' with 'direct' or 'cio' as we don't want to
>> +     write partial blocks to output, which would disable O_DIRECT. An
>> +     alternative would be to enable C_TWOBUFS to accumulate full output
>> +     blocks.  However that wouldn't work when a count is specified, and
>> +     is also less efficient.  */
>> +  if (output_flags & (O_DIRECT | O_CIO))
>> +    input_flags |= O_FULLBLOCK;
> 
> I'm afraid this patch feels wrong somehow.  It's conflating four
> issues: counting, efficiency, disabling O_DIRECT, and disabling O_CIO.
> Some thoughts:
> 
> 1.  Offhand I don't see why O_CIO has anything to do with blocking;
> why is it mentioned here?
> 
> 2.  If C_TWOBUFS is already in effect, then dd needn't set
> O_FULLBLOCK, since C_TWOBUFS already prevents the disabling of
> O_DIRECT.
> 
> 3.  The counting issue is independent of oflag=direct or oflag=cio:
> any solution for counting should work regardless of oflag settings.
> 
> The more I think about it, the more I suspect that this
> O_FULLBLOCK-inferring code should be omitted.  It adds little benefit, as
> it covers a rare combination of flags that is likely to be used only
> by experts, who should know the gotchas in this area anyway.  And the
> extra complexity in documentation will penalize everybody who reads
> it, even the non-experts.

Hmm I actually thought of something that may be better.
The O_DIRECT -> normal switch done in iwrite() for the
last write, should only happen once.
If it happens more than that, we can print a warning,
suggesting the use of iflag=fullblock.

cheers,
Pádraig.

Message #87 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Wed, 02 Mar 2011 16:11:15 -0800

On 03/02/2011 03:29 PM, Pádraig Brady wrote:
> The O_DIRECT ->  normal switch done in iwrite() for the
> last write, should only happen once.
> If it happens more than that, we can print a warning,
> suggesting the use of iflag=fullblock.

Yes, that sounds like a better approach.
Thanks for thinking this through.

I looked into O_CIO and it appears that it does
imply O_DIRECT, but O_DIRECT does not have the
problems on AIX (which has O_CIO) that I guess it
has on GNU/Linux.  Quoting from
<http://www-03.ibm.com/systems/resources/systems_p_os_aix_whitepapers_db_perf_aix.pdf>
  
  The use of Direct I/O requires that certain
  alignment and length restrictions be met by the
  application’s I/O requests. Table 1 lists these
  requirements for JFS2. Failure to meet these
  requirements causes reads and writes to be done
  using normal cached I/O, but after the data is
  transferred to the application buffer, the cached
  copy is discarded.

which suggests that iwrite's fcntl in iwrite is
not needed on AIX.  I don't know if this is worth
optimizing, but perhaps it's worth a comment as
to why we worry about O_DIRECT but not O_CIO in
iwrite.

Message #90 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Thu, 03 Mar 2011 23:44:15 -0800

On 03/02/2011 05:12 AM, Pádraig Brady wrote:

> So I'm not sure how to proceed here.

Before worrying about swab, we should at least try to get a warning
out for the short-read case we discussed earlier.  Here's a revision
of my earlier proposal, with a higher-quality warning.  Quite possibly
we can fold a swab warning into this code later, but one thing at
a time.

2011-03-04  Paul Eggert  <eggert <at> cs.ucla.edu>

	dd: avoid or diagnose some problems with short reads
	* src/dd.c (iread): Diagnose short reads when they mess up counts.
	Derived from a suggestion by Pádraig Brady in:
	http://lists.gnu.org/archive/html/bug-coreutils/2011-02/msg00150.html

diff --git a/NEWS b/NEWS
index a367d8d..32cc7b0 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   reproduce them efficiently in the output file.  mv also benefits
   when it resorts to copying, e.g., between file systems.
 
+  dd bs=BYTES, with either count=BLOCKS or skip=BLOCKS and without
+  iflag=fullblock, now warns if a short block is read (not at end of
+  file).  This helps avoid confusion when counting or skipping bytes.
+
   join now supports -o 'auto' which will automatically infer the
   output format from the first line in each file, to ensure
   the same number of fields are output for each line.
diff --git a/src/dd.c b/src/dd.c
index daddc1e..39fa3ab 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -796,14 +796,43 @@ process_signals (void)
 static ssize_t
 iread (int fd, char *buf, size_t size)
 {
-  while (true)
+  ssize_t nread;
+
+  do
     {
-      ssize_t nread;
       process_signals ();
       nread = read (fd, buf, size);
-      if (! (nread < 0 && errno == EINTR))
-        return nread;
     }
+  while (nread < 0 && errno == EINTR);
+
+  if (0 < nread)
+    {
+      /* If bs=SIZE is given and iflag=fullblock is not, warn if a
+         short block was read (not at EOF), and either count=BLOCKS or
+         skip=BLOCKS is also given.  This helps avoid confusion when
+         counting or skipping bytes.  */
+      static bool warned;
+      static ssize_t prev_nread;
+
+      if (! warned && iread_fnc == iread
+          && 0 < prev_nread && prev_nread < size
+          && (skip_records
+              || (0 < max_records && max_records < (uintmax_t) -1)))
+        {
+          uintmax_t prev = prev_nread;
+          error (0, 0, ngettext (("warning: short read (%"PRIuMAX" byte); "
+                                  "suggest iflag=fullblock"),
+                                 ("warning: short read (%"PRIuMAX" bytes); "
+                                  "suggest iflag=fullblock"),
+                                 select_plural (prev)),
+                 prev);
+          warned = true;
+        }
+
+      prev_nread = nread;
+    }
+
+  return nread;
 }
 
 /* Wrapper around iread function to accumulate full blocks.  */

Message #93 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 04 Mar 2011 10:09:54 +0000

On 04/03/11 07:44, Paul Eggert wrote:
> +      /* If bs=SIZE is given and iflag=fullblock is not, warn if a

Do you check that bs= is specified?
Do you want to as it's independent of the counting issue?

Anyway, with this patch the following slightly contrived example will warn:

# Output first 2 parts
$ (echo part1; sleep 1; echo part2; sleep 1; echo discard) |
  dd count=2 obs=1 2>/dev/null
part1
part2

So I'm a bit wary about adding this at all.

cheers,
Pádraig.

Message #96 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 04 Mar 2011 10:04:02 -0800

On 03/04/2011 02:09 AM, Pádraig Brady wrote:
> On 04/03/11 07:44, Paul Eggert wrote:
>> +      /* If bs=SIZE is given and iflag=fullblock is not, warn if a
>
> Do you check that bs= is specified?

I meant to, but I inadvertently deleted that part of the change,
which meant that the code didn't implement the comment correctly.
Sorry about that; see below.

> Anyway, with this patch the following slightly contrived example will warn:

The following further patch, which fixes the abovementioned typo,
should address that problem.

--- a/src/dd.c
+++ b/src/dd.c
@@ -814,7 +814,7 @@ iread (int fd, char *buf, size_t size)
       static bool warned;
       static ssize_t prev_nread;
 
-      if (! warned && iread_fnc == iread
+      if (! warned && ! (conversions_mask & C_TWOBUFS) && iread_fnc == iread
           && 0 < prev_nread && prev_nread < size
           && (skip_records
               || (0 < max_records && max_records < (uintmax_t) -1)))

Message #99 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 04 Mar 2011 22:41:59 +0000

On 04/03/11 18:04, Paul Eggert wrote:
> On 03/04/2011 02:09 AM, Pádraig Brady wrote:
>> On 04/03/11 07:44, Paul Eggert wrote:
>>> +      /* If bs=SIZE is given and iflag=fullblock is not, warn if a
>>
>> Do you check that bs= is specified?
> 
> I meant to, but I inadvertently deleted that part of the change,
> which meant that the code didn't implement the comment correctly.
> Sorry about that; see below.
> 
>> Anyway, with this patch the following slightly contrived example will
>> warn:
> 
> The following further patch, which fixes the abovementioned typo,
> should address that problem.
> 
> --- a/src/dd.c
> +++ b/src/dd.c
> @@ -814,7 +814,7 @@ iread (int fd, char *buf, size_t size)
>        static bool warned;
>        static ssize_t prev_nread;
>  
> -      if (! warned && iread_fnc == iread
> +      if (! warned && ! (conversions_mask & C_TWOBUFS) && iread_fnc ==
> iread
>            && 0 < prev_nread && prev_nread < size

Looks good!
I can't think of an example where this might erroneously warn.

>            && (skip_records
>                || (0 < max_records && max_records < (uintmax_t) -1)))

If we more aggressively warn by removing the check for the counts above,
then we would directly address http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8171
Unfortunately that would give unwanted warnings though, in some cases.

cheers,
Pádraig.

Message #102 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Sat, 05 Mar 2011 01:44:16 +0000

I'm going to apply the following.

commit decea1c17bcbce3f70bfb50b7de66de5eaba9b91
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Fri Feb 25 12:27:25 2011 +0000

    dd: warn when we disable oflag=direct not at EOF

    An alternative to this is to auto enable iflag=fullblock
    when oflag=direct and bs= is specified.
    It was thought better though, to warn about the specific issue,
    and give full control of dd's options to the user.

    * src/dd.c (iwrite): Warn, when we write after having
    disabled O_DIRECT.
    See https://bugzilla.redhat.com/show_bug.cgi?id=614605

diff --git a/src/dd.c b/src/dd.c
index daddc1e..fd468a6 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -837,6 +837,12 @@ iwrite (int fd, char const *buf, size_t size)
 {
   size_t total_written = 0;

+  if ((output_flags & O_DIRECT) && w_partial == 1)
+    {
+      error (0, 0, _("dd: warning: partial read; oflag=direct disabled; "
+                     "suggest iflag=fullblock"));
+    }
+
   if ((output_flags & O_DIRECT) && size < output_blocksize)
     {
       int old_flags = fcntl (STDOUT_FILENO, F_GETFL);

Message #105 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Fri, 04 Mar 2011 19:29:08 -0800

On 03/04/2011 05:44 PM, Pádraig Brady wrote:
> +  if ((output_flags & O_DIRECT) && w_partial == 1)
> +    {
> +      error (0, 0, _("dd: warning: partial read; oflag=direct disabled; "
> +                     "suggest iflag=fullblock"));

This diagnostic looks wrong.  w_partial means there
was a partial *write*, not a partial *read*.  Anyway,
we should use a better overall solution for partial
reads; I'll try to come up with one.

Message #108 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Sat, 05 Mar 2011 00:17:45 -0800

On 03/04/2011 07:29 PM, Paul Eggert wrote:
> we should use a better overall solution for partial
> reads; I'll try to come up with one.

Here is a proposed change for that, incorporating
the recent suggestions plus a couple more ideas:

From 11b600b65dce686ee9320b5d851fbd1b0beaa832 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sat, 5 Mar 2011 00:14:25 -0800
Subject: [PATCH] dd: diagnose some problems with partial reads

* src/dd.c (warn_partial_read): New static var.
(iread): Diagnose partial reads if needed.
(iwrite): Don't diagnose them here; not needed any more.
(scanargs): Determine whether partial reads should be diagnosted.
---
 src/dd.c |   53 +++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index 6069671..3472442 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -199,6 +199,9 @@ static int input_seek_errno;
 static uintmax_t input_offset;
 static bool input_offset_overflow;
 
+/* True if a partial read should be diagnosed.  */
+static bool warn_partial_read;
+
 /* Records truncated by conv=block. */
 static uintmax_t r_truncate = 0;
 
@@ -894,14 +897,35 @@ invalidate_cache (int fd, off_t len)
 static ssize_t
 iread (int fd, char *buf, size_t size)
 {
-  while (true)
+  ssize_t nread;
+
+  do
     {
-      ssize_t nread;
       process_signals ();
       nread = read (fd, buf, size);
-      if (! (nread < 0 && errno == EINTR))
-        return nread;
     }
+  while (nread < 0 && errno == EINTR);
+
+  if (0 < nread && warn_partial_read)
+    {
+      static ssize_t prev_nread;
+
+      if (0 < prev_nread && prev_nread < size)
+        {
+          uintmax_t prev = prev_nread;
+          error (0, 0, ngettext (("warning: partial read (%"PRIuMAX" byte); "
+                                  "suggest iflag=fullblock"),
+                                 ("warning: partial read (%"PRIuMAX" bytes); "
+                                  "suggest iflag=fullblock"),
+                                 select_plural (prev)),
+                 prev);
+          warn_partial_read = false;
+        }
+
+      prev_nread = nread;
+    }
+
+  return nread;
 }
 
 /* Wrapper around iread function to accumulate full blocks.  */
@@ -935,12 +959,6 @@ iwrite (int fd, char const *buf, size_t size)
 {
   size_t total_written = 0;
 
-  if ((output_flags & O_DIRECT) && w_partial == 1)
-    {
-      error (0, 0, _("dd: warning: partial read; oflag=direct disabled; "
-                     "suggest iflag=fullblock"));
-    }
-
   if ((output_flags & O_DIRECT) && size < output_blocksize)
     {
       int old_flags = fcntl (STDOUT_FILENO, F_GETFL);
@@ -1175,7 +1193,7 @@ scanargs (int argc, char *const *argv)
     input_blocksize = output_blocksize = blocksize;
   else
     {
-      /* POSIX says dd aggregates short reads into
+      /* POSIX says dd aggregates partial reads into
          output_blocksize if bs= is not specified.  */
       conversions_mask |= C_TWOBUFS;
     }
@@ -1195,6 +1213,17 @@ scanargs (int argc, char *const *argv)
       error (0, 0, "%s: %s", _("invalid output flag"), "'fullblock'");
       usage (EXIT_FAILURE);
     }
+
+  /* Warn about partial reads if bs=SIZE is given and iflag=fullblock
+     is not, and if counting or skipping bytes or using direct I/O.
+     This helps to avoid confusion with miscounts, and to avoid issues
+     with direct I/O on GNU/Linux.  */
+  warn_partial_read =
+    (! (conversions_mask & C_TWOBUFS) && ! (input_flags & O_FULLBLOCK)
+     && (skip_records
+         || (0 < max_records && max_records < (uintmax_t) -1)
+         || (input_flags | output_flags) & O_DIRECT));
+
   iread_fnc = ((input_flags & O_FULLBLOCK)
                ? iread_fullblock
                : iread);
@@ -1758,7 +1787,7 @@ dd_copy (void)
          There are 3 reasons why there might be unskipped blocks/bytes:
              1. file is too small
              2. pipe has not enough data
-             3. short reads  */
+             3. partial reads  */
       if (us_blocks || (!input_offset_overflow && us_bytes))
         {
           error (0, 0,
-- 
1.7.4

Message #111 received at 7362 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7362 <at> debbugs.gnu.org
Subject: Re: bug#7362: dd strangeness
Date: Sat, 05 Mar 2011 09:49:01 +0000

On 05/03/11 08:17, Paul Eggert wrote:
> On 03/04/2011 07:29 PM, Paul Eggert wrote:
>> we should use a better overall solution for partial
>> reads; I'll try to come up with one.
> 
> Here is a proposed change for that, incorporating
> the recent suggestions plus a couple more ideas:

OK I like this since it consolidates the code.
It's slightly less general in that it would
erroneously warn with conv=sync oflag=direct,
but that's not a practical concern.

So Ack.

cheers,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.