[3051] in BarnOwl Developers
Re: [PATCH] owl_util_file_deleteline should not error on non-existent file
daemon@ATHENA.MIT.EDU (Jason Gross)
Tue May 17 13:10:47 2011
In-Reply-To: <alpine.DEB.1.10.1105171218380.30967@dr-wily.mit.edu>
From: Jason Gross <jgross@MIT.EDU>
Date: Tue, 17 May 2011 13:10:14 -0400
To: David A Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
--000e0cd6ed1c3c768804a37bdab7
Content-Type: text/plain; charset=ISO-8859-1
How's this look?
owl_function_addstartup checks for file before calling
owl_util_file_deleteline
This fixes trac #175: '"startup" command produces error when run for the
first time'. The startup command deletes duplicate lines, and thus
errored when no file existed. A check has been added to prevent this.
---
functions.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/functions.c b/functions.c
index dddf791..5ab3c43 100644
--- a/functions.c
+++ b/functions.c
@@ -3253,8 +3253,9 @@ void owl_function_addstartup(const char *buff)
filename=owl_global_get_startupfile(&g);
- /* delete earlier copies */
- owl_util_file_deleteline(filename, buff, 1);
+ /* delete earlier copies, if the file exists */
+ if (g_file_test(filename, G_FILE_TEST_EXISTS))
+ owl_util_file_deleteline(filename, buff, 1);
file=fopen(filename, "a");
if (!file) {
On Tue, May 17, 2011 at 12:21 PM, David A Benjamin <davidben@mit.edu> wrote:
> Can you move the error message to the callers, perhaps?
>
> (We really need a more coherant way for passing errors up. Glib has this
> strange GError thing which... works but is probably a bit much.)
>
> I think it's also fine to not care about the race. It's pretty harmless.
>
>
> David
>
> On Tue, 17 May 2011, Jason Gross wrote:
>
> We don't want to display an error at all on the startup file not existing
>> when we call startup, so setting errno carefully won't work.
>>
>> If we worry about race conditions (which I'm not sure we should), then I
>> don't see how you can check before owl_util_file_deleteline. Should we
>> worry about race conditions? Alternatively, I could
>> make owl_util_file_deleteline take an optional boolean error_on_enoent.
>>
>> -Jason
>>
>> On Tue, May 17, 2011 at 12:04 PM, David A Benjamin <davidben@mit.edu>
>> wrote:
>> I think this is the wrong place to add the check. If I ask you to
>> delete a line from a file, and you find that the file doesn't exist, that is
>> definitely grounds for a failure.
>>
>> Better would be to either make owl_util_file_deleteline report what
>> kind of error (errno should work here, but you have to be careful to re-set
>> it because owl_function_error may set errno) so
>> the caller can interpret it correctly or to have startup check before
>> calling owl_util_file_deleteline.
>>
>> David
>>
>>
>> On Tue, 17 May 2011, Jason Gross wrote:
>>
>> This fixes trac #175: '"startup" command produces error when run for
>> the
>> first time'. The startup command deletes duplicate lines, and thus
>> errored when no file existed. The purpose of
>> owl_util_file_deleteline
>> is to make sure that the given line is no longer in the file, so
>> there
>> seems to be no harm in returning 0 (meaning no lines were deleted) on
>> the file not existing.
>> ---
>> util.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/util.c b/util.c
>> index 82f36ef..44899f0 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -423,6 +423,7 @@ gchar *owl_util_recursive_resolve_link(const char
>> *filename)
>> * case-insensitive.
>> *
>> * Returns the number of lines removed on success. Returns -1 on
>> failure.
>> + * Returns 0 if the file does not exist; the line wasn't present.
>> */
>> int owl_util_file_deleteline(const char *filename, const char *line,
>> int backup)
>> {
>> @@ -433,6 +434,7 @@ int owl_util_file_deleteline(const char
>> *filename, const char *line, int backup)
>> int numremoved = 0;
>>
>> if ((old = fopen(filename, "r")) == NULL) {
>> + if (errno == ENOENT) return 0;
>> owl_function_error("Cannot open %s (for reading): %s",
>> filename, strerror(errno));
>> return -1;
>> --
>> 1.7.2.3
>>
>>
>>
>>
>>
--000e0cd6ed1c3c768804a37bdab7
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
How's this look?<div><div><br></div><div>owl_function_addstartup checks=
for file before calling owl_util_file_deleteline</div><div><br></div><div>=
This fixes trac #175: '"startup" command produces error when =
run for the</div>
<div>first time'. =A0The startup command deletes duplicate lines, and t=
hus</div><div>errored when no file existed. =A0A check has been added to pr=
event this.</div><div>---</div><div>=A0functions.c | =A0 =A05 +++--</div><d=
iv>=A01 files changed, 3 insertions(+), 2 deletions(-)</div>
<div><br></div><div>diff --git a/functions.c b/functions.c</div><div>index =
dddf791..5ab3c43 100644</div><div>--- a/functions.c</div><div>+++ b/functio=
ns.c</div><div>@@ -3253,8 +3253,9 @@ void owl_function_addstartup(const cha=
r *buff)</div>
<div><br></div><div>=A0 filename=3Dowl_global_get_startupfile(&g);</div=
><div><br></div><div>- =A0/* delete earlier copies */</div><div>- =A0owl_ut=
il_file_deleteline(filename, buff, 1);</div><div>+ =A0/* delete earlier cop=
ies, if the file exists */</div>
<div>+ =A0if (g_file_test(filename, G_FILE_TEST_EXISTS))</div><div>+ =A0 =
=A0owl_util_file_deleteline(filename, buff, 1);</div><div><br></div><div>=
=A0 file=3Dfopen(filename, "a");</div><div>=A0 if (!file) {</div>=
<div><br></div>
</div><div><br><div><br><div class=3D"gmail_quote">On Tue, May 17, 2011 at =
12:21 PM, David A Benjamin <span dir=3D"ltr"><<a href=3D"mailto:davidben=
@mit.edu" target=3D"_blank">davidben@mit.edu</a>></span> wrote:<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">Can you move the error message to the caller=
s, perhaps?<br>
<br>
(We really need a more coherant way for passing errors up. Glib has this st=
range GError thing which... works but is probably a bit much.)<br>
<br>
I think it's also fine to not care about the race. It's pretty harm=
less.<div><div></div><div><br>
<br>
David<br>
<br>
On Tue, 17 May 2011, Jason Gross wrote:<br>
<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">
We don't want to display an error at all on the startup file not existi=
ng when we call startup, so setting errno carefully won't work.<br>
<br>
If we worry about race conditions (which I'm not sure we should), then =
I don't see how you can check before owl_util_file_deleteline. =A0Shoul=
d we worry about race conditions? Alternatively, I could<br>
make=A0owl_util_file_deleteline take an optional boolean error_on_enoent.<b=
r>
<br>
-Jason<br>
<br>
On Tue, May 17, 2011 at 12:04 PM, David A Benjamin <<a href=3D"mailto:da=
vidben@mit.edu" target=3D"_blank">davidben@mit.edu</a>> wrote:<br>
=A0 =A0 =A0I think this is the wrong place to add the check. If I ask you =
to delete a line from a file, and you find that the file doesn't exist,=
that is definitely grounds for a failure.<br>
<br>
=A0 =A0 =A0Better would be to either make owl_util_file_deleteline report =
what kind of error (errno should work here, but you have to be careful to r=
e-set it because owl_function_error may set errno) so<br>
=A0 =A0 =A0the caller can interpret it correctly or to have startup check =
before calling owl_util_file_deleteline.<br>
<br>
=A0 =A0 =A0David<br>
<br>
<br>
On Tue, 17 May 2011, Jason Gross wrote:<br>
<br>
=A0 =A0 =A0This fixes trac #175: '"startup" command produces=
error when run for the<br>
=A0 =A0 =A0first time'. =A0The startup command deletes duplicate lines=
, and thus<br>
=A0 =A0 =A0errored when no file existed. =A0The purpose of owl_util_file_d=
eleteline<br>
=A0 =A0 =A0is to make sure that the given line is no longer in the file, s=
o there<br>
=A0 =A0 =A0seems to be no harm in returning 0 (meaning no lines were delet=
ed) on<br>
=A0 =A0 =A0the file not existing.<br>
=A0 =A0 =A0---<br>
=A0 =A0 =A0util.c | =A0 =A02 ++<br>
=A0 =A0 =A01 files changed, 2 insertions(+), 0 deletions(-)<br>
<br>
=A0 =A0 =A0diff --git a/util.c b/util.c<br>
=A0 =A0 =A0index 82f36ef..44899f0 100644<br>
=A0 =A0 =A0--- a/util.c<br>
=A0 =A0 =A0+++ b/util.c<br>
=A0 =A0 =A0@@ -423,6 +423,7 @@ gchar *owl_util_recursive_resolve_link(cons=
t char *filename)<br>
=A0 =A0 =A0=A0* case-insensitive.<br>
=A0 =A0 =A0=A0*<br>
=A0 =A0 =A0=A0* Returns the number of lines removed on success. =A0Returns=
-1 on failure.<br>
=A0 =A0 =A0+ * Returns 0 if the file does not exist; the line wasn't p=
resent.<br>
=A0 =A0 =A0=A0*/<br>
=A0 =A0 =A0int owl_util_file_deleteline(const char *filename, const char *=
line, int backup)<br>
=A0 =A0 =A0{<br>
=A0 =A0 =A0@@ -433,6 +434,7 @@ int owl_util_file_deleteline(const char *fi=
lename, const char *line, int backup)<br>
=A0 =A0 =A0=A0int numremoved =3D 0;<br>
<br>
=A0 =A0 =A0=A0if ((old =3D fopen(filename, "r")) =3D=3D NULL) {<=
br>
=A0 =A0 =A0+ =A0 =A0if (errno =3D=3D ENOENT) return 0;<br>
=A0 =A0 =A0=A0 =A0owl_function_error("Cannot open %s (for reading): %=
s",<br>
=A0 =A0 =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 filename, strerror(=
errno));<br>
=A0 =A0 =A0=A0 =A0return -1;<br>
=A0 =A0 =A0--<br>
=A0 =A0 =A01.7.2.3<br>
<br>
<br>
<br>
<br>
</blockquote>
</div></div></blockquote></div><br></div></div>
--000e0cd6ed1c3c768804a37bdab7--
