On Sun, Mar 29, 2015 at 5:17 PM, Daniel Mack <dan...@zonque.org> wrote:
> On 03/29/2015 03:04 PM, Alexander Sverdlin wrote:
>> On 29/03/15 13:44, Daniel Mack wrote:
>>>> @@ -184,6 +185,7 @@ vmstat_next:
>>>>> n = pread(schedstat, buf, sizeof(buf) - 1, 0); if (n <= 0) {
>>>>> close(schedstat); + schedstat = 0;
>>> Note that 0 is a valid file descriptor number. You should really
>>> rather reset the variables to -1 and check for '>= 0'. This applies
>>> to all hunks of this patch, which also needs a rebase onto the
>>> current git HEAD.
>>
>> I believe, it was HEAD as of time of patch submission, but I can of
>> course rebase it once again. Regarding 0: everywhere in the program
>> it relies on the fact that newly allocated memory is zeroed and files
>> are only opened if the corresponding file descriptor field of a
>> structure is 0. So do you propose to change the logic everywhere
>> where the files are opened?
>
> I see. As that code doesn't close stdin, 0 can't be returned by any
> open*(), so that's not a real issues, but all code should still be
> written in a way that it treats 0 as valid descriptor. So we need to
> explicitly initialize fd variables to -1 after new0(), and refactor code
> to where necessary.
Right, we rely on uninitialized fds to be negative. Even when it is
not commonly used, the _cleanup_close_ logic and other commonly
used fd handling functions, which should probably used in bootchart
too, rely on it:
http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c#n272
Kay
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
