Re: [Nano-devel] [PATCH v2] pull in the futimens module from gnulib

[Kamil Dudka]

On Monday, April 03, 2017 20:02:44 Benno Schulenberg wrote:
> On Mon, Apr 3, 2017, at 10:13, Kamil Dudka wrote:
> >  /* Read from inn, write to out.  We assume inn is opened for reading,
> >  
> >   * and out for writing.  We return 0 on success, -1 on read error, or -2
> > 
> > - * on write error. */
> > -int copy_file(FILE *inn, FILE *out)
> > + * on write error.  inn is always closed by this function, out is closed
> > + * only if close_out is true. */
> > +int copy_file(FILE *inn, FILE *out, bool close_out)
> > 
> >  {
> >  
> >      int retval = 0;
> >      char buf[BUFSIZ];
> >      size_t charsread;
> > 
> > +    int (*flush_out_fnc)(FILE *) = (close_out) ? fclose : fflush;
> > 
> >      assert(inn != NULL && out != NULL && inn != out);
> > 
> > @@ -1564,7 +1566,7 @@ int copy_file(FILE *inn, FILE *out)
> > 
> >      if (fclose(inn) == EOF)
> >     
> >     retval = -1;
> > 
> > -    if (fclose(out) == EOF)
> > +    if (flush_out_fnc(out) == EOF)
> > 
> >     retval = -2;
> 
> Why is the flush needed?

The original code called fclose(), which flushes the output buffer.  If you 
did not flush, you would not be able to reliably detect write errors inside 
copy_file() and would need to explicitly handle errors on the subsequent call 
of fclose() at caller's side.

> Could the timestamp get changed if
> the actual writing happened a few seconds later?

Good point.  I guess this could also happen.

> > -   struct utimbuf filetime;
> > +   static struct timespec filetime[2];
> 
> Why does it need to be static?

Because static data is guaranteed to be initialized.  Otherwise you would
have to memset() it, or explicitly initialize the tv_nsec fields.

> > -   filetime.actime = openfile->current_stat->st_atime;
> > -   filetime.modtime = openfile->current_stat->st_mtime;
> > +   filetime[/* atime */ 0].tv_sec = openfile->current_stat->st_atime;
> > +   filetime[/* mtime */ 1].tv_sec = openfile->current_stat->st_mtime;
> 
> I don't think the comments are needed; it's obvious enough by
> what gets assigned to them.

I wanted to make it obvious that there is a reason why we assign the items
in the particular order.  The reason is the API of futimens(), which is not 
very intuitive, although documented.  I will drop the comments per your 
request.

> >     /* Copy the file. */
> > 
> > -   copy_status = copy_file(f, backup_file);
> > +   copy_status = copy_file(f, backup_file, /* close_out */ false);
> 
> Here the inline comment is confusing, because nano doesn't
> use them anywhere else.  Boolean values are always uppercase.

As you wish, will resubmit...

Kamil

> Benno