Silent reverting of my changes to libdebian-installer
In libdebian-installer 0.60, I made the following change (r55491):
* Appease the combination of _FORTIFY_SOURCE=2 (used by default on Ubuntu)
and -Werror. Why exactly glibc demands that fwrite be checked but not
fputs is beyond me.
In libdebian-installer 0.61, Bastian Blank made the following change
(r56169):
* Remove workarounds for ignored errors.
... which simply reverts my change. Because as far as I can tell nobody
had the basic courtesy to mail me about this, I didn't notice until
today.
Funnily enough, I didn't make that change to annoy people; I made it
because I needed it, and I committed it to upstream d-i because it was
harmless and there was no point in introducing divergence between Debian
and Ubuntu for something harmless to Debian. (People criticise me enough
for the cases that *are* currently necessary that I'd rather keep
divergence to a minimum, which I thought was a goal everyone could
happily agree with and certainly one I am currently working towards; in
this case the change seemed uncomplicated and it was my judgement that
it was unlikely to cause offence, so I didn't feel I needed to waste
people's time by mailing it to the list. Furthermore I'm in libd-i's
Uploaders and as far as I know nobody has objected to that!)
Bastian's claim that the error is ignored is wrong in one of the two
cases; my pipe change does not ignore the error.
I think my change should be reinstated because:
* Even though my fwrite change has no functional effect, it documents
that the error is known and ignored for the time being, which is
worthwhile and generally good practice. It happens to do so in such
a way as to appease -D_FORTIFY_SOURCE=2 -Werror, which I don't think
is harmful (note that this is not the thin end of the wedge; this
set of libd-i changes was the only set of changes I had to make to
all of d-i for this, although that may well be because -Werror is
not used universally). I can probably arrange to wrap it in a macro
for readability if you'd prefer that.
Note that this is a real and not insignificant bug; if you run out
of memory, di_parser_rfc822_write_file may silently truncate the
file it's writing. This is surely worth a FIXME comment or two even
if they aren't pretty.
* My pipe change in internal_di_exec has a real functional effect: it
causes internal_di_exec to fail if pipe fails, rather than merrily
carrying on as if nothing had happened. The code as reverted is
simply wrong even if the compiler flags in use by default in Debian
don't notice it. pipe can fail for slightly unlikely but quite
possible reasons (e.g. running out of per-process file descriptors),
and in the event that that happens then internal_di_exec shouldn't
just carry on.
Perhaps I should have written more about this in my changelog comment,
but I thought it would be obvious that compiler warnings (converted to
errors by -Werror) often indicate real bugs.
I'm quite happy to have a civilised discussion about these changes;
circumstances beyond my control force me to carry some kind of fix for
-D_FORTIFY_SOURCE=2 -Werror compilation in Ubuntu regardless, but that
doesn't mean that their form is fixed. I really object to this practice
of backing out changes without comment though.
The patch content of r55491 is attached to this mail.
Thanks,
--
Colin Watson [cjwatson@debian.org]
Index: debian/changelog
===================================================================
--- debian/changelog (revision 55490)
+++ debian/changelog (revision 55491)
@@ -2,6 +2,9 @@ libdebian-installer (0.60) UNRELEASED; urgency=low
* Remove incorrect unused attribute from di_packages_parser_read_name's
user_data parameter.
+ * Appease the combination of _FORTIFY_SOURCE=2 (used by default on Ubuntu)
+ and -Werror. Why exactly glibc demands that fwrite be checked but not
+ fputs is beyond me.
-- Colin Watson <cjwatson@debian.org> Mon, 01 Sep 2008 23:50:02 +0100
Index: src/parser_rfc822.c
===================================================================
--- src/parser_rfc822.c (revision 55490)
+++ src/parser_rfc822.c (revision 55491)
@@ -244,9 +244,15 @@ cleanup:
static void callback (const di_rstring *field, const di_rstring *value, void *data)
{
FILE *f = data;
- fwrite (field->string, field->size, 1, f);
+ if (fwrite (field->string, field->size, 1, f) < 1)
+ {
+ /* FIXME: can't handle write errors, but appease _FORTIFY_SOURCE=2 */
+ }
fputs (": ", f);
- fwrite (value->string, value->size, 1, f);
+ if (fwrite (value->string, value->size, 1, f) < 1)
+ {
+ /* FIXME: can't handle write errors, but appease _FORTIFY_SOURCE=2 */
+ }
fputs ("\n", f);
}
Index: src/exec.c
===================================================================
--- src/exec.c (revision 55490)
+++ src/exec.c (revision 55491)
@@ -60,7 +60,19 @@ static int internal_di_exec (const char *path, boo
}
for (i = 0; i < pipes; i++)
- pipe (&fds[i * 2]);
+ {
+ if (pipe (&fds[i * 2]) < 0)
+ {
+ int j;
+ di_log (DI_LOG_LEVEL_WARNING, "pipe failed");
+ for (j = 0; j < i; j++)
+ {
+ close (fds[i * 2]);
+ close (fds[i * 2 + 1]);
+ }
+ return -1;
+ }
+ }
pid = fork ();
Reply to: