Core Development Tools/Discussion/General/Review diff for cmp/List of Posts
 
Forum Topic - Review diff for cmp:
   
Update
Adam Mallory(deleted)
10/07/2009 12:22 PM
post39535
Review diff for cmp   
Here is a diff to fix an issue brought up in the OS-tech forum.  Recapping the test case:

To reproduce the bug:

echo "hello" > 1.txt

echo "hello" > 2.txt
echo "world" >> 2.txt

cmp 1.txt 2.txt

echo "$?"

it return "0", but it should return "1" and print a EOF error on stderr according to the documentation and to other 
implementation of cmp.

The fix tracks what should have been eof state in the previous versions.
Attachment: Text cmp.diff 1.18 KB
Neil Schellenberger(deleted)
10/07/2009 12:46 PM
post39540
Re: Review diff for cmp   
Hi Adam,

No fault of your change, but that code's simply not going to work
properly for pipes or FIFOs.  The code really ought to take short reads
into account.

FWIW, I'd probably initialize the two prev_* variables to zero or else
comparing two zero length files might conceivably produce "interesting"
results.

Regards,
Neil
Adam Mallory(deleted)
10/07/2009 1:15 PM
post39546
Re: Review diff for cmp   
On 7-Oct-09, at 12:46 PM, Neil Schellenberger wrote:

> No fault of your change, but that code's simply not going to work
> properly for pipes or FIFOs.  The code really ought to take short  
> reads
> into account.

Yeah, short reads never really were taken into account.  It is rather  
unfortunate.

>
> FWIW, I'd probably initialize the two prev_* variables to zero or else
> comparing two zero length files might conceivably produce  
> "interesting"
> results.

They are global so there are init'd to zero by definition.

--
Cheers,
   Adam

  QNX Software Systems
  [ amallory@qnx.com ]
  ---------------------------------------------------
Neil Schellenberger(deleted)
10/07/2009 2:20 PM
post39551
Re: Review diff for cmp   
On Wed, 2009-10-07 at 13:15 -0400, Adam Mallory wrote:
> They are global so there are init'd to zero by definition.

Whoops, right you are; only looked at the diff and not the context,
sorry.  (Does beg the question why they need to be global, but that's
just me being pedantic....)
Adam Mallory(deleted)
10/07/2009 4:17 PM
post39558
Re: Review diff for cmp   
Style rules dictate I should keep things generally the same style.

On 7-Oct-09, at 2:20 PM, Neil Schellenberger wrote:

>
> Whoops, right you are; only looked at the diff and not the context,
> sorry.  (Does beg the question why they need to be global, but that's
> just me being pedantic....)

--
Cheers,
   Adam

  QNX Software Systems
  [ amallory@qnx.com ]
  ---------------------------------------------------
Neil Schellenberger(deleted)
10/07/2009 4:18 PM
post39559
Re: Review diff for cmp   
On Wed, 2009-10-07 at 16:17 -0400, Adam Mallory wrote:
> Style rules dictate I should keep things generally the same style.

Sorry, didn't mean to imply that you should change it.  It was more of a
general and rather pedantic observation.
Steve Reid(deleted)
10/07/2009 12:47 PM
post39542
RE: Review diff for cmp   
What's the PR number? Given that it was a customer who found this, we
should mention it in the release notes when it's fixed.

Thanks.

Steve Reid (stever@qnx.com)
Technical Editor
QNX Software Systems 
 

> -----Original Message-----
> From: Adam Mallory [mailto:community-noreply@qnx.com] 
> Sent: Wednesday, October 07, 2009 12:22 PM
> To: general-toolchain
> Subject: Review diff for cmp
> 
> Here is a diff to fix an issue brought up in the OS-tech 
> forum.  Recapping the test case:
> 
> To reproduce the bug:
> 
> echo "hello" > 1.txt
> 
> echo "hello" > 2.txt
> echo "world" >> 2.txt
> 
> cmp 1.txt 2.txt
> 
> echo "$?"
> 
> it return "0", but it should return "1" and print a EOF error 
> on stderr according to the documentation and to other 
> implementation of cmp.
> 
> The fix tracks what should have been eof state in the 
> previous versions.
> 
> 
> 
> _______________________________________________
> 
> General
> http://community.qnx.com/sf/go/post39535
>
Adam Mallory(deleted)
10/07/2009 1:16 PM
post39547
Re: Review diff for cmp   
I'll open a PR since I need to for any check in.
-Adam

On 7-Oct-09, at 12:47 PM, Steve Reid wrote:

> What's the PR number? Given that it was a customer who found this, we
> should mention it in the release notes when it's fixed.
>
> Thanks.
>
> Steve Reid (stever@qnx.com)
> Technical Editor
> QNX Software Systems
>
>
>> -----Original Message-----
>> From: Adam Mallory [mailto:community-noreply@qnx.com]
>> Sent: Wednesday, October 07, 2009 12:22 PM
>> To: general-toolchain
>> Subject: Review diff for cmp
>>
>> Here is a diff to fix an issue brought up in the OS-tech
>> forum.  Recapping the test case:
>>
>> To reproduce the bug:
>>
>> echo "hello" > 1.txt
>>
>> echo "hello" > 2.txt
>> echo "world" >> 2.txt
>>
>> cmp 1.txt 2.txt
>>
>> echo "$?"
>>
>> it return "0", but it should return "1" and print a EOF error
>> on stderr according to the documentation and to other
>> implementation of cmp.
>>
>> The fix tracks what should have been eof state in the
>> previous versions.
>>
>>
>>
>> _______________________________________________
>>
>> General
>> http://community.qnx.com/sf/go/post39535
>>
>
>
>
> _______________________________________________
>
> General
> http://community.qnx.com/sf/go/post39542
>

--
Cheers,
   Adam

  QNX Software Systems
  [ amallory@qnx.com ]
  ---------------------------------------------------
Adam Mallory(deleted)
01/25/2010 2:41 PM
post45885
Re: Review diff for cmp   
I'm resurrecting this review as Neil's point about short reads made me want to refactor things a little.  This takes 
care of short reads so you can use pipes.  I have to avoid using the real STDIO/stream gear as it was very heavy both in
 overhead and efficiency.

PR:74130

Comments
Attachment: Text cmp.refactor.diff 5.26 KB

Return