Allow passing in strings of length zero to FileWrapper::Write without closing the file.

webrtc/system_wrappers/source/file_impl.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 225 matching lines @@
     return false;
 
   // Check if it's time to stop writing.
   if (max_size_in_bytes_ > 0 &&
       (size_in_bytes_ + length) > max_size_in_bytes_) {
     FlushImpl();
     return false;
   }
 
   size_t num_bytes = fwrite(buf, 1, length, id_);
-  if (num_bytes > 0) {
-    size_in_bytes_ += num_bytes;
+  size_in_bytes_ += num_bytes;

[Henrik Grunell WebRTC, 2016/02/19 15:04:38]

Keep this inside the if block below. Not needed if we close the file.

[terelius, 2016/02/19 16:50:29]

By not closing the file, we can remove the entire if block.

+  if (num_bytes == length) {

[Henrik Grunell WebRTC, 2016/02/19 15:04:38]

The original code is unclear. Good to change this.

1. Checking that we actually wrote the amount we wanted is good.

2. Should we really allow |length| == 0? The user of this class shouldn't really
call if there's nothing to write.

3. It seems strange that the file is closed here at all. The user should decide
what to do if an error happens.

I don't want to block this fix. If (2) and (3) this causes larger changes
outside this class, I'm fine with not changing that. Add a todo to clean this
up.

[terelius, 2016/02/19 16:05:52]

I agree on 1 and 3. I'd also prefer not closing the file. If you think that's
ok, I could shorten the function by 5 lines. I guess that the original author
chose to close the file because the file position indicator is undetermined
after an fwrite error.

Regarding 2: Although it is pointless to write an empty string, I think it is
nevertheless useful to allow since the string could be the result of some
computation. If Write disallows empty strings you'd have to add a check at every
call site (unless you can guarantee that the computation can't produce empty
strings). Moreover, those checks would duplicate checks that are done by fwrite
anyway.

Secondly, the description of the return value becomes more clear if empty
strings are allowed. Compare "true if |length| bytes were written" to "true if
|length| bytes were written and |length| is non-zero".

     return true;
   }
 
   CloseFileImpl();

[Henrik Grunell WebRTC, 2016/02/19 15:04:38]

Actually, do this if above condition isn't met instead. And let the successful
return be last.

[terelius, 2016/02/19 16:50:29]

By not closing the file, we can instead return the result of the condition
without branching.

[Henrik Grunell WebRTC, 2016/02/23 09:38:02]

Does the file need to be closed elsewhere instead? Actually, we don't know the
reason for failure, so we really can't. Just make sure removing the close here
doesn't cause any trouble.

   return false;
 }
 
 int FileWrapperImpl::CloseFileImpl() {
   if (id_ != NULL) {
     if (managed_file_handle_)
       fclose(id_);
     id_ = NULL;
   }
   memset(file_name_utf8_, 0, kMaxFileNameSize);
   open_ = false;
   return 0;
 }
 
 int FileWrapperImpl::FlushImpl() {
   if (id_ != NULL) {
     return fflush(id_);
   }
   return -1;
 }
 
 int FileWrapper::Rewind() {
   RTC_DCHECK(false);
   return -1;
 }
 
 }  // namespace webrtc