[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Nano-devel] [PATCH v2] pull in the futimens module from gnulib
From: |
Kamil Dudka |
Subject: |
Re: [Nano-devel] [PATCH v2] pull in the futimens module from gnulib |
Date: |
Tue, 04 Apr 2017 09:25:57 +0200 |
User-agent: |
KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.29; x86_64; ; ) |
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