Re: rpki-client: check fflush on output files
Jeremie Courreges-Anglas wrote:
Lovely!
> Agreed. Here's an updated diff that tests the return value of
> output_finish(). Suggestions welcome for the error message in this
> case.
>
> It also drops the extra "out = NULL;", and replaces logx() calls with
> warn(3) in this file. I see no reason to drop those messages and errno
> information. logx() seems used mostly for statistics.
>
> Thoughts, tests / oks?
>
>
> Index: extern.h
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 extern.h
> --- extern.h 6 Mar 2020 17:36:42 - 1.24
> +++ extern.h 9 Mar 2020 08:19:20 -
> @@ -372,7 +372,7 @@ extern char* outputdir;
> int outputfiles(struct vrp_tree *v);
> FILE *output_createtmp(char *);
> void output_cleantmp(void);
> -void output_finish(FILE *);
> +int output_finish(FILE *);
> int output_bgpd(FILE *, struct vrp_tree *);
> int output_bird1v4(FILE *, struct vrp_tree *);
> int output_bird1v6(FILE *, struct vrp_tree *);
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 9 Mar 2020 08:20:39 -
> @@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
>
> fout = output_createtmp(outputs[i].name);
> if (fout == NULL) {
> - logx("cannot create %s", outputs[i].name);
> + warn("cannot create %s", outputs[i].name);
> rc = 1;
> continue;
> }
> if ((*outputs[i].fn)(fout, v) != 0) {
> - logx("output for %s format failed", outputs[i].name);
> + warn("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> rc = 1;
> continue;
> }
> - output_finish(fout);
> + if (output_finish(fout) != 0) {
> + warn("finish for %s format failed", outputs[i].name);
> + output_cleantmp();
> + rc = 1;
> + continue;
> + }
> }
>
> return rc;
> @@ -108,14 +113,15 @@ output_createtmp(char *name)
> return f;
> }
>
> -void
> +int
> output_finish(FILE *out)
> {
> - fclose(out);
> - out = NULL;
> -
> - rename(output_tmpname, output_name);
> + if (fclose(out) != 0)
> + return -1;
> + if (rename(output_tmpname, output_name) == -1)
> + return -1;
> output_tmpname[0] = '\0';
> + return 0;
> }
>
> void
Re: rpki-client: check fflush on output files
On Mon, Mar 09, 2020 at 09:41:37AM +0100, Jeremie Courreges-Anglas wrote:
> On Sat, Mar 07 2020, Claudio Jeker wrote:
> > On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
> >> On Fri, Mar 06 2020, "Theo de Raadt" wrote:
> >> >> Jeremie Courreges-Anglas wrote:
> >> >> >
> >> >> >
> >> >> > Checking the return value of fprintf is good but not enough to ensure
> >> >> > that data has properly been written to the file without an error. To
> >> >> > do
> >> >> > that we can use fflush(3) in a single place.
> >> [redacted]
> >> >> How about checking the return value of fclose() in output_finish()
> >> >> instead?
> >> > Oh you want to reach the error-reporting code...
> >>
> >> And the cleanup-on-error code. Doing it here looks more appealing than
> >> adding
> >>if (fflush(out) != 0)
> >>return -1;
> >>
> >> at the end of all the output_* functions.
> >>
> >> If you're careful about write errors you can avoid feeding an
> >> incomplete/garbage file to your BGP daemon. The code was already
> >> careful, but did not account for buffering. This is what this patch
> >> tries to address.
> >>
> >> >> > Build-tested only. ok?
> >> >> >
> >> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
> >> >> > I suspect it's a remnant from a time where the output FILE * was
> >> >> > a global. That would be a separate commit.
> >>
> >> Diff below again for convenience,
> >>
> >>
> >> Index: output.c
> >> ===
> >> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> >> retrieving revision 1.6
> >> diff -u -p -p -u -r1.6 output.c
> >> --- output.c 6 Mar 2020 17:36:42 - 1.6
> >> +++ output.c 6 Mar 2020 23:04:18 -
> >> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
> >>rc = 1;
> >>continue;
> >>}
> >> - if ((*outputs[i].fn)(fout, v) != 0) {
> >> + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
> >>logx("output for %s format failed", outputs[i].name);
> >>fclose(fout);
> >>output_cleantmp();
> >> @@ -112,8 +112,6 @@ void
> >> output_finish(FILE *out)
> >> {
> >>fclose(out);
> >> - out = NULL;
> >> -
> >>rename(output_tmpname, output_name);
> >>output_tmpname[0] = '\0';
> >> }
> >>
> >
> > I think it would be better to pick up the fclose error in output_finish()
> > and while doing that also check for rename() errors. At least those errors
> > should be logged.
>
> Agreed. Here's an updated diff that tests the return value of
> output_finish(). Suggestions welcome for the error message in this
> case.
>
> It also drops the extra "out = NULL;", and replaces logx() calls with
> warn(3) in this file. I see no reason to drop those messages and errno
> information. logx() seems used mostly for statistics.
>
> Thoughts, tests / oks?
Works for me. Ok claudio@
PS: I think some of the output_* prototypes could be moved to output.c
since those functions are only used internally.
> Index: extern.h
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 extern.h
> --- extern.h 6 Mar 2020 17:36:42 - 1.24
> +++ extern.h 9 Mar 2020 08:19:20 -
> @@ -372,7 +372,7 @@ extern char* outputdir;
> int outputfiles(struct vrp_tree *v);
> FILE *output_createtmp(char *);
> void output_cleantmp(void);
> -void output_finish(FILE *);
> +int output_finish(FILE *);
> int output_bgpd(FILE *, struct vrp_tree *);
> int output_bird1v4(FILE *, struct vrp_tree *);
> int output_bird1v6(FILE *, struct vrp_tree *);
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 9 Mar 2020 08:20:39 -
> @@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
>
> fout = output_createtmp(outputs[i].name);
> if (fout == NULL) {
> - logx("cannot create %s", outputs[i].name);
> + warn("cannot create %s", outputs[i].name);
> rc = 1;
> continue;
> }
> if ((*outputs[i].fn)(fout, v) != 0) {
> - logx("output for %s format failed", outputs[i].name);
> + warn("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> rc = 1;
> continue;
> }
> - output_finish(fout);
> + if (output_finish(fout) != 0) {
>
Re: rpki-client: check fflush on output files
On Sat, Mar 07 2020, Claudio Jeker wrote:
> On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
>> On Fri, Mar 06 2020, "Theo de Raadt" wrote:
>> >> Jeremie Courreges-Anglas wrote:
>> >> >
>> >> >
>> >> > Checking the return value of fprintf is good but not enough to ensure
>> >> > that data has properly been written to the file without an error. To do
>> >> > that we can use fflush(3) in a single place.
>> [redacted]
>> >> How about checking the return value of fclose() in output_finish()
>> >> instead?
>> > Oh you want to reach the error-reporting code...
>>
>> And the cleanup-on-error code. Doing it here looks more appealing than
>> adding
>> if (fflush(out) != 0)
>> return -1;
>>
>> at the end of all the output_* functions.
>>
>> If you're careful about write errors you can avoid feeding an
>> incomplete/garbage file to your BGP daemon. The code was already
>> careful, but did not account for buffering. This is what this patch
>> tries to address.
>>
>> >> > Build-tested only. ok?
>> >> >
>> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
>> >> > I suspect it's a remnant from a time where the output FILE * was
>> >> > a global. That would be a separate commit.
>>
>> Diff below again for convenience,
>>
>>
>> Index: output.c
>> ===
>> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
>> retrieving revision 1.6
>> diff -u -p -p -u -r1.6 output.c
>> --- output.c 6 Mar 2020 17:36:42 - 1.6
>> +++ output.c 6 Mar 2020 23:04:18 -
>> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
>> rc = 1;
>> continue;
>> }
>> -if ((*outputs[i].fn)(fout, v) != 0) {
>> +if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
>> logx("output for %s format failed", outputs[i].name);
>> fclose(fout);
>> output_cleantmp();
>> @@ -112,8 +112,6 @@ void
>> output_finish(FILE *out)
>> {
>> fclose(out);
>> -out = NULL;
>> -
>> rename(output_tmpname, output_name);
>> output_tmpname[0] = '\0';
>> }
>>
>
> I think it would be better to pick up the fclose error in output_finish()
> and while doing that also check for rename() errors. At least those errors
> should be logged.
Agreed. Here's an updated diff that tests the return value of
output_finish(). Suggestions welcome for the error message in this
case.
It also drops the extra "out = NULL;", and replaces logx() calls with
warn(3) in this file. I see no reason to drop those messages and errno
information. logx() seems used mostly for statistics.
Thoughts, tests / oks?
Index: extern.h
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.24
diff -u -p -r1.24 extern.h
--- extern.h6 Mar 2020 17:36:42 - 1.24
+++ extern.h9 Mar 2020 08:19:20 -
@@ -372,7 +372,7 @@ extern char* outputdir;
int outputfiles(struct vrp_tree *v);
FILE *output_createtmp(char *);
voidoutput_cleantmp(void);
-voidoutput_finish(FILE *);
+int output_finish(FILE *);
int output_bgpd(FILE *, struct vrp_tree *);
int output_bird1v4(FILE *, struct vrp_tree *);
int output_bird1v6(FILE *, struct vrp_tree *);
Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 - 1.6
+++ output.c9 Mar 2020 08:20:39 -
@@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
fout = output_createtmp(outputs[i].name);
if (fout == NULL) {
- logx("cannot create %s", outputs[i].name);
+ warn("cannot create %s", outputs[i].name);
rc = 1;
continue;
}
if ((*outputs[i].fn)(fout, v) != 0) {
- logx("output for %s format failed", outputs[i].name);
+ warn("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
rc = 1;
continue;
}
- output_finish(fout);
+ if (output_finish(fout) != 0) {
+ warn("finish for %s format failed", outputs[i].name);
+ output_cleantmp();
+ rc = 1;
+ continue;
+ }
}
return rc;
@@ -108,14 +113,15 @@ output_createtmp(char *name)
return f;
}
-void
+int
output_finish(FILE *out)
{
- fclose(out);
- out = NULL;
-
Re: rpki-client: check fflush on output files
On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
> On Fri, Mar 06 2020, "Theo de Raadt" wrote:
> >> Jeremie Courreges-Anglas wrote:
> >> >
> >> >
> >> > Checking the return value of fprintf is good but not enough to ensure
> >> > that data has properly been written to the file without an error. To do
> >> > that we can use fflush(3) in a single place.
> [redacted]
> >> How about checking the return value of fclose() in output_finish() instead?
> > Oh you want to reach the error-reporting code...
>
> And the cleanup-on-error code. Doing it here looks more appealing than
> adding
> if (fflush(out) != 0)
> return -1;
>
> at the end of all the output_* functions.
>
> If you're careful about write errors you can avoid feeding an
> incomplete/garbage file to your BGP daemon. The code was already
> careful, but did not account for buffering. This is what this patch
> tries to address.
>
> >> > Build-tested only. ok?
> >> >
> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
> >> > I suspect it's a remnant from a time where the output FILE * was
> >> > a global. That would be a separate commit.
>
> Diff below again for convenience,
>
>
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -p -u -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 6 Mar 2020 23:04:18 -
> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
> rc = 1;
> continue;
> }
> - if ((*outputs[i].fn)(fout, v) != 0) {
> + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
> logx("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> @@ -112,8 +112,6 @@ void
> output_finish(FILE *out)
> {
> fclose(out);
> - out = NULL;
> -
> rename(output_tmpname, output_name);
> output_tmpname[0] = '\0';
> }
>
I think it would be better to pick up the fclose error in output_finish()
and while doing that also check for rename() errors. At least those errors
should be logged.
--
:wq Claudio
Re: rpki-client: check fflush on output files
On Fri, Mar 06 2020, "Theo de Raadt" wrote:
>> Jeremie Courreges-Anglas wrote:
>> >
>> >
>> > Checking the return value of fprintf is good but not enough to ensure
>> > that data has properly been written to the file without an error. To do
>> > that we can use fflush(3) in a single place.
[redacted]
>> How about checking the return value of fclose() in output_finish() instead?
> Oh you want to reach the error-reporting code...
And the cleanup-on-error code. Doing it here looks more appealing than
adding
if (fflush(out) != 0)
return -1;
at the end of all the output_* functions.
If you're careful about write errors you can avoid feeding an
incomplete/garbage file to your BGP daemon. The code was already
careful, but did not account for buffering. This is what this patch
tries to address.
>> > Build-tested only. ok?
>> >
>> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
>> > I suspect it's a remnant from a time where the output FILE * was
>> > a global. That would be a separate commit.
Diff below again for convenience,
Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -p -u -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 - 1.6
+++ output.c6 Mar 2020 23:04:18 -
@@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
rc = 1;
continue;
}
- if ((*outputs[i].fn)(fout, v) != 0) {
+ if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
logx("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
@@ -112,8 +112,6 @@ void
output_finish(FILE *out)
{
fclose(out);
- out = NULL;
-
rename(output_tmpname, output_name);
output_tmpname[0] = '\0';
}
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: rpki-client: check fflush on output files
Oh you want to reach the error-reporting code...
> How about checking the return value of fclose() in output_finish() instead?
>
> Jeremie Courreges-Anglas wrote:
> >
> >
> > Checking the return value of fprintf is good but not enough to ensure
> > that data has properly been written to the file without an error. To do
> > that we can use fflush(3) in a single place.
> >
> > Build-tested only. ok?
> >
> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
> > I suspect it's a remnant from a time where the output FILE * was
> > a global. That would be a separate commit.
> >
> >
> > Index: output.c
> > ===
> > RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> > retrieving revision 1.6
> > diff -u -p -p -u -r1.6 output.c
> > --- output.c6 Mar 2020 17:36:42 - 1.6
> > +++ output.c6 Mar 2020 19:09:13 -
> > @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
> > rc = 1;
> > continue;
> > }
> > - if ((*outputs[i].fn)(fout, v) != 0) {
> > + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
> > logx("output for %s format failed", outputs[i].name);
> > fclose(fout);
> > output_cleantmp();
> > @@ -112,8 +112,6 @@ void
> > output_finish(FILE *out)
> > {
> > fclose(out);
> > - out = NULL;
> > -
> > rename(output_tmpname, output_name);
> > output_tmpname[0] = '\0';
> > }
> >
> >
> >
> > --
> > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
> >
>
Re: rpki-client: check fflush on output files
How about checking the return value of fclose() in output_finish() instead?
Jeremie Courreges-Anglas wrote:
>
>
> Checking the return value of fprintf is good but not enough to ensure
> that data has properly been written to the file without an error. To do
> that we can use fflush(3) in a single place.
>
> Build-tested only. ok?
>
> Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
> I suspect it's a remnant from a time where the output FILE * was
> a global. That would be a separate commit.
>
>
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -p -u -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 6 Mar 2020 19:09:13 -
> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
> rc = 1;
> continue;
> }
> - if ((*outputs[i].fn)(fout, v) != 0) {
> + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
> logx("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> @@ -112,8 +112,6 @@ void
> output_finish(FILE *out)
> {
> fclose(out);
> - out = NULL;
> -
> rename(output_tmpname, output_name);
> output_tmpname[0] = '\0';
> }
>
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
rpki-client: check fflush on output files
Checking the return value of fprintf is good but not enough to ensure
that data has properly been written to the file without an error. To do
that we can use fflush(3) in a single place.
Build-tested only. ok?
Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
I suspect it's a remnant from a time where the output FILE * was
a global. That would be a separate commit.
Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -p -u -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 - 1.6
+++ output.c6 Mar 2020 19:09:13 -
@@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
rc = 1;
continue;
}
- if ((*outputs[i].fn)(fout, v) != 0) {
+ if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
logx("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
@@ -112,8 +112,6 @@ void
output_finish(FILE *out)
{
fclose(out);
- out = NULL;
-
rename(output_tmpname, output_name);
output_tmpname[0] = '\0';
}
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
