| Message ID | cfc8cde1-bdfa-5e08-adc5-746fc4acd57d@charter.net |
|---|---|
| State | New |
| Headers | show |
| Series | [libgfortran,committed] PR89020 | expand |
On Sat, Jan 26, 2019 at 10:42 PM Jerry DeLisle <jvdelisle@charter.net> wrote: > Committed as simple and obvious. (With a ChangeLog Bobble fixed) > > Regression tested on x86_64. > > Committed r268301 > M libgfortran/ChangeLog > M libgfortran/io/close.c > > Regards, > > Jerry > > 2019-01-26 Jerry DeLisle <jvdelisle@gcc.gnu.org> > > PR libfortran/89020 > * io/close.c (st_close): Generate error if calls to 'remove' > return an error. > > > diff --git a/libgfortran/io/close.c b/libgfortran/io/close.c > index cbcbf4e71a1..2b35e49c9cc 100644 > --- a/libgfortran/io/close.c > +++ b/libgfortran/io/close.c > @@ -99,7 +99,11 @@ st_close (st_parameter_close *clp) > else > { > #if HAVE_UNLINK_OPEN_FILE > - remove (u->filename); > + > + if (remove (u->filename)) > + generate_error (&clp->common, LIBERROR_OS, > + "File cannot be deleted, possibly in use by" > + " another process"); > #else > path = strdup (u->filename); > #endif > @@ -112,7 +116,10 @@ st_close (st_parameter_close *clp) > #if !HAVE_UNLINK_OPEN_FILE > if (path != NULL) > { > - remove (path); > + if (remove (u->filename)) > + generate_error (&clp->common, LIBERROR_OS, > + "File cannot be deleted, possibly in use by" > + " another process"); > free (path); > } > #endif > Checking remove() for an error is a good idea, although speculating why that happened might be confusing if the error happens for some other reason? Particularly so on POSIX systems, where deleting open files is explicitly part of the design. In generate_error(), we already call gf_strerror() to translate errno to a string, which should give a more accurate description of what went wrong? So maybe just say "File cannot be deleted"? And yes, as mentioned in the PR, the second remove should "remove (path)" and not "u->filename".
On 1/27/19 1:13 AM, Janne Blomqvist wrote: > On Sat, Jan 26, 2019 at 10:42 PM Jerry DeLisle <jvdelisle@charter.net --- snip --- > > > Checking remove() for an error is a good idea, although speculating why > that happened might be confusing if the error happens for some other > reason? Particularly so on POSIX systems, where deleting open files is > explicitly part of the design. In generate_error(), we already call > gf_strerror() to translate errno to a string, which should give a more > accurate description of what went wrong? So maybe just say "File cannot > be deleted"? > > And yes, as mentioned in the PR, the second remove should "remove > (path)" and not "u->filename". > > -- > Janne Blomqvist Hi Janne, Yes, I fixed the path mistake last nite. I almost changed that error message for the reasons you suggest. I will go ahead and do this later today. Thanks for your suggestions. Jerry
diff --git a/libgfortran/io/close.c b/libgfortran/io/close.c index cbcbf4e71a1..2b35e49c9cc 100644 --- a/libgfortran/io/close.c +++ b/libgfortran/io/close.c @@ -99,7 +99,11 @@ st_close (st_parameter_close *clp) else { #if HAVE_UNLINK_OPEN_FILE - remove (u->filename); + + if (remove (u->filename)) + generate_error (&clp->common, LIBERROR_OS, + "File cannot be deleted, possibly in use by" + " another process"); #else path = strdup (u->filename); #endif @@ -112,7 +116,10 @@ st_close (st_parameter_close *clp) #if !HAVE_UNLINK_OPEN_FILE if (path != NULL) { - remove (path); + if (remove (u->filename)) + generate_error (&clp->common, LIBERROR_OS, + "File cannot be deleted, possibly in use by" + " another process"); free (path); } #endif