[libgfortran,committed] PR89020

Message ID cfc8cde1-bdfa-5e08-adc5-746fc4acd57d@charter.net
State New
Headers show
Series
  • [libgfortran,committed] PR89020
Related show

Commit Message

Jerry Jan. 26, 2019, 8:42 p.m.
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.

Comments

Janne Blomqvist Jan. 27, 2019, 9:13 a.m. | #1
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".

-- 
Janne Blomqvist
Jerry Jan. 27, 2019, 4:54 p.m. | #2
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

Patch

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