[PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

For regular file, write() operation may also fail, so check it too. If
write() return 0, can simply wait and try again, it should not suspend
infinitely if environments have no critical issues.

The related warning (cross compile for aarch64-linux):

  ../../binutils-gdb/readline/histfile.c: In function ‘history_truncate_file’:
  ../../binutils-gdb/readline/histfile.c:406:13: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused

readline/
    * readline/histfile.c (history_truncate_file):
    Check and retry write() operation.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 readline/ChangeLog.gdb |  5 +++++
 readline/histfile.c    | 25 +++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/readline/ChangeLog.gdb b/readline/ChangeLog.gdb
index 1218fd7..f547a90 100644
--- a/readline/ChangeLog.gdb
+++ b/readline/ChangeLog.gdb
@@ -1,3 +1,8 @@
+2014-06-11  Chen Gang <gang.chen.5i5j@gmail.com>
+
+	* readline/histfile.c (history_truncate_file):
+	Check and retry write() operation.
+
 2013-09-24  Pierre Muller  <muller@sourceware.org>
 
 	* readline.c (bind_arrow_keys_internal):
diff --git a/readline/histfile.c b/readline/histfile.c
index 30a6182..66597a7 100644
--- a/readline/histfile.c
+++ b/readline/histfile.c
@@ -403,11 +403,32 @@ history_truncate_file (fname, lines)
      truncate to. */
   if (bp > buffer && ((file = open (filename, O_WRONLY|O_TRUNC|O_BINARY, 0600)) != -1))
     {
-      write (file, bp, chars_read - (bp - buffer));
+      int chars_write;
+#if defined (__BEOS__)
+      char *orig = bp;
+#endif
+
+      do
+        {
+          chars_write = write (file, bp, chars_read - (bp - buffer));
+          if (chars_write < 0)
+            {
+              rv = errno;
+              close (file);
+              goto truncate_exit;
+            }
+          else if (chars_write == 0)
+            {
+              usleep (1000);
+              continue;
+            }
+          bp += chars_write;
+        }
+      while (chars_read - (bp - buffer) > 0);
 
 #if defined (__BEOS__)
       /* BeOS ignores O_TRUNC. */
-      ftruncate (file, chars_read - (bp - buffer));
+      ftruncate (file, chars_read - (orig - buffer));
 #endif
 
       close (file);
-- 
1.9.2.459.g68773ac

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chet Ramey]

On 6/10/14, 10:35 PM, Chen Gang wrote:
> For regular file, write() operation may also fail, so check it too. If
> write() return 0, can simply wait and try again, it should not suspend
> infinitely if environments have no critical issues.

Readline-6.3 checks the return value from write() and returns a non-zero
value to the history_truncate_file caller.  I really don't think that
waiting forever if write continues to return 0 is a great idea; an error
return is enough to let the caller deal with it.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

On 06/19/2014 04:33 AM, Chet Ramey wrote:
> On 6/10/14, 10:35 PM, Chen Gang wrote:
>> For regular file, write() operation may also fail, so check it too. If
>> write() return 0, can simply wait and try again, it should not suspend
>> infinitely if environments have no critical issues.
> 
> Readline-6.3 checks the return value from write() and returns a non-zero
> value to the history_truncate_file caller.  I really don't think that
> waiting forever if write continues to return 0 is a great idea; an error
> return is enough to let the caller deal with it.
> 

That sounds fine to me, and I will send patch v2 for it. And excuse me,
I have to do some other things today, so I shall try to send it within 2
days (within 2014-06-21) -- if it is too late, please help send it.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

On 06/19/2014 09:31 AM, Chen Gang wrote:
> 
> 
> On 06/19/2014 04:33 AM, Chet Ramey wrote:
>> On 6/10/14, 10:35 PM, Chen Gang wrote:
>>> For regular file, write() operation may also fail, so check it too. If
>>> write() return 0, can simply wait and try again, it should not suspend
>>> infinitely if environments have no critical issues.
>>
>> Readline-6.3 checks the return value from write() and returns a non-zero
>> value to the history_truncate_file caller.  I really don't think that
>> waiting forever if write continues to return 0 is a great idea; an error
>> return is enough to let the caller deal with it.
>>

Oh, sorry, after think of again, for me, we have to waiting forever if
write() continues to return 0.

When this case happens, the file is already truncated, and the left data
which is writing to file will be free after return from
history_truncate_file().

If return an error code in this case, the caller can not deal with it --
the log data which should be remained, have been lost, can not get them
back again.

> 
> That sounds fine to me, and I will send patch v2 for it. And excuse me,
> I have to do some other things today, so I shall try to send it within 2
> days (within 2014-06-21) -- if it is too late, please help send it.
> 

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chet Ramey]

On 6/20/14, 4:57 AM, Chen Gang wrote:
> On 06/19/2014 09:31 AM, Chen Gang wrote:
>>
>>
>> On 06/19/2014 04:33 AM, Chet Ramey wrote:
>>> On 6/10/14, 10:35 PM, Chen Gang wrote:
>>>> For regular file, write() operation may also fail, so check it too. If
>>>> write() return 0, can simply wait and try again, it should not suspend
>>>> infinitely if environments have no critical issues.
>>>
>>> Readline-6.3 checks the return value from write() and returns a non-zero
>>> value to the history_truncate_file caller.  I really don't think that
>>> waiting forever if write continues to return 0 is a great idea; an error
>>> return is enough to let the caller deal with it.
>>>
> 
> Oh, sorry, after think of again, for me, we have to waiting forever if
> write() continues to return 0.

There aren't really any plausible conditions under which write(2) returns
0 instead of -1 when writing a non-zero number of bytes to a regular file.

> 
> When this case happens, the file is already truncated, and the left data
> which is writing to file will be free after return from
> history_truncate_file().
> 
> If return an error code in this case, the caller can not deal with it --
> the log data which should be remained, have been lost, can not get them
> back again.

However, you're right about the data being lost if write fails and returns
-1. Since the sequence of operations is open-read-close-open-write-close, I
think I will change the code for the next version to use a scheme similar
to history_do_write() and restore the original version of the file if the
write fails.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

On 06/21/2014 05:53 AM, Chet Ramey wrote:
> On 6/20/14, 4:57 AM, Chen Gang wrote:
>> On 06/19/2014 09:31 AM, Chen Gang wrote:
>>>
>>>
>>> On 06/19/2014 04:33 AM, Chet Ramey wrote:
>>>> On 6/10/14, 10:35 PM, Chen Gang wrote:
>>>>> For regular file, write() operation may also fail, so check it too. If
>>>>> write() return 0, can simply wait and try again, it should not suspend
>>>>> infinitely if environments have no critical issues.
>>>>
>>>> Readline-6.3 checks the return value from write() and returns a non-zero
>>>> value to the history_truncate_file caller.  I really don't think that
>>>> waiting forever if write continues to return 0 is a great idea; an error
>>>> return is enough to let the caller deal with it.
>>>>
>>
>> Oh, sorry, after think of again, for me, we have to waiting forever if
>> write() continues to return 0.
> 
> There aren't really any plausible conditions under which write(2) returns
> 0 instead of -1 when writing a non-zero number of bytes to a regular file.
> 

Hmm... for me, what you said is acceptable.  And the function comment need be
changed:

  "Returns 0 on success, errno or -1 on failure"

>>
>> When this case happens, the file is already truncated, and the left data
>> which is writing to file will be free after return from
>> history_truncate_file().
>>
>> If return an error code in this case, the caller can not deal with it --
>> the log data which should be remained, have been lost, can not get them
>> back again.
> 
> However, you're right about the data being lost if write fails and returns
> -1. Since the sequence of operations is open-read-close-open-write-close, I
> think I will change the code for the next version to use a scheme similar
> to history_do_write() and restore the original version of the file if the
> write fails.
> 

OK, thanks.

And since you will work for it next, is it still necessary to send patch v2 for
the temporary fix, at present?


Thanks
-- 
Open share and attitude like air warter and life which God blessed

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chet Ramey]

On 6/20/14, 10:25 PM, Chen Gang wrote:

>> There aren't really any plausible conditions under which write(2) returns
>> 0 instead of -1 when writing a non-zero number of bytes to a regular file.
>>
> 
> Hmm... for me, what you said is acceptable.  And the function comment need be
> changed:
> 
>   "Returns 0 on success, errno or -1 on failure"

history_truncate_file will never return -1.

> And since you will work for it next, is it still necessary to send patch v2 for
> the temporary fix, at present?

I will take care of making those changes, thanks.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

On 06/22/2014 07:19 AM, Chet Ramey wrote:
> On 6/20/14, 10:25 PM, Chen Gang wrote:
> 
>>> There aren't really any plausible conditions under which write(2) returns
>>> 0 instead of -1 when writing a non-zero number of bytes to a regular file.
>>>
>>
>> Hmm... for me, what you said is acceptable.  And the function comment need be
>> changed:
>>
>>   "Returns 0 on success, errno or -1 on failure"
> 
> history_truncate_file will never return -1.
> 

Hmm... do you mean:

  "for regular file, write() never return 0, if parameter 'count' > 0?"

  or

  "if write() return 0, can also return 0 to history_truncate_file()?".


>> And since you will work for it next, is it still necessary to send patch v2 for
>> the temporary fix, at present?
> 
> I will take care of making those changes, thanks.

OK, thanks (I need/shall not send patch v2 for it).


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chet Ramey]

On 6/21/14, 10:01 PM, Chen Gang wrote:

>> history_truncate_file will never return -1.
>>
> 
> Hmm... do you mean:
> 
>   "for regular file, write() never return 0, if parameter 'count' > 0?"
> 
>   or
> 
>   "if write() return 0, can also return 0 to history_truncate_file()?".

Both of those things are true, but neither is what I said above.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

On 06/23/2014 09:57 PM, Chet Ramey wrote:
> On 6/21/14, 10:01 PM, Chen Gang wrote:
> 
>>> history_truncate_file will never return -1.
>>>
>>
>> Hmm... do you mean:
>>
>>   "for regular file, write() never return 0, if parameter 'count' > 0?"
>>

I am not quite sure whether it is true, in my experience, it should be
true, but I have no any proofs for it (if you have, welcome to supply).

thank.

>>   or
>>
>>   "if write() return 0, can also return 0 to history_truncate_file()?".
> 

For me, if write() could return 0, when it happened, we had to process the
case within history_truncate_file(), could not only return 0 to indicate
all things go on well.


> Both of those things are true, but neither is what I said above.
> 

If both of those things are not true, what your originally said above
are not true, either.


Thanks.
-- 
Open share and attitude like air warter and life which God blessed

Re: [Bug-readline] [PATCH] readline/histfile.c: Check and retry write() operation in history_truncate_file()

[Chen Gang]

And I guess, we can simply bypass this discussion, because the new fix
can process write() well and let history_truncate_file() never return -1.

Thanks.

On 06/23/2014 10:59 PM, Chen Gang wrote:
> On 06/23/2014 09:57 PM, Chet Ramey wrote:
>> On 6/21/14, 10:01 PM, Chen Gang wrote:
>>
>>>> history_truncate_file will never return -1.
>>>>
>>>
>>> Hmm... do you mean:
>>>
>>>   "for regular file, write() never return 0, if parameter 'count' > 0?"
>>>
> 
> I am not quite sure whether it is true, in my experience, it should be
> true, but I have no any proofs for it (if you have, welcome to supply).
> 
> thank.
> 
>>>   or
>>>
>>>   "if write() return 0, can also return 0 to history_truncate_file()?".
>>
> 
> For me, if write() could return 0, when it happened, we had to process the
> case within history_truncate_file(), could not only return 0 to indicate
> all things go on well.
> 
> 
>> Both of those things are true, but neither is what I said above.
>>
> 
> If both of those things are not true, what your originally said above
> are not true, either.
> 
> 
> Thanks.
> 


-- 
Chen Gang

Open share and attitude like air water and life which God blessed