Re: [hackers] [sbase] [PATCH 04/10] Don't use buffered IO (fread) when not appropriate

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Tue, 6 Dec 2016 10:26:22 +0100

On Tue, Dec 6, 2016 at 9:17 AM, Michael Forney <mforney_AT_mforney.org> wrote:
> On Mon, Dec 5, 2016 at 12:15 PM, Silvan Jegen <s.jegen_AT_gmail.com> wrote:
>> Hi
>>
>> Some comments below.
>>
>> On Sun, Dec 04, 2016 at 09:55:06PM -0800, Michael Forney wrote:
>>> diff --git a/cksum.c b/cksum.c
>>> index 570ca81..b53ec17 100644
>>> --- a/cksum.c
>>> +++ b/cksum.c
>>> _AT_@ -1,7 +1,9 @@
>>> /* See LICENSE file for copyright and license details. */
>>> +#include <fcntl.h>
>>> #include <inttypes.h>
>>> #include <stdio.h>
>>
>> This include is not needed anymore.
>
> It is needed for printf, putchar, fputs, and stdout.

You're right.

It only compiled for me because "util.h" includes stdio.h so the
definitions are included already. We can still remove this include if
we don't want it to be included twice but I don't know which approach
is preferred here.


>>> diff --git a/od.c b/od.c
>>> index b5884e7..9ae1e29 100644
>>> --- a/od.c
>>> +++ b/od.c
>>> _AT_@ -1,8 +1,10 @@
>>> /* See LICENSE file for copyright and license details. */
>>> +#include <fcntl.h>
>>> #include <stdint.h>
>>> #include <stdio.h>
>>
>> This include is not needed anymore.
>
> It is needed for printf, fputc, and stdout.

Same here.


>>> for (i = 0; i < argc; i++) {
>>> - if (!(fps[i] = fopen(argv[i], aflag ? "a" : "w"))) {
>>> - weprintf("fopen %s:", argv[i]);
>>> + if ((fds[i] = open(argv[i], O_WRONLY|O_CREAT|aflag, 0666)) < 0) {
>>
>> umask will be honored when creating a file but I am wondering if just
>> setting mode_t to 0660 would be the safer option here.
>
> POSIX says that it has to be 0666[0]:
>
> """
> When a file that does not exist is created, the following features
> defined in the System Interfaces volume of POSIX.1-2008 shall apply
> unless the utility or function description states otherwise:
>
> ...
>
> 3. If the file is a regular file, the permission bits of the file
> shall be set to: S_IROTH | S_IWOTH | S_IRGRP | S_IWGRP | S_IRUSR |
> S_IWUSR
>
> (see the description of File Modes in XBD Headers, <sys/stat.h>)
> except that the bits specified by the file mode creation mask of the
> process shall be cleared.
> """

Ah, I did not know that POSIX had anything to say on this. Then we
keep it the way it is.


Cheers,

Silvan

> [0] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap01.html#tag_17_01_01_04
>
Received on Tue Dec 06 2016 - 10:26:22 CET

This archive was generated by hypermail 2.3.0 : Tue Dec 06 2016 - 10:36:14 CET