Development discussion for mrsh.

You can setup your Git repository like so:

git config sendemail.to ~emersion/mrsh-dev@lists.sr.ht
13 4

[PATCH] Implemented ulimit builtin

Julian P Samaroo
Details
Message ID
<20190101172754.2441-1-jpsamaroo@jpsamaroo.me>
Sender timestamp
1546363674
DKIM signature
missing
Download raw message
Patch: +73 -0
Warning: I'm totally new to the email/git contribution process, and I'm
a novice C programmer. I tried my best though :)
---
 builtin/builtin.c |  1 +
 builtin/ulimit.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/builtin.h |  1 +
 meson.build       |  1 +
 test/ulimit.sh    |  5 +++++
 5 files changed, 73 insertions(+)
 create mode 100644 builtin/ulimit.c
 create mode 100644 test/ulimit.sh

diff --git a/builtin/builtin.c b/builtin/builtin.c
index 2692532..f54e95f 100644
--- a/builtin/builtin.c
@@ -31,6 +31,7 @@ static const struct builtin builtins[] = {
 	{ "times", builtin_times, true },
 	{ "true", builtin_true, false },
 	{ "type", builtin_type, false },
+	{ "ulimit", builtin_ulimit, false },
 	{ "umask", builtin_umask, false },
 	{ "unalias", builtin_unalias, false },
 	{ "unset", builtin_unset, true },
diff --git a/builtin/ulimit.c b/builtin/ulimit.c
new file mode 100644
index 0000000..073ca34
--- /dev/null
@@ -0,0 +1,65 @@
+#define _POSIX_C_SOURCE 200809L
+#include <mrsh/getopt.h>
+#include <mrsh/shell.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+#include "builtin.h"
+
+static const char ulimit_usage[] = "usage: ulimit [-f blocks]\n";
+
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
+	struct rlimit old, new;
+	bool set_limit = false;
+	long int new_limit;
+	char *endptr;
+	mrsh_optind = 1;
+	int opt;
+	if ((opt = mrsh_getopt(argc, argv, ":f")) != -1) {
+		if (opt == 'f') {
+			set_limit = true;
+		} else {
+			fprintf(stderr, ulimit_usage);
+			return EXIT_FAILURE;
+		}
+	}
+	if (set_limit && mrsh_optind != argc-1) {
+		fprintf(stderr, ulimit_usage);
+		return EXIT_FAILURE;
+	}
+
+	if (set_limit) {
+		errno = 0;
+		new_limit = strtol(argv[mrsh_optind], &endptr, 10);
+		if ((errno == ERANGE && (new_limit == LONG_MAX || new_limit == LONG_MIN))
+				|| (errno != 0 && new_limit == 0)) {
+			fprintf(stderr, "strtol error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (endptr == argv[mrsh_optind]) {
+			fprintf(stderr, "ulimit error: Invalid argument: %s\n", argv[mrsh_optind]);
+			return EXIT_FAILURE;
+		}
+		new.rlim_cur = new_limit * 1024;
+		new.rlim_max = new_limit * 1024;
+		if (setrlimit(RLIMIT_FSIZE, &new) != 0) {
+			fprintf(stderr, "setrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+	} else {
+		if (getrlimit(RLIMIT_FSIZE, &old) != 0) {
+			fprintf(stderr, "getrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (old.rlim_max == RLIM_INFINITY) {
+			printf("unlimited\n");
+		} else {
+			printf("%lu\n", old.rlim_max / 1024);
+		}
+	}
+	return EXIT_SUCCESS;
+}
diff --git a/include/builtin.h b/include/builtin.h
index ed437b2..afebdc4 100644
--- a/include/builtin.h
+++ b/include/builtin.h
@@ -24,6 +24,7 @@ int builtin_shift(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_times(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_true(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_type(struct mrsh_state *state, int argc, char *argv[]);
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
diff --git a/meson.build b/meson.build
index a51e623..b33c354 100644
--- a/meson.build
+++ b/meson.build
@@ -64,6 +64,7 @@ lib_mrsh = library(
 		'builtin/times.c',
 		'builtin/true.c',
 		'builtin/type.c',
+		'builtin/ulimit.c',
 		'builtin/umask.c',
 		'builtin/unalias.c',
 		'builtin/unset.c',
diff --git a/test/ulimit.sh b/test/ulimit.sh
new file mode 100644
index 0000000..748445a
--- /dev/null
+++ b/test/ulimit.sh
@@ -0,0 +1,5 @@
+#!/bin/sh
+
+ulimit
+ulimit -f 65536
+ulimit
-- 
2.16.4
Details
Message ID
<20190101173356.GI1621@homura.localdomain>
In-Reply-To
<20190101172754.2441-1-jpsamaroo@jpsamaroo.me> (view parent)
Sender timestamp
1546364036
DKIM signature
permerror
Download raw message
On 2019-01-01 11:27 AM, Julian P Samaroo wrote:
> Warning: I'm totally new to the email/git contribution process, and I'm
> a novice C programmer. I tried my best though :)

You did remarkably well for someone new to this workflow! I might as
well pick nits, though - this sort of message is considered "timely
commentary", and the way you included it will write it to the permanent
git log. Instead you want to include this after the "---" line in the
future. You can add some extra newlines after "---" to make your message
more comfortable if you like; the first line of the message which is
meaningful to git is diff --git [...].

> diff --git a/builtin/ulimit.c b/builtin/ulimit.c
> -%<-
> +	if ((opt = mrsh_getopt(argc, argv, ":f")) != -1) {
> +		if (opt == 'f') {
> +			set_limit = true;
> +		} else {
> +			fprintf(stderr, ulimit_usage);

Prefer fprintf(stdder, "%s", ulimit_usage);

It's inconsistent with everywhere else, but everywhere else is a bug
(already ticked).

> +			return EXIT_FAILURE;
> +		}
> +	}
> +	if (set_limit && mrsh_optind != argc-1) {

style: add spaces around operators like -

> +		fprintf(stderr, ulimit_usage);

fprintf needs fixing here as well

> +	if (set_limit) {
> +		errno = 0;
> +		new_limit = strtol(argv[mrsh_optind], &endptr, 10);
> +		if ((errno == ERANGE && (new_limit == LONG_MAX || new_limit == LONG_MIN))

style: 80 column limit

> +				|| (errno != 0 && new_limit == 0)) {
> +			fprintf(stderr, "strtol error: %s\n", strerror(errno));
> +			return EXIT_FAILURE;
> +		}
> +		if (endptr == argv[mrsh_optind]) {
> +			fprintf(stderr, "ulimit error: Invalid argument: %s\n", argv[mrsh_optind]);

style: 80 cols

> diff --git a/test/ulimit.sh b/test/ulimit.sh
> -%<-
> +ulimit
> +ulimit -f 65536
> +ulimit

Maybe this test should check /proc/self/limits? Should probably test if
it exists first and pass the test, if not since /proc is not
standardized.
Julian P Samaroo
Details
Message ID
<20190103201137.jf4cn56gf23ljomd@phantasos>
In-Reply-To
<20190101173356.GI1621@homura.localdomain> (view parent)
Sender timestamp
1546546297
DKIM signature
missing
Download raw message
(My first proper email reply, please critique!)

On Tue, Jan 01, 2019 at 12:33:56PM -0500, Drew DeVault wrote:
> On 2019-01-01 11:27 AM, Julian P Samaroo wrote:
> > Warning: I'm totally new to the email/git contribution process, and I'm
> > a novice C programmer. I tried my best though :)
> 
> You did remarkably well for someone new to this workflow! I might as
> well pick nits, though - this sort of message is considered "timely
> commentary", and the way you included it will write it to the permanent
> git log. Instead you want to include this after the "---" line in the
> future. You can add some extra newlines after "---" to make your message
> more comfortable if you like; the first line of the message which is
> meaningful to git is diff --git [...].
> 

Thanks for the pointer, future patches will be tested via git-am so that I
don't make that mistake again :)

> > diff --git a/test/ulimit.sh b/test/ulimit.sh
> > -%<-
> > +ulimit
> > +ulimit -f 65536
> > +ulimit
> 
> Maybe this test should check /proc/self/limits? Should probably test if
> it exists first and pass the test, if not since /proc is not
> standardized.

I'm having a bit of trouble getting this to work properly. Partly because of
missing functionality in mrsh, but mostly because I suck at shell scripting.
Here's what I have so far, hopefully you can give me some tips on what goes
within the if statements:

#!/bin/sh

set -e

mrsh_limits=`ulimit`
[ $mrsh_limits == "unlimited" ]
if [ -a /proc/self/limits ]; then
out=`cat /proc/self/limits | grep "Max file size" | grep unlimited`
fi

ulimit -f 65536

mrsh_limits=`ulimit`
[ $mrsh_limits -eq 65536 ]
if [ -a /proc/self/limits ]; then
out=`cat /proc/self/limits | grep "Max file size" | grep 67108864`
fi


Thanks for the review!

Julian S.
Details
Message ID
<20190103221341.GB1605@homura.localdomain>
In-Reply-To
<20190103201137.jf4cn56gf23ljomd@phantasos> (view parent)
Sender timestamp
1546553621
DKIM signature
permerror
Download raw message
On 2019-01-03  2:11 PM, Julian P Samaroo wrote:
> mrsh_limits=`ulimit`
> [ $mrsh_limits == "unlimited" ]
> if [ -a /proc/self/limits ]; then
> out=`cat /proc/self/limits | grep "Max file size" | grep unlimited`
> fi

I'm not sure what -a is supposed to be. You probably want -e. You also
don't want to assign this to a variable, I'd just let it complete and
discard the output. With set -e it'll fail the test if it exits nonzero
(which grep will if there's no match). You can also skip the cat, just
do:

    grep "Max file size" /proc/self/limits | grep unlimited

Also, don't do if [ ]; then - put them on separate lines.

> ulimit -f 65536
> 
> mrsh_limits=`ulimit`
> [ $mrsh_limits -eq 65536 ]
> if [ -a /proc/self/limits ]; then
> out=`cat /proc/self/limits | grep "Max file size" | grep 67108864`

Where did 67108864 come from? You set it to 65536.
Julian P Samaroo
Details
Message ID
<20190103223136.o3hlwwcyzdg574s6@phantasos>
In-Reply-To
<20190103221341.GB1605@homura.localdomain> (view parent)
Sender timestamp
1546554697
DKIM signature
missing
Download raw message
On Thu, Jan 03, 2019 at 05:13:41PM -0500, Drew DeVault wrote:
> On 2019-01-03  2:11 PM, Julian P Samaroo wrote:
> > mrsh_limits=`ulimit`
> > [ $mrsh_limits == "unlimited" ]
> > if [ -a /proc/self/limits ]; then
> > out=`cat /proc/self/limits | grep "Max file size" | grep unlimited`
> > fi
> 
> I'm not sure what -a is supposed to be. You probably want -e. You also

I guess that's a bash-ism; my POSIX-fu is not up to par. Is there a better
manpage I can use for referencing shell syntax?

> don't want to assign this to a variable, I'd just let it complete and
> discard the output. With set -e it'll fail the test if it exits nonzero
> (which grep will if there's no match). You can also skip the cat, just
> do:
> 
>     grep "Max file size" /proc/self/limits | grep unlimited

I thought I was getting weird behavior where mrsh would act as if it returned
0 even though it didn't, I'll re-test this a bit (and document any
inconsistencies that occur, if any). I'll also remove the cat, good point.

> 
> Also, don't do if [ ]; then - put them on separate lines.
> 

Will do.

> > ulimit -f 65536
> > 
> > mrsh_limits=`ulimit`
> > [ $mrsh_limits -eq 65536 ]
> > if [ -a /proc/self/limits ]; then
> > out=`cat /proc/self/limits | grep "Max file size" | grep 67108864`
> 
> Where did 67108864 come from? You set it to 65536.

/proc/self/limits reports the value in bytes (as does getrlimit and
setrlimit), however I did another dumb and followed bash's example of scaling
by 1024 for output and for -f. However, a re-read of the POSIX manpage for
ulimit indicates that -f takes its argument in terms of 512-byte blocks, so I
guess my ulimit implementation is also bad. I'll fix this up in my v2. Note
that then the test will still check for a value that is different than what
you set with the -f flag, due to the difference in units.

Thanks!

Julian S.
Details
Message ID
<20190103223331.GC1605@homura.localdomain>
In-Reply-To
<20190103223136.o3hlwwcyzdg574s6@phantasos> (view parent)
Sender timestamp
1546554811
DKIM signature
permerror
Download raw message
On 2019-01-03  4:31 PM, Julian P Samaroo wrote:
> I guess that's a bash-ism; my POSIX-fu is not up to par. Is there a better
> manpage I can use for referencing shell syntax?

The POSIX spec:

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

Here's the index, for bookmarking:

http://pubs.opengroup.org/onlinepubs/9699919799/

I also have the shell page specifically bookmarked:

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

> I thought I was getting weird behavior where mrsh would act as if it returned
> 0 even though it didn't, I'll re-test this a bit (and document any
> inconsistencies that occur, if any). I'll also remove the cat, good point.
>
> -%<-
>
> /proc/self/limits reports the value in bytes (as does getrlimit and
> setrlimit), however I did another dumb and followed bash's example of scaling
> by 1024 for output and for -f. However, a re-read of the POSIX manpage for
> ulimit indicates that -f takes its argument in terms of 512-byte blocks, so I
> guess my ulimit implementation is also bad. I'll fix this up in my v2. Note
> that then the test will still check for a value that is different than what
> you set with the -f flag, due to the difference in units.
 
I guess it's a good thing we extended this test :)

[PATCH-v2] Implemented ulimit builtin

Details
Message ID
<20190104143119.4979-1-jpsamaroo@jpsamaroo.me>
In-Reply-To
<20190103223331.GC1605@homura.localdomain> (view parent)
Sender timestamp
1546612280
DKIM signature
missing
Download raw message
Patch: +89 -0
From: Julian P Samaroo <jpsamaroo@jpsamaroo.me>

---
Adds ulimit builtin and a test for it.
 builtin/builtin.c |  1 +
 builtin/ulimit.c  | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 include/builtin.h |  1 +
 meson.build       |  1 +
 test/ulimit.sh    | 19 ++++++++++++++
 5 files changed, 89 insertions(+)
 create mode 100644 builtin/ulimit.c
 create mode 100644 test/ulimit.sh

diff --git a/builtin/builtin.c b/builtin/builtin.c
index 2692532..f54e95f 100644
--- a/builtin/builtin.c
@@ -31,6 +31,7 @@ static const struct builtin builtins[] = {
 	{ "times", builtin_times, true },
 	{ "true", builtin_true, false },
 	{ "type", builtin_type, false },
+	{ "ulimit", builtin_ulimit, false },
 	{ "umask", builtin_umask, false },
 	{ "unalias", builtin_unalias, false },
 	{ "unset", builtin_unset, true },
diff --git a/builtin/ulimit.c b/builtin/ulimit.c
new file mode 100644
index 0000000..7ff7079
--- /dev/null
@@ -0,0 +1,67 @@
+#define _POSIX_C_SOURCE 200809L
+#include <mrsh/getopt.h>
+#include <mrsh/shell.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+#include "builtin.h"
+
+static const char ulimit_usage[] = "usage: ulimit [-f blocks]\n";
+
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
+	struct rlimit old, new;
+	bool set_limit = false;
+	long int new_limit;
+	char *endptr;
+	mrsh_optind = 1;
+	int opt;
+	if ((opt = mrsh_getopt(argc, argv, ":f")) != -1) {
+		if (opt == 'f') {
+			set_limit = true;
+		} else {
+			fprintf(stderr, "%s", ulimit_usage);
+			return EXIT_FAILURE;
+		}
+	}
+	if (set_limit && mrsh_optind != argc - 1) {
+		fprintf(stderr, "%s", ulimit_usage);
+		return EXIT_FAILURE;
+	}
+
+	if (set_limit) {
+		errno = 0;
+		new_limit = strtol(argv[mrsh_optind], &endptr, 10);
+		if ((errno == ERANGE &&
+				(new_limit == LONG_MAX || new_limit == LONG_MIN))
+				|| (errno != 0 && new_limit == 0)) {
+			fprintf(stderr, "strtol error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (endptr == argv[mrsh_optind]) {
+			fprintf(stderr, "ulimit error: Invalid argument: %s\n",
+				argv[mrsh_optind]);
+			return EXIT_FAILURE;
+		}
+		new.rlim_cur = new_limit * 512;
+		new.rlim_max = new_limit * 512;
+		if (setrlimit(RLIMIT_FSIZE, &new) != 0) {
+			fprintf(stderr, "setrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+	} else {
+		if (getrlimit(RLIMIT_FSIZE, &old) != 0) {
+			fprintf(stderr, "getrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (old.rlim_max == RLIM_INFINITY) {
+			printf("unlimited\n");
+		} else {
+			printf("%lu\n", old.rlim_max / 512);
+		}
+	}
+	return EXIT_SUCCESS;
+}
diff --git a/include/builtin.h b/include/builtin.h
index ed437b2..afebdc4 100644
--- a/include/builtin.h
+++ b/include/builtin.h
@@ -24,6 +24,7 @@ int builtin_shift(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_times(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_true(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_type(struct mrsh_state *state, int argc, char *argv[]);
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
diff --git a/meson.build b/meson.build
index a51e623..b33c354 100644
--- a/meson.build
+++ b/meson.build
@@ -64,6 +64,7 @@ lib_mrsh = library(
 		'builtin/times.c',
 		'builtin/true.c',
 		'builtin/type.c',
+		'builtin/ulimit.c',
 		'builtin/umask.c',
 		'builtin/unalias.c',
 		'builtin/unset.c',
diff --git a/test/ulimit.sh b/test/ulimit.sh
new file mode 100644
index 0000000..b9fa03b
--- /dev/null
+++ b/test/ulimit.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+set -e
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits == "unlimited" ]
+if [ -e /proc/self/limits ]
+then
+grep "Max file size" /proc/self/limits | grep "unlimited"
+fi
+
+ulimit -f 100
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits -eq 100 ]
+if [ -e /proc/self/limits ]
+then
+grep "Max file size" /proc/self/limits | grep 51200
+fi
-- 
2.18.1

Re: [PATCH-v2] Implemented ulimit builtin

Details
Message ID
<20190104143536.GD1013@miku>
In-Reply-To
<20190104143119.4979-1-jpsamaroo@jpsamaroo.me> (view parent)
Sender timestamp
1546612536
DKIM signature
permerror
Download raw message
LGTM! We'll see what Simon thinks.

> +++ b/test/ulimit.sh
> -%<-
> +if [ -e /proc/self/limits ]
> +then
> +grep "Max file size" /proc/self/limits | grep "unlimited"

Style: indent this with a tab. Ditto for the other if

Re: [PATCH-v2] Implemented ulimit builtin

Details
Message ID
<L6o9f9_TmgCuJyMYN2CAqHVTJ1gj843I8-rnSYH8PLPkyC6mhcslvnuVDP7ft-7d-Qc7vO2cA1Tc79fkYxTP-Se3k8EmVCWIp577IDhD_Rs=@emersion.fr>
In-Reply-To
<20190104143119.4979-1-jpsamaroo@jpsamaroo.me> (view parent)
Sender timestamp
1546728993
DKIM signature
pass
Download raw message
Thanks for your patch! Here are a few comments below. This is looking pretty
good.

On Friday, January 4, 2019 3:31 PM, <jpsamaroo@jpsamaroo.me> wrote:
> +int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
> +	struct rlimit old, new;
> +	bool set_limit = false;
> +	long int new_limit;
> +	char *endptr;

Style: I'd prefer to declare these variables right before they're used.

> +	mrsh_optind = 1;
> +	int opt;
> +	if ((opt = mrsh_getopt(argc, argv, ":f")) != -1) {

This works since there's only one option, but is confusing and will break if
more are added. I think it makes sense to s/if/while/. This also catches
potential invalid flags.

> +		if (opt == 'f') {
> +			set_limit = true;
> +		} else {
> +			fprintf(stderr, "%s", ulimit_usage);
> +			return EXIT_FAILURE;
> +		}
> +	}
> +	if (set_limit && mrsh_optind != argc - 1) {

Instead of this, the options string passed to `mrsh_getopt` can be modified to
specify that -f takes an argument:

> if a character is followed by a <colon>, the option takes an argument

See getopt(3).

> +		fprintf(stderr, "%s", ulimit_usage);
> +		return EXIT_FAILURE;
> +	}
> +
> +	if (set_limit) {
> +		errno = 0;
> +		new_limit = strtol(argv[mrsh_optind], &endptr, 10);
> +		if ((errno == ERANGE &&
> +				(new_limit == LONG_MAX || new_limit == LONG_MIN))
> +				|| (errno != 0 && new_limit == 0)) {

Maybe we can just check for `errno != 0`, without the ERANGE stuff?

> +			fprintf(stderr, "strtol error: %s\n", strerror(errno));
> +			return EXIT_FAILURE;
> +		}
> +		if (endptr == argv[mrsh_optind]) {

Here we can check `endptr[0] == '\0'`. This makes sure the whole string has been
parsed, and that there isn't some garbage at the end (e.g. "42abc").

> +			fprintf(stderr, "ulimit error: Invalid argument: %s\n",
> +				argv[mrsh_optind]);
> +			return EXIT_FAILURE;
> +		}
> +		new.rlim_cur = new_limit * 512;
> +		new.rlim_max = new_limit * 512;

To make sure we always initialize all members, it's safer to use struct
initializers here:

    struct rlimit new = { .rlim_cur = …, .rlim_max = … };

> +		if (setrlimit(RLIMIT_FSIZE, &new) != 0) {
> +			fprintf(stderr, "setrlimit error: %s\n", strerror(errno));
> +			return EXIT_FAILURE;
> +		}
> +	} else {
> +		if (getrlimit(RLIMIT_FSIZE, &old) != 0) {
> +			fprintf(stderr, "getrlimit error: %s\n", strerror(errno));
> +			return EXIT_FAILURE;
> +		}
> +		if (old.rlim_max == RLIM_INFINITY) {
> +			printf("unlimited\n");
> +		} else {
> +			printf("%lu\n", old.rlim_max / 512);
> +		}
> +	}
> +	return EXIT_SUCCESS;
> +}

[PATCH v3] Implemented ulimit builtin

Details
Message ID
<20190106193804.21031-1-jpsamaroo@jpsamaroo.me>
In-Reply-To
<L6o9f9_TmgCuJyMYN2CAqHVTJ1gj843I8-rnSYH8PLPkyC6mhcslvnuVDP7ft-7d-Qc7vO2cA1Tc79fkYxTP-Se3k8EmVCWIp577IDhD_Rs=@emersion.fr> (view parent)
Sender timestamp
1546803484
DKIM signature
missing
Download raw message
Patch: +89 -0
From: Julian P Samaroo <jpsamaroo@jpsamaroo.me>

---
Addressed fixes from Drew and Simon's reviews, hopefully I didn't miss
anything.

 builtin/builtin.c |  1 +
 builtin/ulimit.c  | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 include/builtin.h |  1 +
 meson.build       |  1 +
 test/ulimit.sh    | 19 ++++++++++++++
 5 files changed, 89 insertions(+)
 create mode 100644 builtin/ulimit.c
 create mode 100644 test/ulimit.sh

diff --git a/builtin/builtin.c b/builtin/builtin.c
index 2692532..f54e95f 100644
--- a/builtin/builtin.c
@@ -31,6 +31,7 @@ static const struct builtin builtins[] = {
 	{ "times", builtin_times, true },
 	{ "true", builtin_true, false },
 	{ "type", builtin_type, false },
+	{ "ulimit", builtin_ulimit, false },
 	{ "umask", builtin_umask, false },
 	{ "unalias", builtin_unalias, false },
 	{ "unset", builtin_unset, true },
diff --git a/builtin/ulimit.c b/builtin/ulimit.c
new file mode 100644
index 0000000..f065189
--- /dev/null
@@ -0,0 +1,67 @@
+#define _POSIX_C_SOURCE 200809L
+#include <mrsh/getopt.h>
+#include <mrsh/shell.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+#include "builtin.h"
+
+static const char ulimit_usage[] = "usage: ulimit [-f blocks]\n";
+
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
+	bool set_limit = false;
+	long int new_limit;
+	mrsh_optind = 1;
+	int opt;
+	while ((opt = mrsh_getopt(argc, argv, ":f:")) != -1) {
+		if (opt == 'f') {
+			set_limit = true;
+			errno = 0;
+			char *endptr;
+			new_limit = strtol(mrsh_optarg, &endptr, 10);
+			if (errno != 0) {
+				fprintf(stderr, "strtol error: %s\n", strerror(errno));
+				return EXIT_FAILURE;
+			}
+			if ((endptr == mrsh_optarg) || (endptr[0] != '\0')) {
+				fprintf(stderr, "ulimit error: Invalid argument: %s\n",
+					mrsh_optarg);
+				return EXIT_FAILURE;
+			}
+		} else {
+			fprintf(stderr, "%s", ulimit_usage);
+			return EXIT_FAILURE;
+		}
+	}
+	if (mrsh_optind < argc) {
+		fprintf(stderr, "%s", ulimit_usage);
+		return EXIT_FAILURE;
+	}
+
+	if (set_limit) {
+		struct rlimit new = {
+			.rlim_cur = new_limit * 512,
+			.rlim_max = new_limit * 512
+		};
+		if (setrlimit(RLIMIT_FSIZE, &new) != 0) {
+			fprintf(stderr, "setrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+	} else {
+		struct rlimit old = { 0 };
+		if (getrlimit(RLIMIT_FSIZE, &old) != 0) {
+			fprintf(stderr, "getrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (old.rlim_max == RLIM_INFINITY) {
+			printf("unlimited\n");
+		} else {
+			printf("%lu\n", old.rlim_max / 512);
+		}
+	}
+	return EXIT_SUCCESS;
+}
diff --git a/include/builtin.h b/include/builtin.h
index ed437b2..afebdc4 100644
--- a/include/builtin.h
+++ b/include/builtin.h
@@ -24,6 +24,7 @@ int builtin_shift(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_times(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_true(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_type(struct mrsh_state *state, int argc, char *argv[]);
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
diff --git a/meson.build b/meson.build
index a51e623..b33c354 100644
--- a/meson.build
+++ b/meson.build
@@ -64,6 +64,7 @@ lib_mrsh = library(
 		'builtin/times.c',
 		'builtin/true.c',
 		'builtin/type.c',
+		'builtin/ulimit.c',
 		'builtin/umask.c',
 		'builtin/unalias.c',
 		'builtin/unset.c',
diff --git a/test/ulimit.sh b/test/ulimit.sh
new file mode 100644
index 0000000..70adffc
--- /dev/null
+++ b/test/ulimit.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+set -e
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits == "unlimited" ]
+if [ -e /proc/self/limits ]
+then
+	grep "Max file size" /proc/self/limits | grep "unlimited"
+fi
+
+ulimit -f 100
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits -eq 100 ]
+if [ -e /proc/self/limits ]
+then
+	grep "Max file size" /proc/self/limits | grep 51200
+fi
-- 
2.18.1

Re: [PATCH v3] Implemented ulimit builtin

Details
Message ID
<85Twto6-Njf2R3K2sQ0Ln9IlZ_eSmiZE9GgLrxVoZb5zviZxq0fea9aM6I0uEZuWZSlignvfneWugIjLYVO2l-vDLO_3teg-bEQ0yq7vrNg=@emersion.fr>
In-Reply-To
<20190106193804.21031-1-jpsamaroo@jpsamaroo.me> (view parent)
Sender timestamp
1546951949
DKIM signature
pass
Download raw message
On Sunday, January 6, 2019 8:38 PM, <jpsamaroo@jpsamaroo.me> wrote:
> +int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
> +	bool set_limit = false;
> +	long int new_limit;
> +	mrsh_optind = 1;
> +	int opt;
> +	while ((opt = mrsh_getopt(argc, argv, ":f:")) != -1) {
> +		if (opt == 'f') {

Hmm. Sorry, I think I've misread the spec. The synopsis says:

    ulimit [-f] [blocks]

Since `-f` is the default, it can just be a no-op. The option doesn't have a
value in the end, it's just a standalone flag.

> +			set_limit = true;
> +			errno = 0;
> +			char *endptr;
> +			new_limit = strtol(mrsh_optarg, &endptr, 10);
> +			if (errno != 0) {
> +				fprintf(stderr, "strtol error: %s\n", strerror(errno));
> +				return EXIT_FAILURE;
> +			}
> +			if ((endptr == mrsh_optarg) || (endptr[0] != '\0')) {
> +				fprintf(stderr, "ulimit error: Invalid argument: %s\n",
> +					mrsh_optarg);
> +				return EXIT_FAILURE;
> +			}
> +		} else {
> +			fprintf(stderr, "%s", ulimit_usage);
> +			return EXIT_FAILURE;
> +		}
> +	}
> +	if (mrsh_optind < argc) {
> +		fprintf(stderr, "%s", ulimit_usage);
> +		return EXIT_FAILURE;
> +	}

[…]

> diff --git a/test/ulimit.sh b/test/ulimit.sh
> new file mode 100644
> index 0000000..70adffc
> --- /dev/null
> +++ b/test/ulimit.sh
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +
> +set -e

Nitpick: this can be shortened to:

    #!/bin/sh -e

[PATCH v4] Implemented ulimit builtin

Details
Message ID
<20190108220006.3170-1-jpsamaroo@jpsamaroo.me>
In-Reply-To
<85Twto6-Njf2R3K2sQ0Ln9IlZ_eSmiZE9GgLrxVoZb5zviZxq0fea9aM6I0uEZuWZSlignvfneWugIjLYVO2l-vDLO_3teg-bEQ0yq7vrNg=@emersion.fr> (view parent)
Sender timestamp
1546984806
DKIM signature
missing
Download raw message
Patch: +88 -0
From: Julian P Samaroo <jpsamaroo@jpsamaroo.me>

---
 builtin/builtin.c |  1 +
 builtin/ulimit.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++++
 include/builtin.h |  1 +
 meson.build       |  1 +
 test/ulimit.sh    | 19 ++++++++++++++
 5 files changed, 88 insertions(+)
 create mode 100644 builtin/ulimit.c
 create mode 100644 test/ulimit.sh

diff --git a/builtin/builtin.c b/builtin/builtin.c
index 2692532..f54e95f 100644
--- a/builtin/builtin.c
@@ -31,6 +31,7 @@ static const struct builtin builtins[] = {
 	{ "times", builtin_times, true },
 	{ "true", builtin_true, false },
 	{ "type", builtin_type, false },
+	{ "ulimit", builtin_ulimit, false },
 	{ "umask", builtin_umask, false },
 	{ "unalias", builtin_unalias, false },
 	{ "unset", builtin_unset, true },
diff --git a/builtin/ulimit.c b/builtin/ulimit.c
new file mode 100644
index 0000000..f9573d9
--- /dev/null
@@ -0,0 +1,66 @@
+#define _POSIX_C_SOURCE 200809L
+#include <mrsh/getopt.h>
+#include <mrsh/shell.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+#include "builtin.h"
+
+static const char ulimit_usage[] = "usage: ulimit [-f] [blocks]\n";
+
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]) {
+	mrsh_optind = 1;
+	int opt;
+	while ((opt = mrsh_getopt(argc, argv, ":f")) != -1) {
+		if (opt == 'f') {
+			// Nothing here
+		} else {
+			fprintf(stderr, "%s", ulimit_usage);
+			return EXIT_FAILURE;
+		}
+	}
+
+	if (mrsh_optind == argc - 1) {
+		errno = 0;
+		char *arg = argv[mrsh_optind];
+		char *endptr;
+		long int new_limit = strtol(arg, &endptr, 10);
+		if (errno != 0) {
+			fprintf(stderr, "strtol error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if ((endptr == arg) || (endptr[0] != '\0')) {
+			fprintf(stderr, "ulimit error: Invalid argument: %s\n",
+				arg);
+			return EXIT_FAILURE;
+		}
+		struct rlimit new = {
+			.rlim_cur = new_limit * 512,
+			.rlim_max = new_limit * 512
+		};
+		if (setrlimit(RLIMIT_FSIZE, &new) != 0) {
+			fprintf(stderr, "setrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+	} else if (mrsh_optind == argc) {
+		struct rlimit old = { 0 };
+		if (getrlimit(RLIMIT_FSIZE, &old) != 0) {
+			fprintf(stderr, "getrlimit error: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		if (old.rlim_max == RLIM_INFINITY) {
+			printf("unlimited\n");
+		} else {
+			printf("%lu\n", old.rlim_max / 512);
+		}
+	} else {
+		fprintf(stderr, "%s", ulimit_usage);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
diff --git a/include/builtin.h b/include/builtin.h
index ed437b2..afebdc4 100644
--- a/include/builtin.h
+++ b/include/builtin.h
@@ -24,6 +24,7 @@ int builtin_shift(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_times(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_true(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_type(struct mrsh_state *state, int argc, char *argv[]);
+int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
diff --git a/meson.build b/meson.build
index a51e623..b33c354 100644
--- a/meson.build
+++ b/meson.build
@@ -64,6 +64,7 @@ lib_mrsh = library(
 		'builtin/times.c',
 		'builtin/true.c',
 		'builtin/type.c',
+		'builtin/ulimit.c',
 		'builtin/umask.c',
 		'builtin/unalias.c',
 		'builtin/unset.c',
diff --git a/test/ulimit.sh b/test/ulimit.sh
new file mode 100644
index 0000000..70adffc
--- /dev/null
+++ b/test/ulimit.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+set -e
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits == "unlimited" ]
+if [ -e /proc/self/limits ]
+then
+	grep "Max file size" /proc/self/limits | grep "unlimited"
+fi
+
+ulimit -f 100
+
+mrsh_limits=`ulimit`
+[ $mrsh_limits -eq 100 ]
+if [ -e /proc/self/limits ]
+then
+	grep "Max file size" /proc/self/limits | grep 51200
+fi
-- 
2.18.1

Re: [PATCH v4] Implemented ulimit builtin

Details
Message ID
<gQZ0ICAge2lLUYmYrpbtTA5r1-_hBGZnfgOPNInYfIbSk76AlvLhrl0D1lrWP_gBDUp8WpGxUSsGtpfGDC72otUSZcpqHjmHsUdpoN_yRMI=@emersion.fr>
In-Reply-To
<20190108220006.3170-1-jpsamaroo@jpsamaroo.me> (view parent)
Sender timestamp
1547032619
DKIM signature
pass
Download raw message
Thanks! This version LGTM, I pushed it with a minor edit to add test/ulimit.sh
to the meson.build file so that `ninja test` runs it.

To git.sr.ht:~emersion/mrsh
   aa7d536..41bcddb  master -> master

Re: [PATCH v4] Implemented ulimit builtin

Details
Message ID
<FMAq9SZ8SdyYtlK1FWlLMbBqhTeHlowjB_wZp4ueJh5rTxGUQGQRjK5Oh3PxE5zRJNO19yraWnRen1DIhSDM_7y0817u6ipX2iJgTv-xAwE=@emersion.fr>
In-Reply-To
<gQZ0ICAge2lLUYmYrpbtTA5r1-_hBGZnfgOPNInYfIbSk76AlvLhrl0D1lrWP_gBDUp8WpGxUSsGtpfGDC72otUSZcpqHjmHsUdpoN_yRMI=@emersion.fr> (view parent)
Sender timestamp
1547037998
DKIM signature
pass
Download raw message
FYI, I pushed two follow-up patches to fix issues uncovered by the CI:

- 1460f32e: fix ulimit printing (format string for rlim_t)
- 8f462054: make test/ulimit.sh POSIX-compliant (use = instead of ==)

In the future, we'll have lists.sr.ht trigger CI builds, but this isn't
possible yet.

1460f32e: https://git.sr.ht/%7Eemersion/mrsh/commit/1460f32eacf6bfeff222111366ab31b69a815d58
8f462054: https://git.sr.ht/%7Eemersion/mrsh/commit/8f4620540cfab66007c308bf39bc5ba78fc1d72c