[PATCH 1/7] zramctl: print all devices as a default action

[PATCH 1/7] zramctl: print all devices as a default action

[Sami Kerola]

This commit makes the command to display information about all zram
devices when the command is ran without any arguments.  Listing includes
the devices that are unused, which in this context means devices that
does not have --size specified, making them to be zero size.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index a26cd8e..04744fd 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -193,18 +193,18 @@ static int zram_set_strparm(struct zram *z, const char *attr, const char *str)
 }
 
 
-static int zram_used(struct zram *z)
+static int zram_used(struct zram *z, int find_unused)
 {
 	uint64_t size;
 	struct sysfs_cxt *sysfs = zram_get_sysfs(z);
 
-	if (sysfs &&
-	    sysfs_read_u64(sysfs, "disksize", &size) == 0 &&
-	    size > 0) {
-
+	if (sysfs && sysfs_read_u64(sysfs, "disksize", &size) == 0) {
+		if (find_unused && size == 0)
+			goto unused;
 		DBG(fprintf(stderr, "%s used", z->devname));
 		return 1;
 	}
+ unused:
 	DBG(fprintf(stderr, "%s unused", z->devname));
 	return 0;
 }
@@ -220,7 +220,7 @@ static struct zram *find_free_zram(void)
 		zram_set_devname(z, NULL, i);
 		if (!zram_exist(z))
 			break;
-		isfree = !zram_used(z);
+		isfree = !zram_used(z, 1);
 	}
 	if (!isfree) {
 		free_zram(z);
@@ -344,12 +344,11 @@ static void status(struct zram *z)
 	else {
 		size_t i;			/* list all used devices */
 		z = new_zram(NULL);
-
 		for (i = 0; ; i++) {
 			zram_set_devname(z, NULL, i);
 			if (!zram_exist(z))
 				break;
-			if (zram_used(z))
+			if (zram_used(z, 0))
 				fill_table_row(tb, z);
 		}
 		free_zram(z);
@@ -511,10 +510,12 @@ int main(int argc, char **argv)
 			columns[ncolumns++] = COL_STREAMS;
 			columns[ncolumns++] = COL_MOUNTPOINT;
 		}
-		if (optind < argc)
+		if (optind < argc) {
 			zram = new_zram(argv[optind++]);
-		status(zram);
-		free_zram(zram);
+			status(zram);
+			free_zram(zram);
+		} else
+			status(NULL);
 		break;
 	case A_RESET:
 		if (optind == argc)
-- 
2.0.3

[PATCH 2/7] docs: add details to zramctl --size option documentation

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.8 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
index 74240a1..a308233 100644
--- a/sys-utils/zramctl.8
+++ b/sys-utils/zramctl.8
@@ -68,8 +68,15 @@ can be changed only after a reset.
 .TP
 .BR \-s , " \-\-size " \fIsize
 Force the zram driver to reread the size of the file associated with
-the specified zram device
-
+the specified zram device.  When not specified the
+.I size
+unit is bytes.  The zram devices size is page size aligned.  When
+requested
+.I size
+does match with page size the device be large enough to hold the reqested
+.I size
+plus length to next page boundary.
+.IP
 The \fIsize\fR argument may be followed by the multiplicative suffixes KiB (=1024),
 MiB (=1024*1024), and so on for GiB, TiB, PiB, EiB, ZiB and YiB (the "iB"
 is optional, e.g., "K" has the same meaning as "KiB") or the suffixes
-- 
2.0.3

[PATCH 3/7] zramctl: mark --reset mutually exclusive with --algorithm and --streams

[Sami Kerola]

Reset is setting algorithm to lz4 and streams to 1.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index 04744fd..9a3d5d2 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -425,8 +425,10 @@ int main(int argc, char **argv)
 	};
 
 	static const ul_excl_t excl[] = {
+		{ 'a', 'r' },
 		{ 'f', 'o', 'r' },
 		{ 'o', 'r', 's' },
+		{ 'r', 't' },
 		{ 0 }
 	};
 	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
-- 
2.0.3

[PATCH 4/7] zramctl: fail status printout when device does not exist

[Sami Kerola]

Earlier.

$ zramctl zram313 ; echo $?
0

Now.

$ zramctl zram313 ; echo $?
zramctl: zram313: no such device
1

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index 9a3d5d2..85dc30c 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -229,7 +229,7 @@ static struct zram *find_free_zram(void)
 	return z;
 }
 
-static void fill_table_row(struct libscols_table *tb, struct zram *z)
+static int fill_table_row(struct libscols_table *tb, struct zram *z)
 {
 	static struct libscols_line *ln;
 	struct sysfs_cxt *sysfs;
@@ -243,7 +243,7 @@ static void fill_table_row(struct libscols_table *tb, struct zram *z)
 
 	sysfs = zram_get_sysfs(z);
 	if (!sysfs)
-		return;
+		return 1;
 
 	ln = scols_table_new_line(tb, NULL);
 	if (!ln)
@@ -316,12 +316,14 @@ static void fill_table_row(struct libscols_table *tb, struct zram *z)
 		if (str)
 			scols_line_refer_data(ln, i, str);
 	}
+	return 0;
 }
 
-static void status(struct zram *z)
+static int status(struct zram *z)
 {
 	struct libscols_table *tb;
 	size_t i;
+	int ret = 0;
 
 	scols_init_debug(0);
 
@@ -340,7 +342,7 @@ static void status(struct zram *z)
 	}
 
 	if (z)
-		fill_table_row(tb, z);		/* just one device specified */
+		ret = fill_table_row(tb, z);		/* just one device specified */
 	else {
 		size_t i;			/* list all used devices */
 		z = new_zram(NULL);
@@ -356,6 +358,7 @@ static void status(struct zram *z)
 
 	scols_print_table(tb);
 	scols_unref_table(tb);
+	return ret;
 }
 
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
@@ -513,8 +516,9 @@ int main(int argc, char **argv)
 			columns[ncolumns++] = COL_MOUNTPOINT;
 		}
 		if (optind < argc) {
-			zram = new_zram(argv[optind++]);
-			status(zram);
+			zram = new_zram(argv[optind]);
+			if (status(zram))
+				errx(EXIT_FAILURE, _("%s: no such device"), argv[optind]);
 			free_zram(zram);
 		} else
 			status(NULL);
-- 
2.0.3

[PATCH 5/7] zramctl: improve error message

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index 85dc30c..9c5b6ed 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -498,7 +498,7 @@ int main(int argc, char **argv)
 		act = find ? A_FINDONLY : A_STATUS;
 
 	if (act != A_RESET && optind + 1 < argc)
-		usage(stderr);		/* only --reset holds more devices */
+		errx(EXIT_FAILURE, _("only one <device> at a time is allowed"));
 
 	switch (act) {
 	case A_STATUS:
-- 
2.0.3

[PATCH 6/7] zramctl: fix mount point printout

[Sami Kerola]

See sample belo how a device mount point was printed without and with
full <device> path.  This change adds the mount point markup to the short
device name printout.

$ zramctl zram3
NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
zram3 lz4             1M   4K   62B    4K       1

$ zramctl /dev/zram3
NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
/dev/zram3 lz4             1M   4K   62B    4K       1 [SWAP]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/zramctl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
index 9c5b6ed..afbc0e8 100644
--- a/sys-utils/zramctl.c
+++ b/sys-utils/zramctl.c
@@ -294,7 +294,15 @@ static int fill_table_row(struct libscols_table *tb, struct zram *z)
 			char path[PATH_MAX] = { '\0' };
 			int fl;
 
-			check_mount_point(z->devname, &fl, path, sizeof(path));
+			if (!access(z->devname, F_OK))
+				check_mount_point(z->devname, &fl, path, sizeof(path));
+			else {
+				char *zdevpath;
+
+				xasprintf(&zdevpath, "/dev/%s", z->devname);
+				check_mount_point(zdevpath, &fl, path, sizeof(path));
+				free(zdevpath);
+			}
 			if (*path)
 				str = xstrdup(path);
 			break;
-- 
2.0.3

[PATCH 7/7] zramctl: add bash completion script

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 bash-completion/Makemodule.am |  3 +++
 bash-completion/zramctl       | 51 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 bash-completion/zramctl

diff --git a/bash-completion/Makemodule.am b/bash-completion/Makemodule.am
index 8d6431b..eafbedf 100644
--- a/bash-completion/Makemodule.am
+++ b/bash-completion/Makemodule.am
@@ -252,6 +252,9 @@ endif
 if BUILD_SETARCH
 dist_bashcompletion_DATA += bash-completion/setarch
 endif
+if BUILD_ZRAMCTL
+dist_bashcompletion_DATA += bash-completion/zramctl
+endif
 
 if BUILD_MESG
 dist_bashcompletion_DATA += bash-completion/mesg
diff --git a/bash-completion/zramctl b/bash-completion/zramctl
new file mode 100644
index 0000000..90ed608
--- /dev/null
+++ b/bash-completion/zramctl
@@ -0,0 +1,51 @@
+_zramctl_module()
+{
+	local cur prev OPTS
+	COMPREPLY=()
+	cur="${COMP_WORDS[COMP_CWORD]}"
+	prev="${COMP_WORDS[COMP_CWORD-1]}"
+	case $prev in
+		'-a'|'--algorithm')
+			COMPREPLY=( $(compgen -W "lzo lz4" -- $cur) )
+			return 0
+			;;
+		'-o'|'--output')
+			# FIXME: how to append to a string with compgen?
+			local OUTPUT
+			OUTPUT="NAME DISKSIZE DATA COMPR ALGORITHM STREAMS ZERO-PAGES TOTAL MOUNTPOINT"
+			compopt -o nospace
+			COMPREPLY=( $(compgen -W "$OUTPUT" -S ',' -- $cur) )
+			return 0
+			;;
+		'-s'|'--size')
+			COMPREPLY=( $(compgen -W "size" -- $cur) )
+			return 0
+			;;
+		'-t'|'--streams')
+			COMPREPLY=( $(compgen -W "number" -- $cur) )
+			return 0
+			;;
+	esac
+	case $cur in
+		-*)
+			OPTS="	--algorithm
+				--bytes
+				--find
+				--noheadings
+				--output
+				--raw
+				--reset
+				--size
+				--streams
+				--help
+				--version"
+			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
+			return 0
+			;;
+	esac
+	local IFS=$'\n'
+	compopt -o filenames
+	COMPREPLY=( $(compgen -f -- ${cur:-"/dev/zram"}) )
+	return 0
+}
+complete -F _zramctl_module zramctl
-- 
2.0.3

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 169 bytes --]

On Sun 03 Aug 2014 15:17:37 Sami Kerola wrote:
> -static int zram_used(struct zram *z)
> +static int zram_used(struct zram *z, int find_unused)

s/int/bool/ ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Sami Kerola]

On 3 August 2014 15:50, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sun 03 Aug 2014 15:17:37 Sami Kerola wrote:
>> -static int zram_used(struct zram *z)
>> +static int zram_used(struct zram *z, int find_unused)
>
> s/int/bool/ ?

There's only handful of boolean types in use in util-linux

util-linux> git grep -l 'bool '
lib/mbsalign.c
login-utils/su-common.c
sys-utils/hwclock-cmos.c
sys-utils/hwclock.c
sys-utils/nsenter.c
tests/ts/login/logindefs

And question about bool is asked before.

http://marc.info/?l=util-linux-ng&m=135817763904699&w=2

The question was never answered, but if I should guess the idea in
util-linux is same as what Linus said should be done in kernel.

https://lkml.org/lkml/2013/8/31/138

Please read the whole thread. It's full of good stuff.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Bernhard Voelker]

On 08/03/2014 04:17 PM, Sami Kerola wrote:
> diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
> index a26cd8e..04744fd 100644
> --- a/sys-utils/zramctl.c
> +++ b/sys-utils/zramctl.c
> @@ -193,18 +193,18 @@ static int zram_set_strparm(struct zram *z, const char *attr, const char *str)
>  }
>  
>  
> -static int zram_used(struct zram *z)
> +static int zram_used(struct zram *z, int find_unused)
>  {
>  	uint64_t size;
>  	struct sysfs_cxt *sysfs = zram_get_sysfs(z);
>  
> -	if (sysfs &&
> -	    sysfs_read_u64(sysfs, "disksize", &size) == 0 &&
> -	    size > 0) {
> -
> +	if (sysfs && sysfs_read_u64(sysfs, "disksize", &size) == 0) {
> +		if (find_unused && size == 0)
> +			goto unused;
>  		DBG(fprintf(stderr, "%s used", z->devname));
>  		return 1;
>  	}
> + unused:
>  	DBG(fprintf(stderr, "%s unused", z->devname));
>  	return 0;
>  }

Do we want to give the same message for both 'unused' and the case
when sysfs or disksize can't be determined?

And IMO the 'goto' here is a big gun for skipping 2 lines and a '}';
just reversing the condition would have done it, too.

Have a nice day,
Berny

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Sun 03 Aug 2014 21:50:33 Sami Kerola wrote:
> On 3 August 2014 15:50, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Sun 03 Aug 2014 15:17:37 Sami Kerola wrote:
> >> -static int zram_used(struct zram *z)
> >> +static int zram_used(struct zram *z, int find_unused)
> > 
> > s/int/bool/ ?
> 
> There's only handful of boolean types in use in util-linux

i think there should be more.  i find them more readable, clearer intent, and 
harder to screw up.  whereas passing around 0/1 everywhere gets funky (and i 
think the code looks funky already).

> util-linux> git grep -l 'bool '
> lib/mbsalign.c
> login-utils/su-common.c
> sys-utils/hwclock-cmos.c
> sys-utils/hwclock.c
> sys-utils/nsenter.c
> tests/ts/login/logindefs
> 
> And question about bool is asked before.
> 
> http://marc.info/?l=util-linux-ng&m=135817763904699&w=2

it's a little hard to tell if it was meant generally or that specific case.  
but if Karel prefers they not be used, then the codebase should reflect that.

> The question was never answered, but if I should guess the idea in
> util-linux is same as what Linus said should be done in kernel.
> 
> https://lkml.org/lkml/2013/8/31/138
> 
> Please read the whole thread. It's full of good stuff.

i'm not really seeing anything applicable to us.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Karel Zak]

On Sun, Aug 03, 2014 at 03:17:37PM +0100, Sami Kerola wrote:
> This commit makes the command to display information about all zram
> devices when the command is ran without any arguments.  Listing includes

 I don't think it's a good idea do it by default. The current zramctl
 follows the logic we use for losetup (or lsblk), it means "list all
 used devices". I'm not sure if we really need to list empty devices,
 such devices are /dev nodes only. Maybe we can add --all if you have
 any use-case for this feature.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 6/7] zramctl: fix mount point printout

[Karel Zak]

On Sun, Aug 03, 2014 at 03:17:42PM +0100, Sami Kerola wrote:
> See sample belo how a device mount point was printed without and with
> full <device> path.  This change adds the mount point markup to the short
> device name printout.
> 
> $ zramctl zram3
> NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
> zram3 lz4             1M   4K   62B    4K       1

 This is bug, it would be better improve zram_set_devname() to make it
 more robust and always convert the path to the full path. I don't think
 that any variability makes it more easy to use.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Sami Kerola]

On 4 August 2014 13:02, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Aug 03, 2014 at 03:17:37PM +0100, Sami Kerola wrote:
>> This commit makes the command to display information about all zram
>> devices when the command is ran without any arguments.  Listing includes
>
>  I don't think it's a good idea do it by default. The current zramctl
>  follows the logic we use for losetup (or lsblk), it means "list all
>  used devices". I'm not sure if we really need to list empty devices,
>  such devices are /dev nodes only. Maybe we can add --all if you have
>  any use-case for this feature.

Sounds reasonable. I'll get rid of the commit by the time of next submission.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 6/7] zramctl: fix mount point printout

[Sami Kerola]

On 4 August 2014 13:15, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Aug 03, 2014 at 03:17:42PM +0100, Sami Kerola wrote:
>> See sample belo how a device mount point was printed without and with
>> full <device> path.  This change adds the mount point markup to the short
>> device name printout.
>>
>> $ zramctl zram3
>> NAME  ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
>> zram3 lz4             1M   4K   62B    4K       1
>
>  This is bug, it would be better improve zram_set_devname() to make it
>  more robust and always convert the path to the full path. I don't think
>  that any variability makes it more easy to use.

The following make device path to be always absolute.

https://github.com/kerolasa/lelux-utiliteetit/commit/5a093e4d11091c48580531fa88d1c64b564b57d3

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Karel Zak]

On Mon, Aug 04, 2014 at 03:31:13AM -0400, Mike Frysinger wrote:
> On Sun 03 Aug 2014 21:50:33 Sami Kerola wrote:
> i think there should be more.  i find them more readable, clearer intent, and 
> harder to screw up.  whereas passing around 0/1 everywhere gets funky (and i 
> think the code looks funky already).
> 
> > util-linux> git grep -l 'bool '
> > lib/mbsalign.c
> > login-utils/su-common.c
> > sys-utils/hwclock-cmos.c
> > sys-utils/hwclock.c
> > sys-utils/nsenter.c
> > tests/ts/login/logindefs
> > 
> > And question about bool is asked before.
> > 
> > http://marc.info/?l=util-linux-ng&m=135817763904699&w=2
> 
> it's a little hard to tell if it was meant generally or that specific case.  
> but if Karel prefers they not be used, then the codebase should reflect that.

I don't have strong opinion against 'bool', but I personally don't use
it. Well, exception are public library APIs, where the bool type is
mistake IMHO. 

The 'bool' type is gray C zone in some cases.

        slcurses.h:#define bool int
        python2.7/asdl.h:typedef enum {false, true} bool;

        tirpc/rpc/types.h:typedef int32_t bool_t;
        ncurses.h:#define bool NCURSES_BOOL
        notmuch.h:typedef int notmuch_bool_t;

 .. or bool, true and false are macros and bool expands to build-in C99 
 _Bool type or it's keyword in C++, etc.


For me "func(int isbar)" has the same readability as "func(bool bar)"
and in structs is always better to use bit fields (arrays).

> > The question was never answered, but if I should guess the idea in
> > util-linux is same as what Linus said should be done in kernel.
> > 
> > https://lkml.org/lkml/2013/8/31/138

Thanks, I have never read this, but he is right :-)

It seems that it's nothing unusual that people fight with bool in C:
http://www.lysator.liu.se/c/c-faq/c-8.html
http://stackoverflow.com/questions/25461/interfacing-with-stdbool-h-c
https://www.gnu.org/software/gnulib/manual/html_node/stdbool_002eh.html

And I don't think that codebase should reflect all my opinions :-)

Note, please, don't send patches that only remove/add bool -- common
sense is better than rules, if you believe that bool is the best thing
for your code than use it (if you cannot use bit fields, bool in
structs will be rejected). I'm going to use 'int' in my code.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/7] zramctl: print all devices as a default action

[Karel Zak]

On Sun, Aug 03, 2014 at 03:17:37PM +0100, Sami Kerola wrote:
>  sys-utils/zramctl.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

 Merged all set from github with some changes.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com