Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Cág]

Oops, wrong recipient.

Nico Golde wrote:

Im sorry but this is utter non-sense ;) also not generalizable like 
this.


Sorry, I didn't know what I was talking about (:
But, in this case I think it's better without it.

Cág

[hackers] [dmenu] dmenu.1: fix -l option || Quentin Rameau

[git]

commit ff8daf88475960f6ccc4e3ded3214147ecb21809
Author: Quentin Rameau 
AuthorDate: Sat Oct 8 14:36:04 2016 +0200
Commit: Hiltjo Posthuma 
CommitDate: Fri Oct 14 15:26:34 2016 +0200

dmenu.1: fix -l option

diff --git a/dmenu.1 b/dmenu.1
index d3ab805..f0e3bd7 100644
--- a/dmenu.1
+++ b/dmenu.1
@@ -7,9 +7,9 @@ dmenu \- dynamic menu
 .RB [ \-f ]
 .RB [ \-i ]
 .RB [ \-l
+.IR lines ]
 .RB [ \-m
 .IR monitor ]
-.IR lines ]
 .RB [ \-p
 .IR prompt ]
 .RB [ \-fn

Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Cág]

Nico Golde wrote:


Why do you think this is "crap"?
Looks like splitting beans to me.


Without going deep I'd say "#define" is usually a bad idea from what
I've seen though I don't have a lot of programming experience as people 
here.

the future of ii (was: [hackers] [PATCH] [ii] get rid of EXIT_bla crap)

[Nico Golde]

Hi,
* Markus Teich  [2016-10-14 12:45]:
> Laslo Hunhold wrote:
> > Oh, and before you inform me. I know that OpenVMS has the roles
> > inverted (exit(1) means success). However, there are two aspects we
> > should keep in mind:
> > 
> > 1) OpenVMS is not the most popular OS any more.
> > 2) Using EXIT_* we cannot write reliable manuals, as we do
> >use 0, 1, 2, ... there.
> 
> adding to that, when reading code with EXIT_bla, you don't know immediately 
> how
> it is defined. Therefore it can easily be confused when also using C-functions
> which return 1 as "true" and 0 as "false". I get that abstracting the value 
> away
> and using a name instead can make sense, but when using `return` or `exit()`,
> you don't have to give meaning to the `0` or `1` while in a line like `ret =
> calloc(5 * sizeof(struct bla) + 7);` it is very helpful to know what those
> numbers refer to semantically.

FWIW, I do think that EXIT_FAILURE and EXIT_SUCCESS is more readable 
than 0/1 and well defined. Ofc, this is not true for EXIT_TIMEOUT 
necessarily (but that hardly makes a difference for practical purposes here 
imho).

Anyhow, I don't think I have the mental capacity to have a purely academic 
discussion about this topic and I find it for the most part very subjective.
I also realize that I lack the time and am emotionally not attached to ii 
anymore as I used to be in the past. I'm essentially just a bottleneck to its 
development.

Hiltjo Posthuma was kind enough to offer taking over the maintenance for this 
project. Thanks for that, I really appreciate this! So I'm leaving this change 
to his discretion.

Cheers,
Nico
-- 
Nico Golde - XMPP: n...@jabber.ccc.de - GPG: 0xA0A0


pgp6S639tdEiA.pgp
Description: PGP signature

Re: [hackers] [sbase][patch]find: copy path before using basename

[Laslo Hunhold]

On Wed, 5 Oct 2016 15:41:55 -0700
Evan Gates  wrote:

Hey Evan,

> On Mon, Oct 3, 2016 at 3:10 PM, FRIGN  wrote:
> > Please don't use VLA's. Use estrdup() in this case.
> 
> sbase-find_basename2.diff: revised patch without VLAs
> sbase-find_noVLAs.diff: path to remove all VLAs in find

let me give you some feedback on the patches.

### sbase-find_basename2.diff

 static int
 pri_name(struct arg *arg)
 {
-   return !fnmatch((char *)arg->extra.p, basename(arg->path), 0);
+   char *path = estrdup(arg->path);
+   int ret = !fnmatch((char *)arg->extra.p, basename(path), 0);
+   free(path);
+   return ret;
 }

I'd strictly separate declaration from initialization so it is easier
to see why we even need "ret" here.

static int
pri_name(struct arg *arg)
{
int ret;
char *path;

path = estrdup(arg->path);
ret = !fnmatch((char *)arg->extra.p, basename(path), 0);
free(path);

return ret;
}

### sbase-find_noVLAs.diff

This already looks good, but I really am a big fan of initializing
variables as late as possible.

+   struct tok *tok, *rpn, *out, **top,
+  *infix = emalloc((2 * argc + 1) * sizeof(*infix)),
+  **stack = emalloc(argc * sizeof(*stack));

Use ereallocarray() for cases like these, it also makes the code more
readable.
Thanks for your contributions!

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Markus Teich]

Laslo Hunhold wrote:
> Oh, and before you inform me. I know that OpenVMS has the roles
> inverted (exit(1) means success). However, there are two aspects we
> should keep in mind:
> 
>   1) OpenVMS is not the most popular OS any more.
>   2) Using EXIT_* we cannot write reliable manuals, as we do
>  use 0, 1, 2, ... there.

Heyho,

adding to that, when reading code with EXIT_bla, you don't know immediately how
it is defined. Therefore it can easily be confused when also using C-functions
which return 1 as "true" and 0 as "false". I get that abstracting the value away
and using a name instead can make sense, but when using `return` or `exit()`,
you don't have to give meaning to the `0` or `1` while in a line like `ret =
calloc(5 * sizeof(struct bla) + 7);` it is very helpful to know what those
numbers refer to semantically.

--Markus

Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Laslo Hunhold]

On Fri, 14 Oct 2016 12:45:27 +0200
Laslo Hunhold  wrote:

> I totally agree with you there. It just obfuscates the code, even
> suggesting EXIT_TIMEOUT was part of Posix, which it is not. I think it
> should be common knowledge by now that a zero exit status indicates
> success and a non-zero exit-status (1, 2, ...) indicates failure.

Oh, and before you inform me. I know that OpenVMS has the roles
inverted (exit(1) means success). However, there are two aspects we
should keep in mind:

1) OpenVMS is not the most popular OS any more.
2) Using EXIT_* we cannot write reliable manuals, as we do
   use 0, 1, 2, ... there.

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Laslo Hunhold]

On Fri, 14 Oct 2016 01:42:16 +0200
Markus Teich  wrote:

Hey Markus,

> this was done to most other suckless projects as well and I just
> noticed it when reading the EXIT_TIMEOUT patch, so here you go.

I totally agree with you there. It just obfuscates the code, even
suggesting EXIT_TIMEOUT was part of Posix, which it is not. I think it
should be common knowledge by now that a zero exit status indicates
success and a non-zero exit-status (1, 2, ...) indicates failure.

I second this.

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [PATCH] [ii] get rid of EXIT_bla crap

[Nico Golde]

Why do you think this is "crap"?
Looks like splitting beans to me. 

Cheers,
Nico

> Am 14.10.2016 um 00:42 schrieb Markus Teich :
> 
> ---
> 
> 
> Heyho,
> 
> this was done to most other suckless projects as well and I just noticed it 
> when
> reading the EXIT_TIMEOUT patch, so here you go.
> 
> --Markus
> 
> 
> ii.c | 30 ++
> 1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/ii.c b/ii.c
> index 5d57458..857388e 100644
> --- a/ii.c
> +++ b/ii.c
> @@ -19,8 +19,6 @@
> #include 
> #include 
> 
> -#define EXIT_TIMEOUT 2
> -
> #ifndef PIPE_BUF /* For OS that doesn't includes PIPE_BUF in limits.h, 
> FreeBSD? */
> #define PIPE_BUF _POSIX_PIPE_BUF
> #endif
> @@ -50,7 +48,7 @@ static void usage() {
>  "(C)opyright MMV-MMXI Nico Golde\n"
>  "usage: ii [-i ] [-s ] [-p ]\n"
>  "  [-n ] [-k ] [-f ]\n", stderr);
> -exit(EXIT_FAILURE);
> +exit(1);
> }
> 
> static char *striplower(char *s) {
> @@ -93,7 +91,7 @@ static int get_filepath(char *filepath, size_t len, char 
> *channel, char *file) {
> static void create_filepath(char *filepath, size_t len, char *channel, char 
> *suffix) {
>if(!get_filepath(filepath, len, striplower(channel), suffix)) {
>fputs("ii: path to irc directory too long\n", stderr);
> -exit(EXIT_FAILURE);
> +exit(1);
>}
> }
> 
> @@ -117,12 +115,12 @@ static void add_channel(char *cname) {
>fd = open_channel(name);
>if(fd == -1) {
>printf("ii: exiting, cannot create in channel: %s\n", name);
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>c = calloc(1, sizeof(Channel));
>if(!c) {
>perror("ii: cannot allocate memory");
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>if(!channels) channels = c;
>else {
> @@ -162,18 +160,18 @@ static int tcpopen(unsigned short port) {
>memset(, 0, sizeof(struct sockaddr_in));
>if(!hp) {
>perror("ii: cannot retrieve host information");
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>sin.sin_family = AF_INET;
>memcpy(_addr, hp->h_addr, hp->h_length);
>sin.sin_port = htons(port);
>if((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0) {
>perror("ii: cannot create socket");
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>if(connect(fd, (const struct sockaddr *) , sizeof(sin)) < 0) {
>perror("ii: cannot connect to host");
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>return fd;
> }
> @@ -414,7 +412,7 @@ static void handle_server_output() {
>static char buf[PIPE_BUF];
>if(read_line(irc, PIPE_BUF, buf) == -1) {
>perror("ii: remote host closed connection");
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>proc_server_cmd(buf);
> }
> @@ -444,11 +442,11 @@ static void run() {
>if(errno == EINTR)
>continue;
>perror("ii: error on select()");
> -exit(EXIT_FAILURE);
> +exit(1);
>} else if(r == 0) {
>if(time(NULL) - last_response >= PING_TIMEOUT) {
>print_out(NULL, "-!- ii shutting down: ping timeout");
> -exit(EXIT_TIMEOUT);
> +exit(2);
>}
>write(irc, ping_msg, strlen(ping_msg));
>continue;
> @@ -474,7 +472,7 @@ int main(int argc, char *argv[]) {
> 
>if(!spw) {
>fputs("ii: getpwuid() failed\n", stderr);
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>snprintf(nick, sizeof(nick), "%s", spw->pw_name);
>snprintf(prefix, sizeof(prefix),"%s/irc", spw->pw_dir);
> @@ -496,13 +494,13 @@ int main(int argc, char *argv[]) {
>#ifdef __OpenBSD__/* OpenBSD pledge(2) support */
>if (pledge("stdio rpath wpath cpath dpath", NULL) == -1) {
>fprintf(stderr, "ii pledge: %s\n", strerror(errno));
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>#endif
> 
>if(!snprintf(path, sizeof(path), "%s/%s", prefix, host)) {
>fputs("ii: path to irc directory too long\n", stderr);
> -exit(EXIT_FAILURE);
> +exit(1);
>}
>create_dirtree(path);
> 
> @@ -510,5 +508,5 @@ int main(int argc, char *argv[]) {
>login(key, fullname);
>run();
> 
> -return EXIT_SUCCESS;
> +return 0;
> }
> -- 
> 2.7.3
> 
>