Dave, thanks for comments, I'll think how to fix, what you say. 2014-07-23 20:43 GMT+03:00 Dave Reisner <d@xxxxxxxxxxxxxx>: > 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@xxxxxxxxx> >> +.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@xxxxxxxxx> >> + * >> + * 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@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards, Timofey. -- To unsubscribe from this list: send the line "unsubscribe util-linux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
