Skip to contentSkip to conversation

Zram handle util

RFC Superseded Timofey Titovets 7 participants 16 messages Jul 19, 2014 – Aug 2, 2014 1 message not in the archive Util-Linux package

lore
Share & tools
lore
#0 lore raw

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?

#1 lore raw

On 19 July 2014 18:28, Timofey Titovets <[email protected]> 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.

Quoted 3 lines

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

Sounds potentially useful.

Quoted 11 lines

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/[email protected]/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.

Signature · 2 lines
Sami Kerola
http://www.iki.fi/kerolasa/

A message that is not in the archive (sent off-list or not yet received): CAGqmi765JkidXDpi6bP2qUk5U7Xry5nF7y0WY4Y6M_Fq8Eiqeg@mail.gmail.com

#3 lore raw

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.

Quoted 6 lines

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
Quoted 9 lines

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.

#4 lore raw
In reply to a message not in the archive

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.

Quoted 6 lines

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
Quoted 9 lines

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.

#5 lore raw
In reply to Timofey Titovets
Quoted 14 lines · 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 ...

Quoted 2 lines

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

#6 lore raw
Quoted 15 lines · Davidlohr Bueso
Quoted 14 lines · 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 ...

Quoted 6 lines
Quoted 2 lines

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.

Quoted 4 lines

Thanks,
Davidlohr

maintainer
#7 lore raw
Quoted 5 lines · Minchan Kim

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

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
#8 lore raw

RFC PATCH 1/1 Created zramctl

In reply to 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(+)
sys-utils/Makemodule.am +5 -0
index 68fd030..5d70c51 100644
@@ -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
sys-utils/zramctl.8 new file +92 -0

92 lines not on this page · raw message

sys-utils/zramctl.c new file +319 -0

320 lines not on this page · raw message

#9 lore raw

Re: [RFC] [Patch] Created zramctl

Quoted 11 lines · 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

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.

Quoted 170 lines

----

 sys-utils/Makemodule.am |   5 +
 sys-utils/zramctl.8     |  92 ++++++++++++++
 sys-utils/zramctl.c     | 319
++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 416 insertions(+)
sys-utils/Makemodule.am +5 -0
index 68fd030..5d70c51 100644
@@ -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
sys-utils/zramctl.8 new file +50 -0
new file mode 100644
index 0000000..4da209d
@@ -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

sys-utils/zramctl.8 new file +38 -0
+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 <[email protected]>
+.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/.
sys-utils/zramctl.c new file +41 -0
new file mode 100644
index 0000000..6ddee9b
@@ -0,0 +1,319 @@
+/*
+ * zramctl - purpose of it
+ *
+ * Copyright (c) 20nn  Example Commercial, Inc
+ * Written by Timofey Titovets <[email protected]>
+ *
+ * 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;

sys-utils/zramctl.c line 51

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+	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.

Quoted 17 lines
sys-utils/zramctl.c new file +17 -0
+	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;

sys-utils/zramctl.c line 78

Quoted 10 lines
sys-utils/zramctl.c new file +10 -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.

Quoted 4 lines
sys-utils/zramctl.c new file +4 -0
+
+	if (disksize[0] == '0')
+		return 0;
+

sys-utils/zramctl.c line 92

Quoted 10 lines
sys-utils/zramctl.c new file +10 -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...

Quoted 5 lines
sys-utils/zramctl.c new file +5 -0
+}
+
+static inline int write_file(char *path, char *data)
+{
+	FILE *file;

sys-utils/zramctl.c line 107

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+	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.

sys-utils/zramctl.c line 117

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+}
+
+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

sys-utils/zramctl.c line 128

Quoted 11 lines
sys-utils/zramctl.c new file +11 -0
+
+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.

Quoted 81 lines
sys-utils/zramctl.c new file +81 -0
+	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);

sys-utils/zramctl.c line 219

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+	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?

Quoted 20 lines
sys-utils/zramctl.c new file +20 -0
+	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);
+

sys-utils/zramctl.c line 248

Quoted 10 lines
sys-utils/zramctl.c new file +9 -1
+	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.

Quoted 6 lines
sys-utils/zramctl.c new file +6 -0
+			break;
+		case 'd':
+			if (dev != NULL) {
+				fprintf(stderr,_("-f || -d <name>\n"));
+				return 1;
+			}

sys-utils/zramctl.c line 264

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+			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.

sys-utils/zramctl.c line 269

Quoted 5 lines
sys-utils/zramctl.c new file +5 -0
+				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!)

sys-utils/zramctl.c line 270

Quoted 1 line
sys-utils/zramctl.c new file +1 -0
+			if (atoi(threads) < 1) {

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

Quoted 54 lines
sys-utils/zramctl.c new file +49 -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
--

To unsubscribe from this list: send the line "unsubscribe util-linux" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

Re: [RFC] [Patch] Created zramctl

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

2014-07-23 20:43 GMT+03:00 Dave Reisner <[email protected]>:

Quoted 515 lines · Timofey Titovets
Quoted 11 lines

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.

Quoted 170 lines

----

 sys-utils/Makemodule.am |   5 +
 sys-utils/zramctl.8     |  92 ++++++++++++++
 sys-utils/zramctl.c     | 319
++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 416 insertions(+)
sys-utils/Makemodule.am +5 -0
index 68fd030..5d70c51 100644
@@ -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
sys-utils/zramctl.8 new file +50 -0
new file mode 100644
index 0000000..4da209d
@@ -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

sys-utils/zramctl.8 new file +38 -0
+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 <[email protected]>
+.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/.
sys-utils/zramctl.c new file +41 -0
new file mode 100644
index 0000000..6ddee9b
@@ -0,0 +1,319 @@
+/*
+ * zramctl - purpose of it
+ *
+ * Copyright (c) 20nn  Example Commercial, Inc
+ * Written by Timofey Titovets <[email protected]>
+ *
+ * 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;

sys-utils/zramctl.c line 51

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+     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.

Quoted 17 lines
sys-utils/zramctl.c new file +17 -0
+     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;

sys-utils/zramctl.c line 78

Quoted 10 lines
sys-utils/zramctl.c new file +10 -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.

Quoted 4 lines
sys-utils/zramctl.c new file +4 -0
+
+     if (disksize[0] == '0')
+             return 0;
+

sys-utils/zramctl.c line 92

Quoted 10 lines
sys-utils/zramctl.c new file +10 -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...

Quoted 5 lines
sys-utils/zramctl.c new file +5 -0
+}
+
+static inline int write_file(char *path, char *data)
+{
+     FILE *file;

sys-utils/zramctl.c line 107

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+     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.

sys-utils/zramctl.c line 117

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+}
+
+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

sys-utils/zramctl.c line 128

Quoted 11 lines
sys-utils/zramctl.c new file +11 -0
+
+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.

Quoted 81 lines
sys-utils/zramctl.c new file +81 -0
+     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);

sys-utils/zramctl.c line 219

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+     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?

Quoted 20 lines
sys-utils/zramctl.c new file +20 -0
+     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);
+

sys-utils/zramctl.c line 248

Quoted 10 lines
sys-utils/zramctl.c new file +9 -1
+     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.

Quoted 6 lines
sys-utils/zramctl.c new file +6 -0
+                     break;
+             case 'd':
+                     if (dev != NULL) {
+                             fprintf(stderr,_("-f || -d <name>\n"));
+                             return 1;
+                     }

sys-utils/zramctl.c line 264

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+                     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.

sys-utils/zramctl.c line 269

Quoted 5 lines
sys-utils/zramctl.c new file +5 -0
+                             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!)

Quoted 1 line

+ if (atoi(threads) < 1) {

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

Quoted 54 lines
sys-utils/zramctl.c new file +49 -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
--

To unsubscribe from this list: send the line "unsubscribe util-linux" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines
Best regards,
Timofey.
maintainer
#11 lore raw

Re: [RFC] [Patch] Created zramctl

In reply to Dave Reisner
Quoted 5 lines · Dave Reisner

sys-utils/zramctl.c line 128

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	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

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
maintainer
#12 lore raw

Re: [RFC] [Patch] Created zramctl

In reply to Timofey Titovets
Quoted 377 lines · 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(+)
sys-utils/Makemodule.am +5 -0
index 68fd030..5d70c51 100644
@@ -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
sys-utils/zramctl.8 new file +50 -0
new file mode 100644
index 0000000..4da209d
@@ -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

sys-utils/zramctl.8 new file +38 -0
+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 <[email protected]>
+.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/.
sys-utils/zramctl.c new file +237 -0
new file mode 100644
index 0000000..6ddee9b
@@ -0,0 +1,319 @@
+/*
+ * zramctl - purpose of it
+ *
+ * Copyright (c) 20nn  Example Commercial, Inc
+ * Written by Timofey Titovets <[email protected]>
+ *
+ * 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);

sys-utils/zramctl.c line 246

Quoted 10 lines
sys-utils/zramctl.c new file +9 -1
+	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()).

Quoted 14 lines
sys-utils/zramctl.c new file +14 -0
+			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':

sys-utils/zramctl.c line 270

Quoted 10 lines
sys-utils/zramctl.c new file +10 -0
+			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

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
#13 lore raw

Re: [RFC] [Patch] Created zramctl

In reply to Timofey Titovets
Quoted 2 lines · Timofey Titovets

+.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 ]

sys-utils/zramctl.8 line 43

Quoted 3 lines

+.B zramctl

sys-utils/zramctl.8 new file +2 -0
+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."

Quoted 3 lines

+.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.

Quoted 2 lines

++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.

Quoted 2 lines

+.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.

Quoted 2 lines

+.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.

Quoted 2 lines

+ * Copyright (c) 20nn Example Commercial, Inc
+ * Written by Timofey Titovets <[email protected]>

This should probably just say:

* Copyright (c) 2014 Timofey Titovets <[email protected]>

sys-utils/zramctl.c line 101

Quoted 3 lines
sys-utils/zramctl.c new file +3 -0
+	file = fopen(p, "w");
+	if (file == NULL) {
+		fprintf(stderr,_("can't write to %s\n"), path);

That should say: "cannot open %s".

sys-utils/zramctl.c line 104

Quoted 3 lines
sys-utils/zramctl.c new file +3 -0
+		return -1;
+	}
+	fwrite(data, strlen(data), 1, file);

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

sys-utils/zramctl.c line 165

Quoted 3 lines
sys-utils/zramctl.c new file +3 -0
+	return 0;
+	goto fail;
+fail:

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

sys-utils/zramctl.c line 167

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	printf("Zram module not loaded");
+	return -1;

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

sys-utils/zramctl.c line 198

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	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")

sys-utils/zramctl.c line 200

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	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."

sys-utils/zramctl.c line 203

Quoted 3 lines
sys-utils/zramctl.c new file +3 -0
+	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>/

sys-utils/zramctl.c line 204

Quoted 1 line
sys-utils/zramctl.c new file +1 -0
+	fputs(_(" -s, --size [size]     device size: 131072, 1024M...\n"), out);

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

sys-utils/zramctl.c line 205

Quoted 1 line
sys-utils/zramctl.c new file +1 -0
+	fputs(_(" -a, --alg [lzo|lz4]   compress algorithm\n"), out);

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

sys-utils/zramctl.c line 206

Quoted 1 line
sys-utils/zramctl.c new file +1 -0
+	fputs(_(" -t, --threads [0<num] number of compress streams\n"), out);

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

Review

Quoted 2 lines
(quoted diff) +2 -0
+			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".

sys-utils/zramctl.c line 264

Quoted 5 lines
sys-utils/zramctl.c new file +5 -0
+		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"

sys-utils/zramctl.c line 295

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	if (!strcmp(dev, "USED")) {
+		fprintf(stderr,_("all device already used\n") );

"all available devices are in use"

sys-utils/zramctl.c line 316

Quoted 2 lines
sys-utils/zramctl.c new file +2 -0
+	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

Signature · 1 line
http://www.fastmail.fm - A no graphics, no pop-ups email service
#14 lore raw

RFC PATCH v2 1/1 Created zramctl

In reply to 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(+)
configure.ac +8 -0
index aae2456..f3204d9 100644
@@ -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]),
sys-utils/Makemodule.am +8 -0
index 4741fed..c6a2c5a 100644
@@ -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
sys-utils/zramctl.8 new file +98 -0

98 lines not on this page · raw message

sys-utils/zramctl.c new file +302 -0

303 lines not on this page · raw message

maintainer
#15 lore raw

Re: [RFC] [Patch v2] Created zramctl

Quoted 1 line · Timofey Titovets

I spend some time and rewrite zramctl for util-linux.

Thanks! Merged.

Quoted 3 lines

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

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
#16 lore raw

Re: [RFC] [Patch v2] Created zramctl

(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 <[email protected]>:

Quoted 14 lines · Timofey Titovets
Quoted 1 line

I spend some time and rewrite zramctl for util-linux.

Thanks! Merged.

Quoted 3 lines

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.

Quoted 6 lines
 # /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 *_*

Quoted 3 lines

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?)

Quoted 7 lines

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

Quoted 9 lines

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.

Quoted 3 lines

--
Karel Zak <[email protected]>
http://karelzak.blogspot.com

--
Best regards,
Timofey.