Message ID | 1440571321-20287-1-git-send-email-eggert@cs.ucla.edu |
---|---|
State | New |
Headers | show |
On 08/26/2015 08:42 AM, Paul Eggert wrote: > * sysdeps/posix/posix_fallocate.c (posix_fallocate): > * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > Fix parenthesization typo. > --- > ChangeLog | 5 +++++ > sysdeps/posix/posix_fallocate.c | 2 +- > sysdeps/posix/posix_fallocate64.c | 2 +- > 3 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index bd6d027..458b850 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,5 +1,10 @@ > 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> > > + Fix broken overflow check in posix_fallocate > + * sysdeps/posix/posix_fallocate.c (posix_fallocate): > + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > + Fix parenthesization typo. Please add the BZ #18873 reference before committing (also to NEWS). I don't recognize this changelog format. Not sure if that's matching the standards. Otherwise okay, thanks.
On 08/26/2015 02:47 AM, Florian Weimer wrote: > On 08/26/2015 08:42 AM, Paul Eggert wrote: >> * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> Fix parenthesization typo. >> --- >> ChangeLog | 5 +++++ >> sysdeps/posix/posix_fallocate.c | 2 +- >> sysdeps/posix/posix_fallocate64.c | 2 +- >> 3 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/ChangeLog b/ChangeLog >> index bd6d027..458b850 100644 >> --- a/ChangeLog >> +++ b/ChangeLog >> @@ -1,5 +1,10 @@ >> 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> >> >> + Fix broken overflow check in posix_fallocate >> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> + Fix parenthesization typo. > > Please add the BZ #18873 reference before committing (also to NEWS). > > I don't recognize this changelog format. Not sure if that's matching > the standards. In some GNU projects they encourage a general paragraph before the change list to explain the fix. The GNU Coding Standard is silent about this kind of additional text. We allow it, but I think Paul is the only one who does this :-) c.
Carlos O'Donell wrote: > I think Paul is the only one > who does this It's standard in many other GNU projects (e.g., gnulib, coreutils), and fits well with the common Git style for commit messages. GCC and GNU Emacs developers often use this style too, now, though not always. It's a reasonable style and glibc should allow patches that use it, if only so that it's one less thing for occasional contributors to worry about.
On 08/28/2015 03:27 AM, Carlos O'Donell wrote: > On 08/26/2015 02:47 AM, Florian Weimer wrote: >> On 08/26/2015 08:42 AM, Paul Eggert wrote: >>> * sysdeps/posix/posix_fallocate.c (posix_fallocate): >>> * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >>> Fix parenthesization typo. >>> --- >>> ChangeLog | 5 +++++ >>> sysdeps/posix/posix_fallocate.c | 2 +- >>> sysdeps/posix/posix_fallocate64.c | 2 +- >>> 3 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/ChangeLog b/ChangeLog >>> index bd6d027..458b850 100644 >>> --- a/ChangeLog >>> +++ b/ChangeLog >>> @@ -1,5 +1,10 @@ >>> 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> >>> >>> + Fix broken overflow check in posix_fallocate >>> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >>> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >>> + Fix parenthesization typo. >> >> Please add the BZ #18873 reference before committing (also to NEWS). >> >> I don't recognize this changelog format. Not sure if that's matching >> the standards. > > In some GNU projects they encourage a general paragraph before the change > list to explain the fix. The GNU Coding Standard is silent about this > kind of additional text. We allow it, but I think Paul is the only one > who does this :-) That part I had seen before, but I also expected something like this: + Fix broken overflow check in posix_fallocate + * sysdeps/posix/posix_fallocate.c (posix_fallocate): + Fix parenthesization typo. + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): + Likewise.
On 08/28/2015 04:46 AM, Florian Weimer wrote: > On 08/28/2015 03:27 AM, Carlos O'Donell wrote: >> On 08/26/2015 02:47 AM, Florian Weimer wrote: >>> On 08/26/2015 08:42 AM, Paul Eggert wrote: >>>> * sysdeps/posix/posix_fallocate.c (posix_fallocate): >>>> * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >>>> Fix parenthesization typo. >>>> --- >>>> ChangeLog | 5 +++++ >>>> sysdeps/posix/posix_fallocate.c | 2 +- >>>> sysdeps/posix/posix_fallocate64.c | 2 +- >>>> 3 files changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/ChangeLog b/ChangeLog >>>> index bd6d027..458b850 100644 >>>> --- a/ChangeLog >>>> +++ b/ChangeLog >>>> @@ -1,5 +1,10 @@ >>>> 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> >>>> >>>> + Fix broken overflow check in posix_fallocate >>>> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >>>> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >>>> + Fix parenthesization typo. >>> >>> Please add the BZ #18873 reference before committing (also to NEWS). >>> >>> I don't recognize this changelog format. Not sure if that's matching >>> the standards. >> >> In some GNU projects they encourage a general paragraph before the change >> list to explain the fix. The GNU Coding Standard is silent about this >> kind of additional text. We allow it, but I think Paul is the only one >> who does this :-) > > That part I had seen before, but I also expected something like this: > > + Fix broken overflow check in posix_fallocate > + * sysdeps/posix/posix_fallocate.c (posix_fallocate): > + Fix parenthesization typo. > + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > + Likewise. Oh, I missed that. Agreed. Cheers, Carlos.
Florian Weimer wrote: > That part I had seen before, but I also expected something like this: > > + Fix broken overflow check in posix_fallocate > + * sysdeps/posix/posix_fallocate.c (posix_fallocate): > + Fix parenthesization typo. > + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > + Likewise. Nowadays the "Likewise" style is considered old-fashioned in the other projects I hang out in. Although it's fine and is still used on occasion, it feels a bit like writing "whereas" or "henceforth". I don't know whether the newer style has been formally documented anywhere, but it's quite common and it is terser.
Two things about the recently-proposed bug fixes prompted by the Coverity scan. First, Coverity itself does the scan for me. They use the glibc source to debug their scanner, and they send me their reports as a service to the community. I don't control the parameters they use. I found the latest set of four fixes by inspecting only the changes in reports from previous glibc. Most of the changes were false alarms (this is typical) and I generally left the code alone for the false alarms. Second, there's a lot of bureaucracy involved in getting obvious bugs like these fixed. The current distracting thread about ChangeLog style is a symptom of the problem. In the old days I would have simply installed the fixes and moved on, but now I'm being asked for testcases and bugzilla references and ChangeLog style reformattting and whatnot and I don't want to upset anybody's applecart nor do I want to bother with all that paperwork (hey, I'm just a volunteer!) so I've done nothing to follow up, which is not good. I can't help contrasting the somewhat offputting responses to my four glibc Coverity scan patches with the quite-friendly response I got from the gettext project when I filed the gettext-related patch upstream. Here's the entire interaction: I ran "git send-email --to=bug-gettext@gnu.org". Within a few hours, Daiki Ueno replied "Thanks, applied." It's a pleasure helping to fix gettext bugs. I wish I could say the same thing about glibc. I realize glibc is a bigger project and needs more bureaucracy yadda yadda yadda, but come on guys, we've gone off the deep end. Anyway, to work through this little problem could you please install the patches for BZ#18868, BZ#18871, BZ#18872, and BZ#18873, whenever you have the time? The fixes themselves all have reasonable consensus, I think. Please feel free to dot all the Is and cross all the Ts that I missed. Thanks.
On 08/28/2015 11:19 AM, Paul Eggert wrote: > Florian Weimer wrote: >> That part I had seen before, but I also expected something like this: >> >> + Fix broken overflow check in posix_fallocate >> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> + Fix parenthesization typo. >> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> + Likewise. > > Nowadays the "Likewise" style is considered old-fashioned in the > other projects I hang out in. Although it's fine and is still used on > occasion, it feels a bit like writing "whereas" or "henceforth". I > don't know whether the newer style has been formally documented > anywhere, but it's quite common and it is terser. If you wouldn't mind, please stick to the existing project format for this patch, but do start a new thread and ask: * Does anyone mind if we support descriptive text in the ChangeLog? * Does anyone mind if we use the terse "Likewise" format? Then we'll get some yays and nays and update the Contribution Checklist. I don't see why anyone would object, but I'd rather we talk about it, briefly. Cheers, Carlos.
Carlos O'Donell wrote: > * Does anyone mind if we use the terse "Likewise" format? There must be some misunderstanding here, as the "Likewise" format is *less* terse than the format I used. That is, I did this: > + Fix broken overflow check in posix_fallocate > + * sysdeps/posix/posix_fallocate.c (posix_fallocate): > + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > + Fix parenthesization typo. rather than this: > + Fix broken overflow check in posix_fallocate > + * sysdeps/posix/posix_fallocate.c (posix_fallocate): > + Fix parenthesization typo. > + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): > + Likewise.
On 08/28/2015 01:52 PM, Paul Eggert wrote: > Carlos O'Donell wrote: >> * Does anyone mind if we use the terse "Likewise" format? > > There must be some misunderstanding here, as the "Likewise" format is *less* terse than the format I used. That is, I did this: > >> + Fix broken overflow check in posix_fallocate >> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> + Fix parenthesization typo. This is what I called the `terse "Likewise"` format. > rather than this: > >> + Fix broken overflow check in posix_fallocate >> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> + Fix parenthesization typo. >> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> + Likewise. This is the standard one. Does that clarify things? c.
Carlos O'Donell wrote:
> Does that clarify things?
Yes.
On Fri, 28 Aug 2015, Carlos O'Donell wrote: > * Does anyone mind if we support descriptive text in the ChangeLog? Of course we should support it; it's permitted in the GNU Coding Standards. I see no use in being stricter about ChangeLog format than what the GNU Coding Standards say (beyond the [BZ #N] annotations, where the GCS say nothing about how to indicate bug numbers). (Of course, if we get automatic processing of commit messages to replace manual ChangeLog editing, we do need to get strict about formatting the commit message in the way expected by that automatic processing - but that's about enabling the processing to see what bit is the ChangeLog entry, not about the details of formatting within the ChangeLog entry.) > * Does anyone mind if we use the terse "Likewise" format? I'm happy with it. That is, (a) with the format explicitly mentioned in the GCS: * file (func1, func2, func3) (func4, func5): Message. and (b) with the generalization described where multiple files are involved.
On 08/26/2015 08:47 AM, Florian Weimer wrote: > On 08/26/2015 08:42 AM, Paul Eggert wrote: >> * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> Fix parenthesization typo. >> --- >> ChangeLog | 5 +++++ >> sysdeps/posix/posix_fallocate.c | 2 +- >> sysdeps/posix/posix_fallocate64.c | 2 +- >> 3 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/ChangeLog b/ChangeLog >> index bd6d027..458b850 100644 >> --- a/ChangeLog >> +++ b/ChangeLog >> @@ -1,5 +1,10 @@ >> 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> >> >> + Fix broken overflow check in posix_fallocate >> + * sysdeps/posix/posix_fallocate.c (posix_fallocate): >> + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): >> + Fix parenthesization typo. > > Please add the BZ #18873 reference before committing (also to NEWS). I have committed this with the bug reference.
On 08/28/2015 05:49 PM, Paul Eggert wrote: > Second, there's a lot of bureaucracy involved in getting obvious bugs > like these fixed. The current distracting thread about ChangeLog style > is a symptom of the problem. I'm sorry you had a bad experience contributing. The ChangeLog discussion was not entirely pointless. I would not have started it if the bug reference was not missing. The posix_fallocate bug would not have needed a bug report. I'm not yet sure about the vfprintf bug. > I can't help contrasting the somewhat offputting responses to my four > glibc Coverity scan patches with the quite-friendly response I got from > the gettext project when I filed the gettext-related patch upstream. > Here's the entire interaction: > > I ran "git send-email --to=bug-gettext@gnu.org". > > Within a few hours, Daiki Ueno replied "Thanks, applied." How can we import such patches in a way that the automated ChangeLog merging kicks in?
Florian Weimer wrote: > How can we import such patches in a way that the automated ChangeLog > merging kicks in? I haven't been following the details of the plans for glibc. For Emacs, coreutils, etc., I use "git am" to import a patch from email. There's no ChangeLog change to merge, since there's no ChangeLog under git control; the ChangeLog is computed automatically from the git commit log (which is part of the email). I hope glibc will become just as easy to deal with.
On 08/31/2015 12:43 PM, Paul Eggert wrote: > Florian Weimer wrote: >> How can we import such patches in a way that the automated >> ChangeLog merging kicks in? > > I haven't been following the details of the plans for glibc. For > Emacs, coreutils, etc., I use "git am" to import a patch from email. > There's no ChangeLog change to merge, since there's no ChangeLog > under git control; the ChangeLog is computed automatically from the > git commit log (which is part of the email). I hope glibc will > become just as easy to deal with. We endeavour to get to the point where "git am" just works. We had some rather long discussions about how to make progress on this at GNU Cauldron 2015 in Prague. c.
diff --git a/ChangeLog b/ChangeLog index bd6d027..458b850 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> + Fix broken overflow check in posix_fallocate + * sysdeps/posix/posix_fallocate.c (posix_fallocate): + * sysdeps/posix/posix_fallocate64.c (__posix_fallocate64_l64): + Fix parenthesization typo. + Fix memory leak in printf_positional * stdio-common/vfprintf.c (printf_positional): Free temporary data allocated via malloc. diff --git a/sysdeps/posix/posix_fallocate.c b/sysdeps/posix/posix_fallocate.c index e7fe201..d0479a6 100644 --- a/sysdeps/posix/posix_fallocate.c +++ b/sysdeps/posix/posix_fallocate.c @@ -37,7 +37,7 @@ posix_fallocate (int fd, __off_t offset, __off_t len) /* Perform overflow check. The outer cast relies on a GCC extension. */ - if ((__off_t) ((uint64_t) offset) + ((uint64_t) len) < 0) + if ((__off_t) ((uint64_t) offset + (uint64_t) len) < 0) return EFBIG; /* pwrite below will not do the right thing in O_APPEND mode. */ diff --git a/sysdeps/posix/posix_fallocate64.c b/sysdeps/posix/posix_fallocate64.c index ee32679..fb2dac6 100644 --- a/sysdeps/posix/posix_fallocate64.c +++ b/sysdeps/posix/posix_fallocate64.c @@ -37,7 +37,7 @@ __posix_fallocate64_l64 (int fd, __off64_t offset, __off64_t len) /* Perform overflow check. The outer cast relies on a GCC extension. */ - if ((__off64_t) ((uint64_t) offset) + ((uint64_t) len) < 0) + if ((__off64_t) ((uint64_t) offset + (uint64_t) len) < 0) return EFBIG; /* pwrite64 below will not do the right thing in O_APPEND mode. */