[RFC] Zram handle util

[RFC] Zram handle util

[Timofey Titovets]

Good time of day.
I want hear your opinions.
Several days ago, i writed zramctl util (on C++) for have useful app for 
setup zrams..
Code hosted her: https://github.com/Nefelim4ag/zramctl

Examples:
zramctl find # return name of first free device (zram0)
zramctl find 1024M lz4 4 # (i not implement -? options now)
			 # 4 - threads
			 # setup and return name of device (zram1)
zramctl reset zram0 zram1 ...


I want to port it on C and try include it in util-linux, like losetup.
It has a sense? What do you think?

Re: [RFC] Zram handle util

[Sami Kerola]

On 19 July 2014 18:28, Timofey Titovets <nefelim4ag@gmail.com> wrote:

Hi Timofey,

I'm not the util-linux maintainer, but since I've done few
contributions perhaps I can give couple hints how to proceed.

> I want hear your opinions.
> Several days ago, i writed zramctl util (on C++) for have useful app for
> setup zrams..

Sounds potentially useful.

> Code hosted her: https://github.com/Nefelim4ag/zramctl
>
> Examples:
> zramctl find # return name of first free device (zram0)
> zramctl find 1024M lz4 4 # (i not implement -? options now)
>                          # 4 - threads
>                          # setup and return name of device (zram1)
> zramctl reset zram0 zram1 ...
>
> I want to port it on C and try include it in util-linux, like losetup.
> It has a sense? What do you think?

There are all sorts of small things each command in this package is
expected to do. Good way to get introduced with them is to take the
sample code below, and modify it to be the new command.

https://github.com/karelzak/util-linux/blob/master/Documentation/boilerplate.c

But before getting too excited about writing new tool it might be
worth while to investigate related discussion.

https://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg16342.html

By glance it seems a good utility would be able to integrate with udev
and systemd, it should have add, remove, and possibly modify method. I
would prefer the command to list what is current status of zram
devices when nothing else is instructed. And of course manual page is
needed. See util-linux/Documentation/howto-man-page.txt file to get
manual done.

Without any investigation I have no idea what should be done with
systemd integration. The zramctl might need a systemd service file,
and possibly some sort of configuration file to setup the devices at
boot desired way. Or something totally different. I am sure there is
someone who knows what should be done, and with a bit of luck the mail
archive discussion I pointed out has an answer to this already.

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

Fwd: [RFC] Zram handle util

[Timofey Titovets]

Thanks for comment and boilerplate.c file.

I think it can't be related to systemd or udev, because as i know zram
configure and using with using many different scripts or through udev
config files. May be in future i open discuss in systemd-devel about
zramctl but now, i try to close several things in systemd todo, like
readahead optimization.

> By glance it seems a good utility would be able to integrate with udev
> and systemd, it should have add, remove, and possibly modify method. I
> would prefer the command to list what is current status of zram
> devices when nothing else is instructed. And of course manual page is
> needed. See util-linux/Documentation/howto-man-page.txt file to get
> manual done.

Also, kernel can't add or delete zram devices on demand, it hard coded
in module. Yes, i can reload module every time, but it awkward.
I write patch for correct this https://lkml.org/lkml/2014/7/17/664 ,
but now it not merged(and will it?).

I just want to make all acceptable generic solution (i can configure
loop devices through sysfs, but i have losetup...).

zramctl have status command:
$ zramctl status
 NAME     DISKSIZE       ORIG    COMPRES  ALG  THR
 zram0  1073741824     401408              8101   lz4   4
 zram1  1073741824              0                   0   lz4   4

> Without any investigation I have no idea what should be done with
> systemd integration. The zramctl might need a systemd service file,
> and possibly some sort of configuration file to setup the devices at
> boot desired way. Or something totally different. I am sure there is
> someone who knows what should be done, and with a bit of luck the mail
> archive discussion I pointed out has an answer to this already.
> --
> Sami Kerola
> http://www.iki.fi/kerolasa/

I also have custom systemd-swap script (handle swap device, zram
swaps, file swaps), what depend on zramctl, and i think if systemd
maintainers will want to expand various swap setting inside systemd, i
implement can it.

--
Best regards,
Timofey.

Fwd: [RFC] Zram handle util

[Timofey Titovets]

Thanks for comment and boilerplate.c file.

I think it can't be related to systemd or udev, because as i know zram
configure and using with using many different scripts or through udev
config files. May be in future i open discuss in systemd-devel about
zramctl but now, i try to close several things in systemd todo, like
readahead optimization.

> By glance it seems a good utility would be able to integrate with udev
> and systemd, it should have add, remove, and possibly modify method. I
> would prefer the command to list what is current status of zram
> devices when nothing else is instructed. And of course manual page is
> needed. See util-linux/Documentation/howto-man-page.txt file to get
> manual done.

Also, kernel can't add or delete zram devices on demand, it hard coded
in module. Yes, i can reload module every time, but it awkward.
I write patch for correct this https://lkml.org/lkml/2014/7/17/664 ,
but now it not merged(and will it?).

I just want to make all acceptable generic solution (i can configure
loop devices through sysfs, but i have losetup...).

zramctl have status command:
$ zramctl status
 NAME     DISKSIZE       ORIG    COMPRES  ALG  THR
 zram0  1073741824     401408              8101   lz4   4
 zram1  1073741824              0                   0   lz4   4

> Without any investigation I have no idea what should be done with
> systemd integration. The zramctl might need a systemd service file,
> and possibly some sort of configuration file to setup the devices at
> boot desired way. Or something totally different. I am sure there is
> someone who knows what should be done, and with a bit of luck the mail
> archive discussion I pointed out has an answer to this already.
> --
> Sami Kerola
> http://www.iki.fi/kerolasa/

I also have custom systemd-swap script (handle swap device, zram
swaps, file swaps), what depend on zramctl, and i think if systemd
maintainers will want to expand various swap setting inside systemd, i
implement can it.

--
Best regards,
Timofey.

Re: [RFC] Zram handle util

[Davidlohr Bueso]

On Sat, 2014-07-19 at 20:28 +0300, Timofey Titovets wrote:
> Good time of day.
> I want hear your opinions.
> Several days ago, i writed zramctl util (on C++) for have useful app for 
> setup zrams..
> Code hosted her: https://github.com/Nefelim4ag/zramctl
> 
> Examples:
> zramctl find # return name of first free device (zram0)
> zramctl find 1024M lz4 4 # (i not implement -? options now)
> 			 # 4 - threads
> 			 # setup and return name of device (zram1)
> zramctl reset zram0 zram1 ...
> 
> 
> I want to port it on C and try include it in util-linux, like losetup.
> It has a sense? What do you think?

Minchan, any thoughts in keeping this sort of utility in the kernel
tree? Perhaps tools/zram/? No idea if the code is suitable to consider
right away, but it kinda makes sense.

Thanks,
Davidlohr

Re: [RFC] Zram handle util

[Minchan Kim]

On Sun, Jul 20, 2014 at 01:36:48PM -0700, Davidlohr Bueso wrote:
> On Sat, 2014-07-19 at 20:28 +0300, Timofey Titovets wrote:
> > Good time of day.
> > I want hear your opinions.
> > Several days ago, i writed zramctl util (on C++) for have useful app for 
> > setup zrams..
> > Code hosted her: https://github.com/Nefelim4ag/zramctl
> > 
> > Examples:
> > zramctl find # return name of first free device (zram0)
> > zramctl find 1024M lz4 4 # (i not implement -? options now)
> > 			 # 4 - threads
> > 			 # setup and return name of device (zram1)
> > zramctl reset zram0 zram1 ...
> > 
> > 
> > I want to port it on C and try include it in util-linux, like losetup.
> > It has a sense? What do you think?
> 
> Minchan, any thoughts in keeping this sort of utility in the kernel
> tree? Perhaps tools/zram/? No idea if the code is suitable to consider
> right away, but it kinda makes sense.

First of all, Thanks for your effor, Timofey!

Adding such userspace tool in kernel src directory doesn't make sense.
If we wanted it, there are tons of user tools in kernel src directory
so IMHO, util-linux is better place.

Thanks.

> 
> Thanks,
> Davidlohr
> 

Re: [RFC] Zram handle util

[Karel Zak]

On Mon, Jul 21, 2014 at 07:57:21AM +0000, Minchan Kim wrote:
> First of all, Thanks for your effor, Timofey!
> 
> Adding such userspace tool in kernel src directory doesn't make sense.
> If we wanted it, there are tons of user tools in kernel src directory
> so IMHO, util-linux is better place.

 I don't think that regular (non-debugging, non-development) userspace utils 
 belong to the kernel tree...

 Please, see the howtos recommended by Sami and send a patch(es) with
 the code and a man page. It (util-linux specific details) does not
 have to be perfect, the important is to have correctly implemented
 zram specific things.

    Karel

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

[RFC] [Patch] Created zramctl

[Timofey Titovets]

Good time of day,
I spend some time and rewrite zramctl for util-linux.

Please review code and man pages, i think what i can do it better.
If you have any suggestion, say it out.

Can be pulled from:
https://github.com/Nefelim4ag/util-linux

For detail see man pages zramctl.8

----
  sys-utils/Makemodule.am |   5 +
  sys-utils/zramctl.8     |  92 ++++++++++++++
  sys-utils/zramctl.c     | 319 
++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 416 insertions(+)

diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 68fd030..5d70c51 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -172,6 +172,11 @@ eject_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
  dist_man_MANS += sys-utils/eject.1
  endif

+sbin_PROGRAMS += zramctl
+dist_man_MANS += sys-utils/zramctl.8
+zramctl_SOURCES = sys-utils/zramctl.c
+zramctl_LDADD = $(LDADD) libcommon.la libsmartcols.la
+zramctl_CFLAGS = $(AM_CFLAGS) -I$(ul_libsmartcols_incdir)

  if BUILD_LOSETUP
  sbin_PROGRAMS += losetup
diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
new file mode 100644
index 0000000..4da209d
--- /dev/null
+++ b/sys-utils/zramctl.8
@@ -0,0 +1,92 @@
+.TH ZRAMCTL 8 "July 2014" "util-linux" "System Administration"
+.SH NAME
+zramctl \- set up and control zram devices
+.SH SYNOPSIS
+.ad l
+Get info:
+.sp
+.in +5
+.B zramctl
+.sp
+.in -5
+Reset zram:
+.sp
+.in +5
+.B "zramctl \-r"
+.IR zramdev
+.sp
+.in -5
+Print name of first unused zram device:
+.sp
+.in +5
+.B "zramctl \-f"
+.sp
+.in -5
+Setup zram device:
+.sp
+.in +5
+.B zramctl
+.RB { \-f\ [ \-d\ \fIzramdev\fP] }
+.RB [ \-s
+.IR size ]
+.RB \ [ \-t
+.IR number ]
+.in +8
+.RB [ \-a
+.IR algorithm ]
+.sp
+.in -13
+.ad b
+.SH DESCRIPTION
+.B zramctl
+is used to fast setup zram devices parametrs, to reset zram devices and to
+query the status of a used zram devices.
+If no option is given, all zram devices are shown.
+
+
+.SH OPTIONS
+
+.IP "\fB\-s, \-\-size\fP \fIsize\fP
+force zram driver to reread size of the file associated with the 
specified zram device
++The \fIsize\fR arguments may be followed by the multiplicative
++suffixes 128K 512M, 1G.
+.IP "\fB\-r, \-\-reset\fP \fIzramdev\fP"
+Reset options specified zram device(s). Zram device setting can be 
changed only
+after reset.
+.IP "\fB\-f, \-\-find\fP"
+find the first unused zram device. If a
+.R \-s \fIsize\fR
+argument is present, use this device.
+.IP "\fB\-h, \-\-help\fP"
+print help
+.IP "\fB\-t, \-\-threads \fInumber\fP"
+Set number of maximum compress streams what used for device.
+.IP "\fB\-a, \-\-alg \fI{lzo|lz4}\fP""
+Set compress algorithm used for compress data in zram device.
+
+.SH RETURN VALUE
+.B zramctl
+returns 0 on success, nonzero on failure.
+
+.SH FILES
+.TP
+.I /dev/zram[0..N]
+zram block devices
+
+.SH EXAMPLE
+The following commands can be used as an example of using the zram device.
+.nf
+.IP
+# zramctl --find --size 1024M
+/dev/zram0
+# mkswap /dev/zram0
+# swapon /dev/zram0
+ ...
+# swapoff /dev/zram0
+# zramctl --reset /dev/zram0
+.fi
+.SH AUTHORS
+Timofey Titovets <nefelim4ag@gmail.com>
+.SH AVAILABILITY
+The zramctl command is part of the util-linux package and is available from
+ftp://ftp.kernel.org/pub/linux/utils/util-linux/.
diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
new file mode 100644
index 0000000..6ddee9b
--- /dev/null
+++ b/sys-utils/zramctl.c
@@ -0,0 +1,319 @@
+/*
+ * zramctl - purpose of it
+ *
+ * Copyright (c) 20nn  Example Commercial, Inc
+ * Written by Timofey Titovets <Nefelim4ag@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <getopt.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include "c.h"
+#include "closestream.h"
+#include "nls.h"
+
+static inline int zram_exist(char *name)
+{
+	struct stat info;
+	char path[16] = "/dev/";
+	strncat(path, name, 8);
+	if(stat(path, &info) != 0 )
+		return 0;
+	if (info.st_mode&S_IFBLK)
+		return 1;
+	return 0;
+}
+
+static inline int read_file(char *path, char *data)
+{
+	FILE *file;
+	unsigned i = 0;
+	char ch = EOF;
+	char *p;
+	p = path; // hack for work fopen
+	file = fopen(p, "r");
+	if (file == NULL) {
+		fprintf(stderr,_("can't open !%s!\n"), path);
+		return -1;
+	}
+
+	while (1) {
+		ch = fgetc(file);
+		if (ch == EOF || ch == '\n')
+			break;
+		else
+			data[i]=ch;
+		i++;
+	}
+	fclose(file);
+
+	return 0;
+}
+
+static inline int used(char *name)
+{
+	char path_to_disksize[32] = "/sys/block/";
+	char disksize[32]="";
+	strncat(path_to_disksize, name, 8);
+	strncat(path_to_disksize, "/disksize", 9);
+	if(read_file(path_to_disksize, disksize) < 0)
+		return -1;
+
+	if (disksize[0] == '0')
+		return 0;
+
+	return 1;
+}
+
+static inline int get_value(char *name, char *data, char *filename)
+{
+	char path[48] = "/sys/block/";
+	strncat(path, name, 8);
+	strncat(path,"/", 2);
+	strncat(path, filename, 32);
+	return read_file(path, data);
+}
+
+static inline int write_file(char *path, char *data)
+{
+	FILE *file;
+	char *p = path;
+	file = fopen(p, "w");
+	if (file == NULL) {
+		fprintf(stderr,_("can't write to %s\n"), path);
+		return -1;
+	}
+	fwrite(data, strlen(data), 1, file);
+	if (fclose(file) != 0)
+		return -1;
+	return 1;
+}
+
+static inline int value2devparm(char *name, char *data, char *filename)
+{
+	char path[48] = "/sys/block/";
+	strncat(path, name, 8);
+	strncat(path,"/", 2);
+	strncat(path, filename, 32);
+	return write_file(path, data);
+}
+
+static inline int status(void)
+{
+	char disksize[32]="";
+	char orig_data_size[32]="";
+	char compr_data_size[32]="";
+	char comp_algorithm[6]="";
+	char max_comp_streams[12]="";
+	unsigned i;
+	char *table = "%6s %12s %10s %10s %4s %4s \n";
+	printf(table, "NAME","DISKSIZE","ORIG","COMPRES","ALG","THR");
+	for (i=0;;i++) {
+		char name[8] = "zram";
+		char num[4];
+		char *pos;
+		sprintf(num,"%i",i);
+		strncat(name, num, 8);
+		if(!zram_exist(name))
+			break;
+		if(!used(name))
+			continue;
+		get_value(name, disksize, "disksize");
+		get_value(name, orig_data_size, "orig_data_size");
+		get_value(name, compr_data_size, "compr_data_size");
+		get_value(name, comp_algorithm, "comp_algorithm");
+		pos = strstr(comp_algorithm, "[lzo]");
+		if (pos == NULL) {
+			pos = strstr(comp_algorithm, "[lz4]");
+			if (pos == NULL)
+				strncpy(comp_algorithm,"-", 2);
+			else
+				strncpy(comp_algorithm, "lz4", 4);
+		} else
+			strncpy(comp_algorithm, "lzo", 4);
+		get_value(name, max_comp_streams, "max_comp_streams");
+		printf(table,
+			name,
+			disksize,
+			orig_data_size,
+			compr_data_size,
+			comp_algorithm,
+			max_comp_streams
+		      );
+	}
+
+	return 0;
+	goto fail;
+fail:
+	printf("Zram module not loaded");
+	return -1;
+}
+
+static inline char *find(void)
+{
+	char *ret = NULL;
+	for (unsigned i=0;;i++) {
+		char name[8] = "zram";
+		char num[4];
+
+		sprintf(num,"%i",i);
+		strncat(name, num, 4);
+		if (!zram_exist(name))
+			break;
+		else	//if enough one dir founded -> module loaded
+			ret = "USED";
+
+		if (used(name) == 0) {
+			ret = name;
+			break;
+		}
+	}
+
+		return ret;
+}
+
+
+
+static inline void usage(FILE *out)
+{
+	fputs(USAGE_HEADER, out);
+	fprintf(out, _(" %s [-d zramX|-f] -s 64M -a lz4 -t 16\n"), "zramctl");
+	fputs(USAGE_OPTIONS, out);
+	fputs(_(" <no args>             return status of used devices\n"), out);
+	fputs(_(" -f, --find            find free device\n"), out);
+	fputs(_(" -d, --device [name]   specify device: zramX\n"), out);
+	fputs(_(" -r, --reset  [name]   reset specified device\n"), out);
+	fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);
+	fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);
+	fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);
+	fputs(USAGE_SEPARATOR, out);
+	fputs(USAGE_HELP, out);
+	fputs(USAGE_VERSION, out);
+	fprintf(out, USAGE_MAN_TAIL("zramctl(8)"));
+	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
+}
+
+int main(int argc, char **argv)
+{
+	int c = 0;
+	char *dev = NULL;
+	char *size = NULL;
+	char *alg = NULL;
+	char threads[5] = "1";
+	unsigned f = 0;
+
+	static const struct option longopts[] = {
+		{"find", no_argument, NULL, 'f'},
+		{"device", required_argument, NULL, 'd'},
+		{"size", required_argument, NULL, 's'},
+		{"alg", required_argument, NULL, 'a'},
+		{"threads", required_argument, NULL, 't'},
+		{"reset", required_argument, NULL, 'r'},
+		{"version", no_argument, NULL, 'V'},
+		{"help", no_argument, NULL, 'h'},
+		{NULL, 0, NULL, 0}
+	};
+
+	setlocale(LC_ALL, "");
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	while ((c = getopt_long(argc, argv, "fd:s:a:t:r:Vh", longopts, NULL)) 
!= -1) {
+		switch (c) {
+		case 'f':
+			if (dev != NULL) {
+				fprintf(stderr,_("-f || -d <name>\n"));
+				return 1;
+			}
+			f = 1;
+			dev = find();
+			break;
+		case 'd':
+			if (dev != NULL) {
+				fprintf(stderr,_("-f || -d <name>\n"));
+				return 1;
+			}
+			dev = optarg;
+			break;
+		case 's':
+			size = optarg;
+			break;
+		case 'a':
+			if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
+				alg = optarg;
+			else {
+				fprintf(stderr,_("%s != lzo|lz4\n"),alg );
+				return 1;
+			}
+			break;
+		case 't':
+			strncpy(threads, optarg, 5);
+			if (atoi(threads) < 1) {
+				usage(stderr);
+				return 1;
+			}
+			break;
+		case 'r':
+			dev = optarg;
+			if (value2devparm(dev, "1", "reset") < 0)
+				return 1;
+			break;
+		case 'V':
+			printf(UTIL_LINUX_VERSION);
+			return EXIT_SUCCESS;
+		case 'h':
+			usage(stdout);
+		default:
+			usage(stderr);
+		}
+	}
+	if (argc == 1) {
+		status();
+		return EXIT_SUCCESS;
+	}
+
+	if (!strcmp(dev, "USED")) {
+		fprintf(stderr,_("all device already used\n") );
+		return 1;
+	}
+
+	if (dev != NULL && size != NULL) {
+		if (value2devparm(dev, "1", "reset") < 0)
+			return 1;
+		if (atoi(threads) > 1)
+
+			if (value2devparm(dev, threads, "max_comp_streams") < 0)
+				return 1;
+		if (alg != NULL)
+			if (value2devparm(dev, alg, "comp_algorithm") < 0)
+				return 1;
+
+		if (value2devparm(dev, size, "disksize") < 0)
+			return 1;
+	} else {
+	}
+
+	if (dev != NULL && f > 0)
+		fprintf(stdout,_("%s\n"), dev);
+
+	return EXIT_SUCCESS;
+}
\ No newline at end of file

Re: [RFC] [Patch] Created zramctl

[Dave Reisner]

On Wed, Jul 23, 2014 at 08:28:44PM +0300, Timofey Titovets wrote:
> Good time of day,
> I spend some time and rewrite zramctl for util-linux.
> 
> Please review code and man pages, i think what i can do it better.
> If you have any suggestion, say it out.
> 
> Can be pulled from:
> https://github.com/Nefelim4ag/util-linux
> 
> For detail see man pages zramctl.8
> 

I have a general fear of your repeated usage of strcpy-ish functions
into fixed size buffers and lack of error reporting...

I've left a few specific comments inline. I'm sure there's other things
that need pointing out.

> ----
>  sys-utils/Makemodule.am |   5 +
>  sys-utils/zramctl.8     |  92 ++++++++++++++
>  sys-utils/zramctl.c     | 319
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 416 insertions(+)
> 
> diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
> index 68fd030..5d70c51 100644
> --- a/sys-utils/Makemodule.am
> +++ b/sys-utils/Makemodule.am
> @@ -172,6 +172,11 @@ eject_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
>  dist_man_MANS += sys-utils/eject.1
>  endif
> 
> +sbin_PROGRAMS += zramctl
> +dist_man_MANS += sys-utils/zramctl.8
> +zramctl_SOURCES = sys-utils/zramctl.c
> +zramctl_LDADD = $(LDADD) libcommon.la libsmartcols.la
> +zramctl_CFLAGS = $(AM_CFLAGS) -I$(ul_libsmartcols_incdir)
> 
>  if BUILD_LOSETUP
>  sbin_PROGRAMS += losetup
> diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
> new file mode 100644
> index 0000000..4da209d
> --- /dev/null
> +++ b/sys-utils/zramctl.8
> @@ -0,0 +1,92 @@
> +.TH ZRAMCTL 8 "July 2014" "util-linux" "System Administration"
> +.SH NAME
> +zramctl \- set up and control zram devices
> +.SH SYNOPSIS
> +.ad l
> +Get info:
> +.sp
> +.in +5
> +.B zramctl
> +.sp
> +.in -5
> +Reset zram:
> +.sp
> +.in +5
> +.B "zramctl \-r"
> +.IR zramdev
> +.sp
> +.in -5
> +Print name of first unused zram device:
> +.sp
> +.in +5
> +.B "zramctl \-f"
> +.sp
> +.in -5
> +Setup zram device:
> +.sp
> +.in +5
> +.B zramctl
> +.RB { \-f\ [ \-d\ \fIzramdev\fP] }
> +.RB [ \-s
> +.IR size ]
> +.RB \ [ \-t
> +.IR number ]
> +.in +8
> +.RB [ \-a
> +.IR algorithm ]
> +.sp
> +.in -13
> +.ad b
> +.SH DESCRIPTION
> +.B zramctl
> +is used to fast setup zram devices parametrs, to reset zram devices and to
> +query the status of a used zram devices.
> +If no option is given, all zram devices are shown.
> +
> +
> +.SH OPTIONS
> +
> +.IP "\fB\-s, \-\-size\fP \fIsize\fP
> +force zram driver to reread size of the file associated with the specified
> zram device
> ++The \fIsize\fR arguments may be followed by the multiplicative
> ++suffixes 128K 512M, 1G.
> +.IP "\fB\-r, \-\-reset\fP \fIzramdev\fP"
> +Reset options specified zram device(s). Zram device setting can be changed
> only
> +after reset.
> +.IP "\fB\-f, \-\-find\fP"
> +find the first unused zram device. If a
> +.R \-s \fIsize\fR
> +argument is present, use this device.
> +.IP "\fB\-h, \-\-help\fP"
> +print help
> +.IP "\fB\-t, \-\-threads \fInumber\fP"
> +Set number of maximum compress streams what used for device.
> +.IP "\fB\-a, \-\-alg \fI{lzo|lz4}\fP""
> +Set compress algorithm used for compress data in zram device.
> +
> +.SH RETURN VALUE
> +.B zramctl
> +returns 0 on success, nonzero on failure.
> +
> +.SH FILES
> +.TP
> +.I /dev/zram[0..N]
> +zram block devices
> +
> +.SH EXAMPLE
> +The following commands can be used as an example of using the zram device.
> +.nf
> +.IP
> +# zramctl --find --size 1024M
> +/dev/zram0
> +# mkswap /dev/zram0
> +# swapon /dev/zram0
> + ...
> +# swapoff /dev/zram0
> +# zramctl --reset /dev/zram0
> +.fi
> +.SH AUTHORS
> +Timofey Titovets <nefelim4ag@gmail.com>
> +.SH AVAILABILITY
> +The zramctl command is part of the util-linux package and is available from
> +ftp://ftp.kernel.org/pub/linux/utils/util-linux/.
> diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
> new file mode 100644
> index 0000000..6ddee9b
> --- /dev/null
> +++ b/sys-utils/zramctl.c
> @@ -0,0 +1,319 @@
> +/*
> + * zramctl - purpose of it
> + *
> + * Copyright (c) 20nn  Example Commercial, Inc
> + * Written by Timofey Titovets <Nefelim4ag@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <getopt.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "c.h"
> +#include "closestream.h"
> +#include "nls.h"
> +
> +static inline int zram_exist(char *name)
> +{
> +	struct stat info;
> +	char path[16] = "/dev/";
> +	strncat(path, name, 8);
> +	if(stat(path, &info) != 0 )
> +		return 0;
> +	if (info.st_mode&S_IFBLK)
> +		return 1;
> +	return 0;
> +}
> +
> +static inline int read_file(char *path, char *data)
> +{
> +	FILE *file;
> +	unsigned i = 0;
> +	char ch = EOF;
> +	char *p;
> +	p = path; // hack for work fopen

I don't understand this comment, or code, at all. Your comment should
explain as much.

> +	file = fopen(p, "r");
> +	if (file == NULL) {
> +		fprintf(stderr,_("can't open !%s!\n"), path);
> +		return -1;
> +	}
> +
> +	while (1) {
> +		ch = fgetc(file);
> +		if (ch == EOF || ch == '\n')
> +			break;
> +		else
> +			data[i]=ch;
> +		i++;
> +	}
> +	fclose(file);
> +
> +	return 0;
> +}
> +
> +static inline int used(char *name)
> +{
> +	char path_to_disksize[32] = "/sys/block/";
> +	char disksize[32]="";
> +	strncat(path_to_disksize, name, 8);
> +	strncat(path_to_disksize, "/disksize", 9);
> +	if(read_file(path_to_disksize, disksize) < 0)
> +		return -1;

This seems to be poorly reimplementing things in include/sysfs.h.

> +
> +	if (disksize[0] == '0')
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static inline int get_value(char *name, char *data, char *filename)
> +{
> +	char path[48] = "/sys/block/";
> +	strncat(path, name, 8);
> +	strncat(path,"/", 2);
> +	strncat(path, filename, 32);
> +	return read_file(path, data);

Same as above...

> +}
> +
> +static inline int write_file(char *path, char *data)
> +{
> +	FILE *file;
> +	char *p = path;
> +	file = fopen(p, "w");
> +	if (file == NULL) {
> +		fprintf(stderr,_("can't write to %s\n"), path);
> +		return -1;
> +	}
> +	fwrite(data, strlen(data), 1, file);
> +	if (fclose(file) != 0)
> +		return -1;
> +	return 1;

If this fails, it does so silently. There's no explanation of what your
synthesized return values mean.

> +}
> +
> +static inline int value2devparm(char *name, char *data, char *filename)
> +{
> +	char path[48] = "/sys/block/";
> +	strncat(path, name, 8);
> +	strncat(path,"/", 2);
> +	strncat(path, filename, 32);
> +	return write_file(path, data);
> +}

Same comment about sysfs.h

> +
> +static inline int status(void)
> +{
> +	char disksize[32]="";
> +	char orig_data_size[32]="";
> +	char compr_data_size[32]="";
> +	char comp_algorithm[6]="";
> +	char max_comp_streams[12]="";
> +	unsigned i;
> +	char *table = "%6s %12s %10s %10s %4s %4s \n";
> +	printf(table, "NAME","DISKSIZE","ORIG","COMPRES","ALG","THR");

Not really extensible. You really ought to adopt the style used in utils
like lsblk or findmnt to make this dynamic.

> +	for (i=0;;i++) {
> +		char name[8] = "zram";
> +		char num[4];
> +		char *pos;
> +		sprintf(num,"%i",i);
> +		strncat(name, num, 8);
> +		if(!zram_exist(name))
> +			break;
> +		if(!used(name))
> +			continue;
> +		get_value(name, disksize, "disksize");
> +		get_value(name, orig_data_size, "orig_data_size");
> +		get_value(name, compr_data_size, "compr_data_size");
> +		get_value(name, comp_algorithm, "comp_algorithm");
> +		pos = strstr(comp_algorithm, "[lzo]");
> +		if (pos == NULL) {
> +			pos = strstr(comp_algorithm, "[lz4]");
> +			if (pos == NULL)
> +				strncpy(comp_algorithm,"-", 2);
> +			else
> +				strncpy(comp_algorithm, "lz4", 4);
> +		} else
> +			strncpy(comp_algorithm, "lzo", 4);
> +		get_value(name, max_comp_streams, "max_comp_streams");
> +		printf(table,
> +			name,
> +			disksize,
> +			orig_data_size,
> +			compr_data_size,
> +			comp_algorithm,
> +			max_comp_streams
> +		      );
> +	}
> +
> +	return 0;
> +	goto fail;
> +fail:
> +	printf("Zram module not loaded");
> +	return -1;
> +}
> +
> +static inline char *find(void)
> +{
> +	char *ret = NULL;
> +	for (unsigned i=0;;i++) {
> +		char name[8] = "zram";
> +		char num[4];
> +
> +		sprintf(num,"%i",i);
> +		strncat(name, num, 4);
> +		if (!zram_exist(name))
> +			break;
> +		else	//if enough one dir founded -> module loaded
> +			ret = "USED";
> +
> +		if (used(name) == 0) {
> +			ret = name;
> +			break;
> +		}
> +	}
> +
> +		return ret;
> +}
> +
> +
> +
> +static inline void usage(FILE *out)
> +{
> +	fputs(USAGE_HEADER, out);
> +	fprintf(out, _(" %s [-d zramX|-f] -s 64M -a lz4 -t 16\n"), "zramctl");
> +	fputs(USAGE_OPTIONS, out);
> +	fputs(_(" <no args>             return status of used devices\n"), out);
> +	fputs(_(" -f, --find            find free device\n"), out);
> +	fputs(_(" -d, --device [name]   specify device: zramX\n"), out);
> +	fputs(_(" -r, --reset  [name]   reset specified device\n"), out);
> +	fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);
> +	fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);
> +	fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);
> +	fputs(USAGE_SEPARATOR, out);
> +	fputs(USAGE_HELP, out);
> +	fputs(USAGE_VERSION, out);
> +	fprintf(out, USAGE_MAN_TAIL("zramctl(8)"));
> +	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int c = 0;
> +	char *dev = NULL;
> +	char *size = NULL;
> +	char *alg = NULL;

These variable names are pretty nondescript. alg? what is that? a search
algorithm? red algae? size? size of what?

> +	char threads[5] = "1";
> +	unsigned f = 0;
> +
> +	static const struct option longopts[] = {
> +		{"find", no_argument, NULL, 'f'},
> +		{"device", required_argument, NULL, 'd'},
> +		{"size", required_argument, NULL, 's'},
> +		{"alg", required_argument, NULL, 'a'},
> +		{"threads", required_argument, NULL, 't'},
> +		{"reset", required_argument, NULL, 'r'},
> +		{"version", no_argument, NULL, 'V'},
> +		{"help", no_argument, NULL, 'h'},
> +		{NULL, 0, NULL, 0}
> +	};
> +
> +	setlocale(LC_ALL, "");
> +	bindtextdomain(PACKAGE, LOCALEDIR);
> +	textdomain(PACKAGE);
> +	atexit(close_stdout);
> +
> +	while ((c = getopt_long(argc, argv, "fd:s:a:t:r:Vh", longopts, NULL)) !=
> -1) {
> +		switch (c) {
> +		case 'f':
> +			if (dev != NULL) {
> +				fprintf(stderr,_("-f || -d <name>\n"));
> +				return 1;
> +			}
> +			f = 1;
> +			dev = find();

find *what*? The name of this function should be descriptive enough to
tell you that.

> +			break;
> +		case 'd':
> +			if (dev != NULL) {
> +				fprintf(stderr,_("-f || -d <name>\n"));
> +				return 1;
> +			}
> +			dev = optarg;
> +			break;
> +		case 's':
> +			size = optarg;
> +			break;
> +		case 'a':
> +			if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
> +				alg = optarg;
> +			else {
> +				fprintf(stderr,_("%s != lzo|lz4\n"),alg );

How would this string be localized? LZO is LZO is LZO, regardless of
what language you're speaking.

> +				return 1;
> +			}
> +			break;
> +		case 't':
> +			strncpy(threads, optarg, 5);

Why do you need to copy this? More importantly, if someone passes
'100000', you'll be left with { '1', '0', '0', '0', '0' } in your array
(no zero byte termination!)

> +			if (atoi(threads) < 1) {

Poor UX here -- you're erroring out and dumping a wall of text without
explaining why.

> +				usage(stderr);
> +				return 1;
> +			}
> +			break;
> +		case 'r':
> +			dev = optarg;
> +			if (value2devparm(dev, "1", "reset") < 0)
> +				return 1;
> +			break;
> +		case 'V':
> +			printf(UTIL_LINUX_VERSION);
> +			return EXIT_SUCCESS;
> +		case 'h':
> +			usage(stdout);
> +		default:
> +			usage(stderr);
> +		}
> +	}
> +	if (argc == 1) {
> +		status();
> +		return EXIT_SUCCESS;
> +	}
> +
> +	if (!strcmp(dev, "USED")) {
> +		fprintf(stderr,_("all device already used\n") );
> +		return 1;
> +	}
> +
> +	if (dev != NULL && size != NULL) {
> +		if (value2devparm(dev, "1", "reset") < 0)
> +			return 1;
> +		if (atoi(threads) > 1)
> +
> +			if (value2devparm(dev, threads, "max_comp_streams") < 0)
> +				return 1;
> +		if (alg != NULL)
> +			if (value2devparm(dev, alg, "comp_algorithm") < 0)
> +				return 1;
> +
> +		if (value2devparm(dev, size, "disksize") < 0)
> +			return 1;
> +	} else {
> +	}
> +
> +	if (dev != NULL && f > 0)
> +		fprintf(stdout,_("%s\n"), dev);
> +
> +	return EXIT_SUCCESS;
> +}
> \ No newline at end of file
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [Patch] Created zramctl

[Timofey Titovets]

Dave, thanks for comments, I'll think how to fix, what you say.

2014-07-23 20:43 GMT+03:00 Dave Reisner <d@falconindy.com>:
> On Wed, Jul 23, 2014 at 08:28:44PM +0300, Timofey Titovets wrote:
>> Good time of day,
>> I spend some time and rewrite zramctl for util-linux.
>>
>> Please review code and man pages, i think what i can do it better.
>> If you have any suggestion, say it out.
>>
>> Can be pulled from:
>> https://github.com/Nefelim4ag/util-linux
>>
>> For detail see man pages zramctl.8
>>
>
> I have a general fear of your repeated usage of strcpy-ish functions
> into fixed size buffers and lack of error reporting...
>
> I've left a few specific comments inline. I'm sure there's other things
> that need pointing out.
>
>> ----
>>  sys-utils/Makemodule.am |   5 +
>>  sys-utils/zramctl.8     |  92 ++++++++++++++
>>  sys-utils/zramctl.c     | 319
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 416 insertions(+)
>>
>> diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
>> index 68fd030..5d70c51 100644
>> --- a/sys-utils/Makemodule.am
>> +++ b/sys-utils/Makemodule.am
>> @@ -172,6 +172,11 @@ eject_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
>>  dist_man_MANS += sys-utils/eject.1
>>  endif
>>
>> +sbin_PROGRAMS += zramctl
>> +dist_man_MANS += sys-utils/zramctl.8
>> +zramctl_SOURCES = sys-utils/zramctl.c
>> +zramctl_LDADD = $(LDADD) libcommon.la libsmartcols.la
>> +zramctl_CFLAGS = $(AM_CFLAGS) -I$(ul_libsmartcols_incdir)
>>
>>  if BUILD_LOSETUP
>>  sbin_PROGRAMS += losetup
>> diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
>> new file mode 100644
>> index 0000000..4da209d
>> --- /dev/null
>> +++ b/sys-utils/zramctl.8
>> @@ -0,0 +1,92 @@
>> +.TH ZRAMCTL 8 "July 2014" "util-linux" "System Administration"
>> +.SH NAME
>> +zramctl \- set up and control zram devices
>> +.SH SYNOPSIS
>> +.ad l
>> +Get info:
>> +.sp
>> +.in +5
>> +.B zramctl
>> +.sp
>> +.in -5
>> +Reset zram:
>> +.sp
>> +.in +5
>> +.B "zramctl \-r"
>> +.IR zramdev
>> +.sp
>> +.in -5
>> +Print name of first unused zram device:
>> +.sp
>> +.in +5
>> +.B "zramctl \-f"
>> +.sp
>> +.in -5
>> +Setup zram device:
>> +.sp
>> +.in +5
>> +.B zramctl
>> +.RB { \-f\ [ \-d\ \fIzramdev\fP] }
>> +.RB [ \-s
>> +.IR size ]
>> +.RB \ [ \-t
>> +.IR number ]
>> +.in +8
>> +.RB [ \-a
>> +.IR algorithm ]
>> +.sp
>> +.in -13
>> +.ad b
>> +.SH DESCRIPTION
>> +.B zramctl
>> +is used to fast setup zram devices parametrs, to reset zram devices and to
>> +query the status of a used zram devices.
>> +If no option is given, all zram devices are shown.
>> +
>> +
>> +.SH OPTIONS
>> +
>> +.IP "\fB\-s, \-\-size\fP \fIsize\fP
>> +force zram driver to reread size of the file associated with the specified
>> zram device
>> ++The \fIsize\fR arguments may be followed by the multiplicative
>> ++suffixes 128K 512M, 1G.
>> +.IP "\fB\-r, \-\-reset\fP \fIzramdev\fP"
>> +Reset options specified zram device(s). Zram device setting can be changed
>> only
>> +after reset.
>> +.IP "\fB\-f, \-\-find\fP"
>> +find the first unused zram device. If a
>> +.R \-s \fIsize\fR
>> +argument is present, use this device.
>> +.IP "\fB\-h, \-\-help\fP"
>> +print help
>> +.IP "\fB\-t, \-\-threads \fInumber\fP"
>> +Set number of maximum compress streams what used for device.
>> +.IP "\fB\-a, \-\-alg \fI{lzo|lz4}\fP""
>> +Set compress algorithm used for compress data in zram device.
>> +
>> +.SH RETURN VALUE
>> +.B zramctl
>> +returns 0 on success, nonzero on failure.
>> +
>> +.SH FILES
>> +.TP
>> +.I /dev/zram[0..N]
>> +zram block devices
>> +
>> +.SH EXAMPLE
>> +The following commands can be used as an example of using the zram device.
>> +.nf
>> +.IP
>> +# zramctl --find --size 1024M
>> +/dev/zram0
>> +# mkswap /dev/zram0
>> +# swapon /dev/zram0
>> + ...
>> +# swapoff /dev/zram0
>> +# zramctl --reset /dev/zram0
>> +.fi
>> +.SH AUTHORS
>> +Timofey Titovets <nefelim4ag@gmail.com>
>> +.SH AVAILABILITY
>> +The zramctl command is part of the util-linux package and is available from
>> +ftp://ftp.kernel.org/pub/linux/utils/util-linux/.
>> diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
>> new file mode 100644
>> index 0000000..6ddee9b
>> --- /dev/null
>> +++ b/sys-utils/zramctl.c
>> @@ -0,0 +1,319 @@
>> +/*
>> + * zramctl - purpose of it
>> + *
>> + * Copyright (c) 20nn  Example Commercial, Inc
>> + * Written by Timofey Titovets <Nefelim4ag@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it would be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, write to the Free Software Foundation, Inc.,
>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> + */
>> +
>> +#include <getopt.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <string.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +
>> +#include "c.h"
>> +#include "closestream.h"
>> +#include "nls.h"
>> +
>> +static inline int zram_exist(char *name)
>> +{
>> +     struct stat info;
>> +     char path[16] = "/dev/";
>> +     strncat(path, name, 8);
>> +     if(stat(path, &info) != 0 )
>> +             return 0;
>> +     if (info.st_mode&S_IFBLK)
>> +             return 1;
>> +     return 0;
>> +}
>> +
>> +static inline int read_file(char *path, char *data)
>> +{
>> +     FILE *file;
>> +     unsigned i = 0;
>> +     char ch = EOF;
>> +     char *p;
>> +     p = path; // hack for work fopen
>
> I don't understand this comment, or code, at all. Your comment should
> explain as much.
>
>> +     file = fopen(p, "r");
>> +     if (file == NULL) {
>> +             fprintf(stderr,_("can't open !%s!\n"), path);
>> +             return -1;
>> +     }
>> +
>> +     while (1) {
>> +             ch = fgetc(file);
>> +             if (ch == EOF || ch == '\n')
>> +                     break;
>> +             else
>> +                     data[i]=ch;
>> +             i++;
>> +     }
>> +     fclose(file);
>> +
>> +     return 0;
>> +}
>> +
>> +static inline int used(char *name)
>> +{
>> +     char path_to_disksize[32] = "/sys/block/";
>> +     char disksize[32]="";
>> +     strncat(path_to_disksize, name, 8);
>> +     strncat(path_to_disksize, "/disksize", 9);
>> +     if(read_file(path_to_disksize, disksize) < 0)
>> +             return -1;
>
> This seems to be poorly reimplementing things in include/sysfs.h.
>
>> +
>> +     if (disksize[0] == '0')
>> +             return 0;
>> +
>> +     return 1;
>> +}
>> +
>> +static inline int get_value(char *name, char *data, char *filename)
>> +{
>> +     char path[48] = "/sys/block/";
>> +     strncat(path, name, 8);
>> +     strncat(path,"/", 2);
>> +     strncat(path, filename, 32);
>> +     return read_file(path, data);
>
> Same as above...
>
>> +}
>> +
>> +static inline int write_file(char *path, char *data)
>> +{
>> +     FILE *file;
>> +     char *p = path;
>> +     file = fopen(p, "w");
>> +     if (file == NULL) {
>> +             fprintf(stderr,_("can't write to %s\n"), path);
>> +             return -1;
>> +     }
>> +     fwrite(data, strlen(data), 1, file);
>> +     if (fclose(file) != 0)
>> +             return -1;
>> +     return 1;
>
> If this fails, it does so silently. There's no explanation of what your
> synthesized return values mean.
>
>> +}
>> +
>> +static inline int value2devparm(char *name, char *data, char *filename)
>> +{
>> +     char path[48] = "/sys/block/";
>> +     strncat(path, name, 8);
>> +     strncat(path,"/", 2);
>> +     strncat(path, filename, 32);
>> +     return write_file(path, data);
>> +}
>
> Same comment about sysfs.h
>
>> +
>> +static inline int status(void)
>> +{
>> +     char disksize[32]="";
>> +     char orig_data_size[32]="";
>> +     char compr_data_size[32]="";
>> +     char comp_algorithm[6]="";
>> +     char max_comp_streams[12]="";
>> +     unsigned i;
>> +     char *table = "%6s %12s %10s %10s %4s %4s \n";
>> +     printf(table, "NAME","DISKSIZE","ORIG","COMPRES","ALG","THR");
>
> Not really extensible. You really ought to adopt the style used in utils
> like lsblk or findmnt to make this dynamic.
>
>> +     for (i=0;;i++) {
>> +             char name[8] = "zram";
>> +             char num[4];
>> +             char *pos;
>> +             sprintf(num,"%i",i);
>> +             strncat(name, num, 8);
>> +             if(!zram_exist(name))
>> +                     break;
>> +             if(!used(name))
>> +                     continue;
>> +             get_value(name, disksize, "disksize");
>> +             get_value(name, orig_data_size, "orig_data_size");
>> +             get_value(name, compr_data_size, "compr_data_size");
>> +             get_value(name, comp_algorithm, "comp_algorithm");
>> +             pos = strstr(comp_algorithm, "[lzo]");
>> +             if (pos == NULL) {
>> +                     pos = strstr(comp_algorithm, "[lz4]");
>> +                     if (pos == NULL)
>> +                             strncpy(comp_algorithm,"-", 2);
>> +                     else
>> +                             strncpy(comp_algorithm, "lz4", 4);
>> +             } else
>> +                     strncpy(comp_algorithm, "lzo", 4);
>> +             get_value(name, max_comp_streams, "max_comp_streams");
>> +             printf(table,
>> +                     name,
>> +                     disksize,
>> +                     orig_data_size,
>> +                     compr_data_size,
>> +                     comp_algorithm,
>> +                     max_comp_streams
>> +                   );
>> +     }
>> +
>> +     return 0;
>> +     goto fail;
>> +fail:
>> +     printf("Zram module not loaded");
>> +     return -1;
>> +}
>> +
>> +static inline char *find(void)
>> +{
>> +     char *ret = NULL;
>> +     for (unsigned i=0;;i++) {
>> +             char name[8] = "zram";
>> +             char num[4];
>> +
>> +             sprintf(num,"%i",i);
>> +             strncat(name, num, 4);
>> +             if (!zram_exist(name))
>> +                     break;
>> +             else    //if enough one dir founded -> module loaded
>> +                     ret = "USED";
>> +
>> +             if (used(name) == 0) {
>> +                     ret = name;
>> +                     break;
>> +             }
>> +     }
>> +
>> +             return ret;
>> +}
>> +
>> +
>> +
>> +static inline void usage(FILE *out)
>> +{
>> +     fputs(USAGE_HEADER, out);
>> +     fprintf(out, _(" %s [-d zramX|-f] -s 64M -a lz4 -t 16\n"), "zramctl");
>> +     fputs(USAGE_OPTIONS, out);
>> +     fputs(_(" <no args>             return status of used devices\n"), out);
>> +     fputs(_(" -f, --find            find free device\n"), out);
>> +     fputs(_(" -d, --device [name]   specify device: zramX\n"), out);
>> +     fputs(_(" -r, --reset  [name]   reset specified device\n"), out);
>> +     fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);
>> +     fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);
>> +     fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);
>> +     fputs(USAGE_SEPARATOR, out);
>> +     fputs(USAGE_HELP, out);
>> +     fputs(USAGE_VERSION, out);
>> +     fprintf(out, USAGE_MAN_TAIL("zramctl(8)"));
>> +     exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +     int c = 0;
>> +     char *dev = NULL;
>> +     char *size = NULL;
>> +     char *alg = NULL;
>
> These variable names are pretty nondescript. alg? what is that? a search
> algorithm? red algae? size? size of what?
>
>> +     char threads[5] = "1";
>> +     unsigned f = 0;
>> +
>> +     static const struct option longopts[] = {
>> +             {"find", no_argument, NULL, 'f'},
>> +             {"device", required_argument, NULL, 'd'},
>> +             {"size", required_argument, NULL, 's'},
>> +             {"alg", required_argument, NULL, 'a'},
>> +             {"threads", required_argument, NULL, 't'},
>> +             {"reset", required_argument, NULL, 'r'},
>> +             {"version", no_argument, NULL, 'V'},
>> +             {"help", no_argument, NULL, 'h'},
>> +             {NULL, 0, NULL, 0}
>> +     };
>> +
>> +     setlocale(LC_ALL, "");
>> +     bindtextdomain(PACKAGE, LOCALEDIR);
>> +     textdomain(PACKAGE);
>> +     atexit(close_stdout);
>> +
>> +     while ((c = getopt_long(argc, argv, "fd:s:a:t:r:Vh", longopts, NULL)) !=
>> -1) {
>> +             switch (c) {
>> +             case 'f':
>> +                     if (dev != NULL) {
>> +                             fprintf(stderr,_("-f || -d <name>\n"));
>> +                             return 1;
>> +                     }
>> +                     f = 1;
>> +                     dev = find();
>
> find *what*? The name of this function should be descriptive enough to
> tell you that.
>
>> +                     break;
>> +             case 'd':
>> +                     if (dev != NULL) {
>> +                             fprintf(stderr,_("-f || -d <name>\n"));
>> +                             return 1;
>> +                     }
>> +                     dev = optarg;
>> +                     break;
>> +             case 's':
>> +                     size = optarg;
>> +                     break;
>> +             case 'a':
>> +                     if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
>> +                             alg = optarg;
>> +                     else {
>> +                             fprintf(stderr,_("%s != lzo|lz4\n"),alg );
>
> How would this string be localized? LZO is LZO is LZO, regardless of
> what language you're speaking.
>
>> +                             return 1;
>> +                     }
>> +                     break;
>> +             case 't':
>> +                     strncpy(threads, optarg, 5);
>
> Why do you need to copy this? More importantly, if someone passes
> '100000', you'll be left with { '1', '0', '0', '0', '0' } in your array
> (no zero byte termination!)
>
>> +                     if (atoi(threads) < 1) {
>
> Poor UX here -- you're erroring out and dumping a wall of text without
> explaining why.
>
>> +                             usage(stderr);
>> +                             return 1;
>> +                     }
>> +                     break;
>> +             case 'r':
>> +                     dev = optarg;
>> +                     if (value2devparm(dev, "1", "reset") < 0)
>> +                             return 1;
>> +                     break;
>> +             case 'V':
>> +                     printf(UTIL_LINUX_VERSION);
>> +                     return EXIT_SUCCESS;
>> +             case 'h':
>> +                     usage(stdout);
>> +             default:
>> +                     usage(stderr);
>> +             }
>> +     }
>> +     if (argc == 1) {
>> +             status();
>> +             return EXIT_SUCCESS;
>> +     }
>> +
>> +     if (!strcmp(dev, "USED")) {
>> +             fprintf(stderr,_("all device already used\n") );
>> +             return 1;
>> +     }
>> +
>> +     if (dev != NULL && size != NULL) {
>> +             if (value2devparm(dev, "1", "reset") < 0)
>> +                     return 1;
>> +             if (atoi(threads) > 1)
>> +
>> +                     if (value2devparm(dev, threads, "max_comp_streams") < 0)
>> +                             return 1;
>> +             if (alg != NULL)
>> +                     if (value2devparm(dev, alg, "comp_algorithm") < 0)
>> +                             return 1;
>> +
>> +             if (value2devparm(dev, size, "disksize") < 0)
>> +                     return 1;
>> +     } else {
>> +     }
>> +
>> +     if (dev != NULL && f > 0)
>> +             fprintf(stdout,_("%s\n"), dev);
>> +
>> +     return EXIT_SUCCESS;
>> +}
>> \ No newline at end of file
>> --
>> To unsubscribe from this list: send the line "unsubscribe util-linux" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards,
Timofey.

Re: [RFC] [Patch] Created zramctl

[Karel Zak]

On Wed, Jul 23, 2014 at 01:43:48PM -0400, Dave Reisner wrote:
> > +	char *table = "%6s %12s %10s %10s %4s %4s \n";
> > +	printf(table, "NAME","DISKSIZE","ORIG","COMPRES","ALG","THR");
> 
> Not really extensible. You really ought to adopt the style used in utils
> like lsblk or findmnt to make this dynamic.

 Dave is talking about libsmartcols, see losetup.c: show_table(), it's
 probably the most simple example for the same thing like zramctl.

    Karel


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

Re: [RFC] [Patch] Created zramctl

[Karel Zak]

On Wed, Jul 23, 2014 at 08:28:44PM +0300, Timofey Titovets wrote:
> Good time of day,
> I spend some time and rewrite zramctl for util-linux.
> 
> Please review code and man pages, i think what i can do it better.
> If you have any suggestion, say it out.
> 
> Can be pulled from:
> https://github.com/Nefelim4ag/util-linux
> 
> For detail see man pages zramctl.8
> 
> ----
>  sys-utils/Makemodule.am |   5 +
>  sys-utils/zramctl.8     |  92 ++++++++++++++
>  sys-utils/zramctl.c     | 319
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 416 insertions(+)
> 
> diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
> index 68fd030..5d70c51 100644
> --- a/sys-utils/Makemodule.am
> +++ b/sys-utils/Makemodule.am
> @@ -172,6 +172,11 @@ eject_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
>  dist_man_MANS += sys-utils/eject.1
>  endif
> 
> +sbin_PROGRAMS += zramctl
> +dist_man_MANS += sys-utils/zramctl.8
> +zramctl_SOURCES = sys-utils/zramctl.c
> +zramctl_LDADD = $(LDADD) libcommon.la libsmartcols.la
> +zramctl_CFLAGS = $(AM_CFLAGS) -I$(ul_libsmartcols_incdir)
> 
>  if BUILD_LOSETUP
>  sbin_PROGRAMS += losetup
> diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
> new file mode 100644
> index 0000000..4da209d
> --- /dev/null
> +++ b/sys-utils/zramctl.8
> @@ -0,0 +1,92 @@
> +.TH ZRAMCTL 8 "July 2014" "util-linux" "System Administration"
> +.SH NAME
> +zramctl \- set up and control zram devices
> +.SH SYNOPSIS
> +.ad l
> +Get info:
> +.sp
> +.in +5
> +.B zramctl
> +.sp
> +.in -5
> +Reset zram:
> +.sp
> +.in +5
> +.B "zramctl \-r"
> +.IR zramdev
> +.sp
> +.in -5
> +Print name of first unused zram device:
> +.sp
> +.in +5
> +.B "zramctl \-f"
> +.sp
> +.in -5
> +Setup zram device:
> +.sp
> +.in +5
> +.B zramctl
> +.RB { \-f\ [ \-d\ \fIzramdev\fP] }
> +.RB [ \-s
> +.IR size ]
> +.RB \ [ \-t
> +.IR number ]
> +.in +8
> +.RB [ \-a
> +.IR algorithm ]
> +.sp
> +.in -13
> +.ad b
> +.SH DESCRIPTION
> +.B zramctl
> +is used to fast setup zram devices parametrs, to reset zram devices and to
> +query the status of a used zram devices.
> +If no option is given, all zram devices are shown.
> +
> +
> +.SH OPTIONS
> +
> +.IP "\fB\-s, \-\-size\fP \fIsize\fP
> +force zram driver to reread size of the file associated with the specified
> zram device
> ++The \fIsize\fR arguments may be followed by the multiplicative
> ++suffixes 128K 512M, 1G.
> +.IP "\fB\-r, \-\-reset\fP \fIzramdev\fP"
> +Reset options specified zram device(s). Zram device setting can be changed
> only
> +after reset.
> +.IP "\fB\-f, \-\-find\fP"
> +find the first unused zram device. If a
> +.R \-s \fIsize\fR
> +argument is present, use this device.
> +.IP "\fB\-h, \-\-help\fP"
> +print help
> +.IP "\fB\-t, \-\-threads \fInumber\fP"
> +Set number of maximum compress streams what used for device.
> +.IP "\fB\-a, \-\-alg \fI{lzo|lz4}\fP""
> +Set compress algorithm used for compress data in zram device.
> +
> +.SH RETURN VALUE
> +.B zramctl
> +returns 0 on success, nonzero on failure.
> +
> +.SH FILES
> +.TP
> +.I /dev/zram[0..N]
> +zram block devices
> +
> +.SH EXAMPLE
> +The following commands can be used as an example of using the zram device.
> +.nf
> +.IP
> +# zramctl --find --size 1024M
> +/dev/zram0
> +# mkswap /dev/zram0
> +# swapon /dev/zram0
> + ...
> +# swapoff /dev/zram0
> +# zramctl --reset /dev/zram0
> +.fi
> +.SH AUTHORS
> +Timofey Titovets <nefelim4ag@gmail.com>
> +.SH AVAILABILITY
> +The zramctl command is part of the util-linux package and is available from
> +ftp://ftp.kernel.org/pub/linux/utils/util-linux/.
> diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
> new file mode 100644
> index 0000000..6ddee9b
> --- /dev/null
> +++ b/sys-utils/zramctl.c
> @@ -0,0 +1,319 @@
> +/*
> + * zramctl - purpose of it
> + *
> + * Copyright (c) 20nn  Example Commercial, Inc
> + * Written by Timofey Titovets <Nefelim4ag@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <getopt.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "c.h"
> +#include "closestream.h"
> +#include "nls.h"
> +
> +static inline int zram_exist(char *name)
> +{
> +	struct stat info;
> +	char path[16] = "/dev/";
> +	strncat(path, name, 8);
> +	if(stat(path, &info) != 0 )
> +		return 0;
> +	if (info.st_mode&S_IFBLK)
> +		return 1;
> +	return 0;
> +}
> +
> +static inline int read_file(char *path, char *data)
> +{
> +	FILE *file;
> +	unsigned i = 0;
> +	char ch = EOF;
> +	char *p;
> +	p = path; // hack for work fopen
> +	file = fopen(p, "r");
> +	if (file == NULL) {
> +		fprintf(stderr,_("can't open !%s!\n"), path);
> +		return -1;
> +	}
> +
> +	while (1) {
> +		ch = fgetc(file);
> +		if (ch == EOF || ch == '\n')
> +			break;
> +		else
> +			data[i]=ch;
> +		i++;
> +	}
> +	fclose(file);
> +
> +	return 0;
> +}
> +
> +static inline int used(char *name)
> +{
> +	char path_to_disksize[32] = "/sys/block/";
> +	char disksize[32]="";
> +	strncat(path_to_disksize, name, 8);
> +	strncat(path_to_disksize, "/disksize", 9);
> +	if(read_file(path_to_disksize, disksize) < 0)
> +		return -1;
> +
> +	if (disksize[0] == '0')
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static inline int get_value(char *name, char *data, char *filename)
> +{
> +	char path[48] = "/sys/block/";
> +	strncat(path, name, 8);
> +	strncat(path,"/", 2);
> +	strncat(path, filename, 32);
> +	return read_file(path, data);
> +}
> +
> +static inline int write_file(char *path, char *data)
> +{
> +	FILE *file;
> +	char *p = path;
> +	file = fopen(p, "w");
> +	if (file == NULL) {
> +		fprintf(stderr,_("can't write to %s\n"), path);
> +		return -1;
> +	}
> +	fwrite(data, strlen(data), 1, file);
> +	if (fclose(file) != 0)
> +		return -1;
> +	return 1;
> +}
> +
> +static inline int value2devparm(char *name, char *data, char *filename)
> +{
> +	char path[48] = "/sys/block/";
> +	strncat(path, name, 8);
> +	strncat(path,"/", 2);
> +	strncat(path, filename, 32);
> +	return write_file(path, data);
> +}
> +
> +static inline int status(void)
> +{
> +	char disksize[32]="";
> +	char orig_data_size[32]="";
> +	char compr_data_size[32]="";
> +	char comp_algorithm[6]="";
> +	char max_comp_streams[12]="";
> +	unsigned i;
> +	char *table = "%6s %12s %10s %10s %4s %4s \n";
> +	printf(table, "NAME","DISKSIZE","ORIG","COMPRES","ALG","THR");
> +	for (i=0;;i++) {
> +		char name[8] = "zram";
> +		char num[4];
> +		char *pos;
> +		sprintf(num,"%i",i);
> +		strncat(name, num, 8);
> +		if(!zram_exist(name))
> +			break;
> +		if(!used(name))
> +			continue;
> +		get_value(name, disksize, "disksize");
> +		get_value(name, orig_data_size, "orig_data_size");
> +		get_value(name, compr_data_size, "compr_data_size");
> +		get_value(name, comp_algorithm, "comp_algorithm");
> +		pos = strstr(comp_algorithm, "[lzo]");
> +		if (pos == NULL) {
> +			pos = strstr(comp_algorithm, "[lz4]");
> +			if (pos == NULL)
> +				strncpy(comp_algorithm,"-", 2);
> +			else
> +				strncpy(comp_algorithm, "lz4", 4);
> +		} else
> +			strncpy(comp_algorithm, "lzo", 4);
> +		get_value(name, max_comp_streams, "max_comp_streams");
> +		printf(table,
> +			name,
> +			disksize,
> +			orig_data_size,
> +			compr_data_size,
> +			comp_algorithm,
> +			max_comp_streams
> +		      );
> +	}
> +
> +	return 0;
> +	goto fail;
> +fail:
> +	printf("Zram module not loaded");
> +	return -1;
> +}
> +
> +static inline char *find(void)
> +{
> +	char *ret = NULL;
> +	for (unsigned i=0;;i++) {
> +		char name[8] = "zram";
> +		char num[4];
> +
> +		sprintf(num,"%i",i);
> +		strncat(name, num, 4);
> +		if (!zram_exist(name))
> +			break;
> +		else	//if enough one dir founded -> module loaded
> +			ret = "USED";
> +
> +		if (used(name) == 0) {
> +			ret = name;
> +			break;
> +		}
> +	}
> +
> +		return ret;
> +}
> +
> +
> +
> +static inline void usage(FILE *out)
> +{
> +	fputs(USAGE_HEADER, out);
> +	fprintf(out, _(" %s [-d zramX|-f] -s 64M -a lz4 -t 16\n"), "zramctl");
> +	fputs(USAGE_OPTIONS, out);
> +	fputs(_(" <no args>             return status of used devices\n"), out);
> +	fputs(_(" -f, --find            find free device\n"), out);
> +	fputs(_(" -d, --device [name]   specify device: zramX\n"), out);
> +	fputs(_(" -r, --reset  [name]   reset specified device\n"), out);
> +	fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);
> +	fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);
> +	fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);
> +	fputs(USAGE_SEPARATOR, out);
> +	fputs(USAGE_HELP, out);
> +	fputs(USAGE_VERSION, out);
> +	fprintf(out, USAGE_MAN_TAIL("zramctl(8)"));
> +	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int c = 0;
> +	char *dev = NULL;
> +	char *size = NULL;
> +	char *alg = NULL;
> +	char threads[5] = "1";
> +	unsigned f = 0;
> +
> +	static const struct option longopts[] = {
> +		{"find", no_argument, NULL, 'f'},
> +		{"device", required_argument, NULL, 'd'},
> +		{"size", required_argument, NULL, 's'},
> +		{"alg", required_argument, NULL, 'a'},
> +		{"threads", required_argument, NULL, 't'},
> +		{"reset", required_argument, NULL, 'r'},
> +		{"version", no_argument, NULL, 'V'},
> +		{"help", no_argument, NULL, 'h'},
> +		{NULL, 0, NULL, 0}
> +	};
> +
> +	setlocale(LC_ALL, "");
> +	bindtextdomain(PACKAGE, LOCALEDIR);
> +	textdomain(PACKAGE);
> +	atexit(close_stdout);
> +
> +	while ((c = getopt_long(argc, argv, "fd:s:a:t:r:Vh", longopts, NULL)) !=
> -1) {
> +		switch (c) {
> +		case 'f':
> +			if (dev != NULL) {
> +				fprintf(stderr,_("-f || -d <name>\n"));
> +				return 1;
> +			}

 we usually use err() or errx() rather than fprintf() + return/exit()).

> +			f = 1;
> +			dev = find();
> +			break;
> +		case 'd':
> +			if (dev != NULL) {
> +				fprintf(stderr,_("-f || -d <name>\n"));
> +				return 1;
> +			}
> +			dev = optarg;
> +			break;
> +		case 's':
> +			size = optarg;
> +			break;
> +		case 'a':
> +			if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
> +				alg = optarg;
> +			else {
> +				fprintf(stderr,_("%s != lzo|lz4\n"),alg );
> +				return 1;
> +			}
> +			break;
> +		case 't':
> +			strncpy(threads, optarg, 5);
> +			if (atoi(threads) < 1) {


 see strutils.h where is strtos16_or_err() and another functions to
 parse users input.

    Karel

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

Re: [RFC] [Patch] Created zramctl

[Benno Schulenberg]

On Wed, Jul 23, 2014, at 19:28, Timofey Titovets wrote:
> +.B zramctl
> +.RB { \-f\ [ \-d\ \fIzramdev\fP] }

Hm.  Elsewhere it says that -f and -d cannot be combined,
so you probably forgot the OR operator here:

.RB [ \-f " | " \-d\ \fIzramdev\fP ]

> +.B zramctl
> +is used to fast setup zram devices parametrs, to reset zram devices and to
> +query the status of a used zram devices.

Better:
"is used to quickly set up zram device parameters, to reset zram devices,
and to query the status of used zram devices."

> +.IP "\fB\-s, \-\-size\fP \fIsize\fP
> +force zram driver to reread size of the file associated with the 
> specified zram device

Start a sentence with a capital, end with a period.

> ++The \fIsize\fR arguments may be followed by the multiplicative
> ++suffixes 128K 512M, 1G.

This doesn't make sense.  Also the extra plusses shouldn't be there.
Probably you mean something like:

  The \fIsize\fR argument may be followed by one of the multiplicative
  suffixes K, M, G...  For example: 512M.

> +.IP "\fB\-h, \-\-help\fP"
> +print help

The --version option is missing from the man page.  Copy both of
them verbatim from another recent man page.

> +.SH EXAMPLE
> +The following commands can be used as an example of using the zram device.

The heading already says that it is an example.  Better describe roughly
what the commands do: set up a zram device of one gigabyte , use it as
swap, and reset it after use.  Something like that.


> + * Copyright (c) 20nn  Example Commercial, Inc
> + * Written by Timofey Titovets <Nefelim4ag@gmail.com>

This should probably just say:

 * Copyright (c) 2014 Timofey Titovets <nefelim4ag@gmail.com>


> +	file = fopen(p, "w");
> +	if (file == NULL) {
> +		fprintf(stderr,_("can't write to %s\n"), path);

That should say: "cannot open %s".

> +		return -1;
> +	}
> +	fwrite(data, strlen(data), 1, file);

You don't want to check that the write succeeded?

> +	return 0;
> +	goto fail;
> +fail:

Ehm... return 0 and then goto right here?

> +	printf("Zram module not loaded");
> +	return -1;

Also here you would want to use err() or errx():
errx("the zram module is not loaded");


> +	fputs(USAGE_HEADER, out);
> +	fprintf(out, _(" %s [-d zramX|-f] -s 64M -a lz4 -t 16\n"), "zramctl");

It should describe all possible commands, not give an example:

_(" %s [-d zram<N>|-f] -s <size> -a lz4|lzo -t <number>\n")

> +	fputs(USAGE_OPTIONS, out);
> +	fputs(_(" <no args>             return status of used devices\n"), out);

This is unusual.  After the "Options:" header only options should
follow.  Other descriptions should come before it, or at the end.

It is not yet usual, but it would be nice to have a "docstring" after
the above synopsis, a sentence that very concisely describes the
command.  In this case something like: "Set up a zram device.
Without arguments it reports the status of all used devices."

> +	fputs(_(" -f, --find            find free device\n"), out);
> +	fputs(_(" -d, --device [name]   specify device: zramX\n"), out);
> +	fputs(_(" -r, --reset  [name]   reset specified device\n"), out);

s/[name]/<name>/

> +	fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);

s/[size]/<size>/   And don't mix examples into the decription.

> +	fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);

s/[lzo|lz4]/lzo|lz4/

> +	fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);

s/[0<num]/<number>/

> +			if (dev != NULL) {
> +				fprintf(stderr,_("-f || -d <name>\n"));

Better use an error message in everyday language, not in programmer
speak: "only one of -options d and -f can be used".

> +		case 'a':
> +			if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
> +				alg = optarg;
> +			else {
> +				fprintf(stderr,_("%s != lzo|lz4\n"),alg );

In the last line s/alg/optarg/.  Or leave it out altogether.
Also use a human-readable message:
"the algorithm should be either lzo or lz4"

> +	if (!strcmp(dev, "USED")) {
> +		fprintf(stderr,_("all device already used\n") );

"all available devices are in use"

> +	if (dev != NULL && f > 0)
> +		fprintf(stdout,_("%s\n"), dev);

There's no point in gettextizing a string that contains nothing
translatable, so leave out the _() here.

Benno

-- 
http://www.fastmail.fm - A no graphics, no pop-ups email service

[RFC] [Patch v2] Created zramctl

[Timofey Titovets]

Good time of day,
I spend some time and rewrite zramctl for util-linux.

Please review code and man pages.
If you have any suggestion, say it out.

Can be pulled from:
https://github.com/Nefelim4ag/util-linux.git

For detail see man pages zramctl.8

v1 -> v2:
Use function from ./include directory
Better error handling
Implemented flexible table for status command, thanks Karel Zak for 
libsmartcols
Some fixes, thanks: Dave Reisner, Karel Zak, Benno Schulenberg


Note:
sysfs.h powerful, but i can't understand how working sysfs.h and how i 
can implement it(?), and i think what my helper function is easier now, 
than sysfs.h realization

----
  configure.ac            |   8 ++
  sys-utils/Makemodule.am |   8 ++
  sys-utils/zramctl.8     |  98 ++++++++++++++++
  sys-utils/zramctl.c     | 302 
++++++++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 416 insertions(+)


diff --git a/configure.ac b/configure.ac
index aae2456..f3204d9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -859,6 +859,14 @@ UL_REQUIRES_LINUX([losetup])
  UL_REQUIRES_BUILD([losetup], [libsmartcols])
  AM_CONDITIONAL([BUILD_LOSETUP], [test "x$build_losetup" = xyes])

+AC_ARG_ENABLE([zramctl],
+  AS_HELP_STRING([--disable-zramctl], [do not build zramctl]),
+  [], [UL_DEFAULT_ENABLE([zramctl], [check])]
+)
+UL_BUILD_INIT([zramctl])
+UL_REQUIRES_LINUX([zramctl])
+UL_REQUIRES_BUILD([zramctl], [libsmartcols])
+AM_CONDITIONAL([BUILD_ZRAMCTL], [test "x$build_zramctl" = xyes])

  AC_ARG_ENABLE([fsck],
    AS_HELP_STRING([--disable-fsck], [do not build fsck]),
diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 4741fed..c6a2c5a 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -362,3 +362,11 @@ dist_man_MANS += sys-utils/setpriv.1
  setpriv_SOURCES = sys-utils/setpriv.c
  setpriv_LDADD = $(LDADD) -lcap-ng libcommon.la
  endif
+
+if BUILD_ZRAMCTL
+sbin_PROGRAMS += zramctl
+dist_man_MANS += sys-utils/zramctl.8
+zramctl_SOURCES = sys-utils/zramctl.c
+zramctl_LDADD = $(LDADD) libcommon.la libsmartcols.la
+zramctl_CFLAGS = $(AM_CFLAGS) -I$(ul_libsmartcols_incdir)
+endif
\ No newline at end of file
diff --git a/sys-utils/zramctl.8 b/sys-utils/zramctl.8
new file mode 100644
index 0000000..158c391
--- /dev/null
+++ b/sys-utils/zramctl.8
@@ -0,0 +1,98 @@
+.TH ZRAMCTL 8 "July 2014" "util-linux" "System Administration"
+.SH NAME
+zramctl \- set up and control zram devices
+.SH SYNOPSIS
+.ad l
+Get info:
+.sp
+.in +5
+.B zramctl
+.sp
+.in -5
+Reset zram:
+.sp
+.in +5
+.B "zramctl \-r"
+.IR zramdev
+.sp
+.in -5
+Print name of first unused zram device:
+.sp
+.in +5
+.B "zramctl \-f"
+.sp
+.in -5
+Setup zram device:
+.sp
+.in +5
+.B zramctl
+.RB [ \-f " | " \-d\ \fIzramdev\fP ]
+.RB [ \-s
+.IR size ]
+.RB \ [ \-t
+.IR number ]
+.in +8
+.RB [ \-a
+.IR algorithm ]
+.sp
+.in -13
+.ad b
+.SH DESCRIPTION
+.B zramctl
+Is used to quickly set up zram device parameters, to reset zram devices,
+and to query the status of used zram devices.
+If no option is given, all zram devices are shown.
+
+
+.SH OPTIONS
+
+.IP "\fB\-s, \-\-size\fP \fIsize\fP
+Force zram driver to reread size of the file associated with the 
specified zram device
++The \fIsize\fR arguments may be followed by the multiplicative
++suffixes K, M, G... Example: 512M.
+.IP "\fB\-r, \-\-reset\fP \fIzramdev\fP"
+Reset options specified zram device(s). Zram device setting can be 
changed only
+after reset.
+.IP "\fB\-d, \-\-device \fIzramdev\fP"
+if only \-d \fIzramdev\fP specified - show status for specified device. 
if additional
+\-s \fIsize\fR specified, setup specified device.
+.IP "\fB\-f, \-\-find\fP"
+find the first unused zram device. If a
+.R \-s \fIsize\fR
+argument is present, use this device.
+.IP "\fB\-h, \-\-help\fP"
+print help
+.IP "\fB\-t, \-\-threads \fInumber\fP"
+Set number of maximum compress streams what used for device.
+.IP "\fB\-a, \-\-alg \fI{lzo|lz4}\fP""
+Set compress algorithm used for compress data in zram device.
+.B "\fB\-V, \-\-version\fP"
+Display version information and exit.
+
+.SH RETURN VALUE
+.B zramctl
+returns 0 on success, nonzero on failure.
+
+.SH FILES
+.TP
+.I /dev/zram[0..N]
+zram block devices
+
+.SH EXAMPLE
+The following commands can be used for setup the zram device with 
gigabyte size
+ and using as swap device.
+.nf
+.IP
+# zramctl --find --size 1024M
+/dev/zram0
+# mkswap /dev/zram0
+# swapon /dev/zram0
+ ...
+# swapoff /dev/zram0
+# zramctl --reset /dev/zram0
+.fi
+.SH AUTHORS
+Timofey Titovets <nefelim4ag@gmail.com>
+.SH AVAILABILITY
+The zramctl command is part of the util-linux package and is available from
+ftp://ftp.kernel.org/pub/linux/utils/util-linux/.
diff --git a/sys-utils/zramctl.c b/sys-utils/zramctl.c
new file mode 100644
index 0000000..e1ad4eb
--- /dev/null
+++ b/sys-utils/zramctl.c
@@ -0,0 +1,302 @@
+/*
+ * zramctl - purpose of it
+ *
+ * Copyright (c) 2014 Timofey Titovets <Nefelim4ag@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <getopt.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdarg.h>
+#include "errno.h"
+#include "path.h"
+#include "pathnames.h"
+#include "exitcodes.h"
+#include "optutils.h"
+#include "ismounted.h"
+#include "strutils.h"
+#include "ismounted.h"
+#include "strutils.h"
+#include "sysfs.h"
+#include <libsmartcols.h>
+
+#include "c.h"
+#include "closestream.h"
+#include "nls.h"
+
+static inline int zram_exist(char *name)
+{
+	char path[16] = "/dev/";
+
+	strncat(path, name, 8);
+
+	if (!strcmp(name, "zram0")) {
+		if (path_exist(path))
+			return 1;
+		else
+			errx(EXIT_FAILURE,_("zram module not loaded"));
+	}
+
+	return path_exist(path);
+}
+
+static inline void get_value(char *name, char *data, char *filename)
+{
+	char path[64] = "/sys/block/";
+	strncat(path, name, 8);
+	strncat(path,"/", 2);
+	strncat(path, filename, 64);
+	path_read_str(data, 32, path);
+}
+
+static inline int used(char *name)
+{
+	char disksize[64]="";
+	get_value(name, disksize, "disksize");
+	if (disksize[0] == '0')
+		return 0;
+	return 1;
+}
+
+static inline void value2devparm(char *name, char *data, char *filename)
+{
+	char path[64] = "/sys/block/";
+	strncat(path, name, 8);
+	strncat(path,"/", 2);
+	strncat(path, filename, 32);
+	path_write_str(data, path);
+}
+
+static inline void fill_table_row(struct libscols_table *tb, char *name)
+{
+	char disksize[32]="";
+	char orig_data_size[32]="";
+	char compr_data_size[32]="";
+	char comp_algorithm[16]="";
+	char max_comp_streams[32]="";
+	static struct libscols_line *ln;
+
+	enum {
+		COL_NAME,
+		COL_DISKSIZE,
+		COL_ORIG_DATA_SIZE,
+		COMPR_DATA_SIZE,
+		COMP_ALGORITHM,
+		MAX_COMP_STREAMS
+	};
+
+	get_value(name, disksize, "disksize");
+	get_value(name, orig_data_size, "orig_data_size");
+	get_value(name, compr_data_size, "compr_data_size");
+	get_value(name, comp_algorithm, "comp_algorithm");
+
+	if (strstr(comp_algorithm, "[lzo]") == NULL) {
+		if (strstr(comp_algorithm, "[lz4]") == NULL)
+			strncpy(comp_algorithm,"-", 2);
+		else
+			strncpy(comp_algorithm, "lz4", 4);
+	} else
+		strncpy(comp_algorithm, "lzo", 4);
+
+	get_value(name, max_comp_streams, "max_comp_streams");
+
+	ln = scols_table_new_line(tb, NULL);
+	scols_line_set_data(ln, COL_NAME, name);
+	scols_line_set_data(ln, COL_DISKSIZE, disksize);
+	scols_line_set_data(ln, COL_ORIG_DATA_SIZE, orig_data_size);
+	scols_line_set_data(ln, COMPR_DATA_SIZE, compr_data_size);
+	scols_line_set_data(ln, COMP_ALGORITHM, comp_algorithm);
+	scols_line_set_data(ln, MAX_COMP_STREAMS, max_comp_streams);
+}
+
+static inline void status(char *dev)
+{
+	struct libscols_table *tb;
+
+	tb = scols_new_table();
+	scols_table_new_column(tb, "NAME", 0, SCOLS_FL_RIGHT);
+	scols_table_new_column(tb, "DISKSIZE", 1, SCOLS_FL_RIGHT);
+	scols_table_new_column(tb, "ORIG", 2, SCOLS_FL_RIGHT);
+	scols_table_new_column(tb, "COMPRESS", 3, SCOLS_FL_RIGHT);
+	scols_table_new_column(tb, "ALG", 4, SCOLS_FL_RIGHT);
+	scols_table_new_column(tb, "THR", 5, SCOLS_FL_RIGHT);
+
+	if (dev != NULL) {
+		fill_table_row(tb, dev);
+	} else {
+		for (int i=0;;i++) {
+			char name[8] = "zram";
+			char num[4];
+
+			sprintf(num,"%i",i);
+			strncat(name, num, 8);
+
+			if(!zram_exist(name))
+				break;
+
+			if(!used(name))
+				continue;
+			fill_table_row(tb, name);
+		}
+	}
+
+	scols_print_table(tb);
+	scols_unref_table(tb);
+	EXIT_SUCCESS;
+}
+
+static inline char *find_free_zram(void)
+{
+	char *ret;
+	for (unsigned i=0;;i++) {
+		char name[8] = "zram";
+		char num[4];
+
+		sprintf(num,"%i",i);
+		strncat(name, num, 4);
+		if (!zram_exist(name))
+			break;
+
+		// Avoid warning: return adress of local variable
+		ret = name;
+
+		if (used(ret) == 0)
+			return ret;
+	}
+	errx(EXIT_FAILURE, _("All device already in use"));
+}
+
+
+
+static inline void usage(FILE *out)
+{
+	fputs(USAGE_HEADER, out);
+	fprintf(out, _(" %s [-d zram<N>|-f] -s <size> -a lz4|lzo -t 
<number>\n"), "zramctl");
+	fputs(USAGE_OPTIONS, out);
+	fputs(_(" -f, --find             find free device\n"), out);
+	fputs(_(" -d, --device <name>    specify device: zramX\n"), out);
+	fputs(_(" -r, --reset  <name>    reset specified device\n"), out);
+	fputs(_(" -s, --size <size>      device size: 131072, 1024M...\n"), out);
+	fputs(_(" -a, --alg lzo|lz4      compress algorithm\n"), out);
+	fputs(_(" -t, --threads <number> number of compress streams\n\n"), out);
+	fputs(_(" <no args>              return status of used devices\n"), out);
+	fputs(USAGE_SEPARATOR, out);
+	fputs(USAGE_HELP, out);
+	fputs(USAGE_VERSION, out);
+	fprintf(out, USAGE_MAN_TAIL("zramctl(8)"));
+	exit(out == stderr ? 1 : EXIT_SUCCESS);
+}
+
+int main(int argc, char **argv)
+{
+	int c = 0;
+	char *dev = NULL;
+	char *size = NULL; // zram disk size
+	char *alg = NULL;  // compress algorithm lzo || lz4
+	char *threads = NULL;
+	unsigned f = 0;
+
+	static const struct option longopts[] = {
+		{"find", no_argument, NULL, 'f'},
+		{"device", required_argument, NULL, 'd'},
+		{"size", required_argument, NULL, 's'},
+		{"alg", required_argument, NULL, 'a'},
+		{"threads", required_argument, NULL, 't'},
+		{"reset", required_argument, NULL, 'r'},
+		{"version", no_argument, NULL, 'V'},
+		{"help", no_argument, NULL, 'h'},
+		{NULL, 0, NULL, 0}
+	};
+
+	static const ul_excl_t excl[] = {
+	{ 'd', 'f' },
+	{ 0 }
+	};
+
+	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
+
+	setlocale(LC_ALL, "");
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	while ((c = getopt_long(argc, argv, "fd:s:a:t:r:Vh", longopts, NULL)) 
!= -1) {
+
+		err_exclusive_options(c, longopts, excl, excl_st);
+
+		switch (c) {
+		case 'f':
+			f = 1;
+			dev = find_free_zram();
+			break;
+		case 'd':
+			dev = optarg;
+			break;
+		case 's':
+			size = optarg;
+			break;
+		case 'a':
+			if (!strcmp(optarg,"lzo") || !strcmp(optarg,"lz4"))
+				alg = optarg;
+			else
+				errx(EXIT_FAILURE,
+				     _("%s: supported lzo or lz4 only"),
+				     optarg);
+			break;
+		case 't':
+			threads = optarg;
+			if (strtos64_or_err(threads, "-t <integer>") < 1) {
+				errx(EXIT_FAILURE,
+				     _("-t %s <- must be greater than zero"),
+				     threads);
+			}
+			break;
+		case 'r':
+			dev = optarg;
+			value2devparm(dev, "1", "reset");
+			break;
+		case 'V':
+			printf(UTIL_LINUX_VERSION);
+			return EXIT_SUCCESS;
+		case 'h':
+			usage(stdout);
+		default:
+			usage(stderr);
+		}
+	}
+
+	if (argc == 1)
+		status(dev);
+
+	if (argc == 3 && dev != NULL)
+		status(dev);
+
+	if (dev != NULL && size != NULL) {
+		value2devparm(dev, "1", "reset");
+		if (threads != NULL && strtos64_or_err(threads, "-t <integer>") > 1)
+			value2devparm(dev, threads, "max_comp_streams");
+
+		if (alg != NULL)
+			value2devparm(dev, alg, "comp_algorithm");
+
+		value2devparm(dev, size, "disksize");
+	}
+
+	if (dev != NULL && f > 0)
+		fprintf(stdout,_("%s\n"), dev);
+
+	return EXIT_SUCCESS;
+}
\ No newline at end of file

Re: [RFC] [Patch v2] Created zramctl

[Karel Zak]

On Sun, Jul 27, 2014 at 10:04:45PM +0300, Timofey Titovets wrote:
> I spend some time and rewrite zramctl for util-linux.

 Thanks! Merged.

> sysfs.h powerful, but i can't understand how working sysfs.h and how i can
> implement it(?), and i think what my helper function is easier now, than
> sysfs.h realization

 I have modified your code to use sysfs stuff and to use libsmartcols
 in better way. So now it's more compatible with another ls-like utils
 we have in util-linux. For example you can specify output columns by
 --output, it makes it more friendly for scripts etc.

 # /zramctl 
 NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
 /dev/zram1               100M  44K  876B   44K         /mnt/test
 /dev/zram2               123M   4K   78B    4K         [SWAP]
 /dev/zram3                10M   0B    0B    0B         


 Note that I have kernel 3.14, so things like number of compression
 streams and compression algorithm are invisible for me (I going to
 update and test it with new kernel this afternoon).


 Anyway, zRam seems like nice project. I'll try to add support to
 swapon next week. My idea is something like (in fstab):

  /dev/zram0  swap  swap  x-zram.disksize=100M,x-zram.algorithm=lzo,auto


 it means use x-* options (we use this notation for another things) to
 specify zram setting. The controversial thing is that we have to call
 mkswap from swapon, but it's already supported for swap
 reinitialization (after system resume or on page size change etc.)

 ... or is it over-engineering?

    Karel


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

Re: [RFC] [Patch v2] Created zramctl

[Timofey Titovets]

(i forget include util-linux to CC)
First of all, big thanks Karel for your great work, you are C wizard =)

2014-08-01 13:37 GMT+03:00 Karel Zak <kzak@redhat.com>:
> On Sun, Jul 27, 2014 at 10:04:45PM +0300, Timofey Titovets wrote:
>> I spend some time and rewrite zramctl for util-linux.
>
>  Thanks! Merged.
>
>> sysfs.h powerful, but i can't understand how working sysfs.h and how i can
>> implement it(?), and i think what my helper function is easier now, than
>> sysfs.h realization
>
>  I have modified your code to use sysfs stuff and to use libsmartcols
>  in better way. So now it's more compatible with another ls-like utils
>  we have in util-linux. For example you can specify output columns by
>  --output, it makes it more friendly for scripts etc.
>
>  # /zramctl
>  NAME       ALGORITHM DISKSIZE DATA COMPR TOTAL STREAMS MOUNTPOINT
>  /dev/zram1               100M  44K  876B   44K         /mnt/test
>  /dev/zram2               123M   4K   78B    4K         [SWAP]
>  /dev/zram3                10M   0B    0B    0B
>
Looks nice, mount point it's cool column, very useful *_*

>  Note that I have kernel 3.14, so things like number of compression
>  streams and compression algorithm are invisible for me (I going to
>  update and test it with new kernel this afternoon).
I test recent build from util-linux git, and all working good (archlinux 3.15.8)
I can't test it on 3.14, but on old kernels, what zramctl say if user
try to setup compression algorithm or threads count?
(may be on old kernel, zramctl must show warning and just ignore -a
and -t option?)

>
>  Anyway, zRam seems like nice project. I'll try to add support to
>  swapon next week. My idea is something like (in fstab):
>
>   /dev/zram0  swap  swap  x-zram.disksize=100M,x-zram.algorithm=lzo,auto
>
>
>  it means use x-* options (we use this notation for another things) to
>  specify zram setting. The controversial thing is that we have to call
>  mkswap from swapon, but it's already supported for swap
>  reinitialization (after system resume or on page size change etc.)
>
>  ... or is it over-engineering?
>
>     Karel
>
Do mkswap in swapon it good feature, i think what it changes can
simplify using zram in swap case, and not only for zram.
As example in gentoo manual for zram
(http://wiki.gentoo.org/wiki/Zram) this changes allow to clean several
lines in code, for sh scripts and udev rules.
Same true as for your idea with fstab.
In several tools, what setup zram as swap it also allow to clean some
part of code.
This is a cool thing, and i vote for this feature.


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

--
Best regards,
Timofey.