Friendly ping
On Fri, Oct 29, 2021 at 10:06:44AM +0200, Martin Vahlensieck wrote:
> Hi
>
> Here are some small changes to quotaon(8). If you want I can split
> them up, but since they are small I am sending one diff. Here is
> a list of changes roughly in the order they appear in the diff:
>
> - Constify some function arguments
>
> - Use __progname instead of separate whoami variable + small KNF
> where the line is touched anyways.
>
> - Order letters in getopt(3) call
>
> - Convert a fprintf(3) + perror(3) to warn(3). warn(3) is
> already used in this function for similar purposes.
>
> - Replace strtok(3) with strsep(3). Is there a more
> elegant way than my solution? (I took the freedom to collect all
> the char * variables into one line).
>
> - Set cp to NULL at the end of the while loop scanning the mount
> options. Otherwise if the last mount option contains a '=' the
> part after it is used as the quota file (of course only if the
> desired userquota/groupquota option isn't found).
>
> - Use strncmp(3) instead of memcmp(3). Looking at the kernel code
> it seems that it is zero terminated. The other code nearby uses
> strcmp(3) already.
>
> - Invert an if condition to prevent an empty body.
>
> Happy to hear feedback, also feel free to only commit parts of it.
>
> Best,
>
> Martin
>
> P.S.: If the prototypes are touched anyways, the names of the
> arguments might be removed as well to comply with KNF. Let me
> know if you want that.
>
> Index: quotaon.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/quotaon/quotaon.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 quotaon.c
> --- quotaon.c 26 Apr 2018 12:42:51 -0000 1.27
> +++ quotaon.c 29 Oct 2021 07:46:25 -0000
> @@ -44,6 +44,8 @@
> #include <fstab.h>
> #include <unistd.h>
>
> +extern char *__progname;
> +
> char *qfname = QUOTAFILENAME;
> char *qfextension[] = INITQFNAMES;
>
> @@ -52,34 +54,31 @@ int gflag; /* operate on group quotas *
> int uflag; /* operate on user quotas */
> int vflag; /* verbose */
>
> -void usage(char *whoami);
> -int hasquota(struct fstab *fs, int type, char **qfnamep, int force);
> -int quotaonoff(struct fstab *fs, int offmode, int type, char *qfpathname);
> -int oneof(char *target, char *list[], int cnt);
> -int readonly(struct fstab *fs);
> +void usage();
> +int hasquota(const struct fstab *fs, int type, char **qfnamep, int force);
> +int quotaonoff(const struct fstab *fs, int offmode, int type, char
> *qfpathname);
> +int oneof(const char *target, char *list[], int cnt);
> +int readonly(const struct fstab *fs);
>
>
> int
> main(int argc, char *argv[])
> {
> struct fstab *fs;
> - char *qfnp, *whoami;
> + char *qfnp;
> long argnum, done = 0;
> int i, offmode = 0, errs = 0;
> extern int optind;
> int ch;
>
> - whoami = strrchr(*argv, '/') + 1;
> - if (whoami == (char *)1)
> - whoami = *argv;
> - if (strcmp(whoami, "quotaoff") == 0)
> + if (strcmp(__progname, "quotaoff") == 0)
> offmode = 1;
> - else if (strcmp(whoami, "quotaon") != 0) {
> + else if (strcmp(__progname, "quotaon") != 0) {
> fprintf(stderr, "Name must be quotaon or quotaoff not %s\n",
> - whoami);
> + __progname);
> exit(1);
> }
> - while ((ch = getopt(argc, argv, "avug")) != -1) {
> + while ((ch = getopt(argc, argv, "aguv")) != -1) {
> switch (ch) {
> case 'a':
> aflag = 1;
> @@ -94,13 +93,13 @@ main(int argc, char *argv[])
> vflag = 1;
> break;
> default:
> - usage(whoami);
> + usage();
> }
> }
> argc -= optind;
> argv += optind;
> if (argc <= 0 && !aflag)
> - usage(whoami);
> + usage();
> if (!gflag && !uflag) {
> gflag = 1;
> uflag = 1;
> @@ -142,22 +141,20 @@ main(int argc, char *argv[])
> }
>
> void
> -usage(char *whoami)
> +usage()
> {
> -
> - fprintf(stderr, "usage: %s [-aguv] filesystem ...\n", whoami);
> + fprintf(stderr, "usage: %s [-aguv] filesystem ...\n", __progname);
> exit(1);
> }
>
> int
> -quotaonoff(struct fstab *fs, int offmode, int type, char *qfpathname)
> +quotaonoff(const struct fstab *fs, int offmode, int type, char *qfpathname)
> {
> if (strcmp(fs->fs_file, "/") && readonly(fs))
> return (1);
> if (offmode) {
> if (quotactl(fs->fs_file, QCMD(Q_QUOTAOFF, type), 0, 0) < 0) {
> - fprintf(stderr, "quotaoff: ");
> - perror(fs->fs_file);
> + warn("%s", fs->fs_file);
> return (1);
> }
> if (vflag)
> @@ -180,7 +177,7 @@ quotaonoff(struct fstab *fs, int offmode
> * Check to see if target appears in list of size cnt.
> */
> int
> -oneof(char *target, char *list[], int cnt)
> +oneof(const char *target, char *list[], int cnt)
> {
> int i;
>
> @@ -194,10 +191,9 @@ oneof(char *target, char *list[], int cn
> * Check to see if a particular quota is to be enabled.
> */
> int
> -hasquota(struct fstab *fs, int type, char **qfnamep, int force)
> +hasquota(const struct fstab *fs, int type, char **qfnamep, int force)
> {
> - char *opt;
> - char *cp;
> + char *opt, *cp, *pbuf;
> static char initname, usrname[100], grpname[100];
> static char buf[BUFSIZ];
>
> @@ -209,13 +205,15 @@ hasquota(struct fstab *fs, int type, cha
> initname = 1;
> }
> strlcpy(buf, fs->fs_mntops, sizeof buf);
> - for (opt = strtok(buf, ","); opt; opt = strtok(NULL, ",")) {
> + pbuf = buf;
> + while ((opt = strsep(&pbuf, ","))) {
> if ((cp = strchr(opt, '=')) != NULL)
> *cp++ = '\0';
> if (type == USRQUOTA && strcmp(opt, usrname) == 0)
> break;
> if (type == GRPQUOTA && strcmp(opt, grpname) == 0)
> break;
> + cp = NULL;
> }
> if (!force && !opt)
> return (0);
> @@ -234,17 +232,15 @@ hasquota(struct fstab *fs, int type, cha
> * MFS is special -- it puts "mfs:" in the kernel's mount table
> */
> int
> -readonly(struct fstab *fs)
> +readonly(const struct fstab *fs)
> {
> struct statfs fsbuf;
>
> if (statfs(fs->fs_file, &fsbuf) < 0 ||
> strcmp(fsbuf.f_mntonname, fs->fs_file) ||
> strcmp(fsbuf.f_mntfromname, fs->fs_spec)) {
> - if (strcmp(fs->fs_file, "mfs") ||
> - memcmp(fsbuf.f_mntfromname, "mfs:", sizeof("mfs:")-1))
> - ;
> - else {
> + if (strcmp(fs->fs_file, "mfs") == 0 &&
> + strncmp(fsbuf.f_mntfromname, "mfs:", 4) == 0) {
> printf("%s: not mounted\n", fs->fs_file);
> return (1);
> }
>
