On Thu, Aug 18, 2022 at 11:05:52AM -0600, Todd C. Miller wrote:
> On Thu, 18 Aug 2022 11:51:19 -0000, Klemens Nanni wrote:
>
> > I've checked all usage combination of flags and arguments, the new code
> > does what the new synopsis says.
>
> I agree with the general direction but have one concern. See inline.
>
> - todd
>
> > Index: installboot.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/installboot/installboot.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 installboot.c
> > --- installboot.c 20 Jul 2021 14:51:56 -0000 1.14
> > +++ installboot.c 18 Aug 2022 11:42:43 -0000
> > @@ -31,17 +31,16 @@ int prepare;
> > int stages;
> > int verbose;
> >
> > -char *root = "/";
> > +char *root;
>
> If you don't initalize root here, a NULL pointer will be used later
> when the -v option is not also used. You could also kill the useless
> strdup of optarg when the -r option is used.
Of course, thanks.
>
> > char *stage1;
> > char *stage2;
> >
> > static __dead void
> > usage(void)
> > {
> > - extern char *__progname;
> > -
> > - fprintf(stderr, "usage: %s [-npv] [-r root] disk [stage1%s]\n",
> > - __progname, (stages >= 2) ? " [stage2]" : "");
> > + fprintf(stderr, "usage:\t%1$s [-nv] [-r root] disk [stage1%2$s]\n"
> > + "\t%1$s [-nv] -p disk\n",
> > + getprogname(), (stages >= 2) ? " [stage2]" : "");
> >
> > exit(1);
> > }
> > @@ -80,6 +79,8 @@ main(int argc, char **argv)
> >
> > if (argc < 1 || argc > stages + 1)
> > usage();
> > + if (prepare && (root != NULL || argc > 1))
> > + usage();
> >
> > dev = argv[0];
> > if (argc > 1)
> > @@ -87,9 +88,21 @@ main(int argc, char **argv)
> > if (argc > 2)
> > stage2 = argv[2];
> >
> > + if ((devfd = opendev(dev, (nowrite ? O_RDONLY : O_RDWR), OPENDEV_PART,
> > + &realdev)) == -1)
> > + err(1, "open: %s", realdev);
> > +
> > + if (prepare) {
> > + md_prepareboot(devfd, realdev);
> > + return 0;
> > + }
> > +
> > /* Prefix stages with root, unless they were user supplied. */
> > - if (verbose)
> > + if (verbose) {
> > + if (root == NULL)
> > + root = "/";
> > fprintf(stderr, "Using %s as root\n", root);
> > + }
> > if (argc <= 1 && stage1 != NULL) {
> > stage1 = fileprefix(root, stage1);
>
> With your diff root may be NULL when calling fileprefix().
root is now initalised before the verbose check so it is independent of
-v usage and happens after prepare code, indicating that -p does not
care about -r.
>
> > if (stage1 == NULL)
> > @@ -101,10 +114,6 @@ main(int argc, char **argv)
> > exit(1);
> > }
> >
> > - if ((devfd = opendev(dev, (nowrite ? O_RDONLY : O_RDWR), OPENDEV_PART,
> > - &realdev)) == -1)
> > - err(1, "open: %s", realdev);
> > -
> > if (verbose) {
> > fprintf(stderr, "%s bootstrap on %s\n",
> > (nowrite ? "would install" : "installing"), realdev);
> > @@ -118,11 +127,6 @@ main(int argc, char **argv)
> > }
> >
> > md_loadboot();
> > -
> > - if (prepare) {
> > - md_prepareboot(devfd, realdev);
> > - return 0;
> > - }
> >
> > #ifdef SOFTRAID
> > sr_installboot(devfd, dev);
> >
>
Index: installboot.8
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/installboot.8,v
retrieving revision 1.5
diff -u -p -r1.5 installboot.8
--- installboot.8 20 Jul 2021 14:51:56 -0000 1.5
+++ installboot.8 18 Aug 2022 11:32:51 -0000
@@ -22,10 +22,14 @@
.Nd install bootstrap on a disk
.Sh SYNOPSIS
.Nm installboot
-.Op Fl npv
+.Op Fl nv
.Op Fl r Ar root
.Ar disk
.Op Ar stage1 Op Ar stage2
+.Nm
+.Op Fl nv
+.Fl p
+.Ar disk
.Sh DESCRIPTION
.Nm
installs bootstrap on the specified disk.
@@ -38,7 +42,7 @@ the beginning of the disk.
The options are as follows:
.Bl -tag -width Ds
.It Fl n
-Perform a dry run - do not actually write any bootstrap to the disk.
+Perform a dry run - do not actually write to the disk.
.It Fl p
Prepare filesystem.
This will create a new filesystem on the partition reserved for the
Index: installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/installboot.c,v
retrieving revision 1.14
diff -u -p -r1.14 installboot.c
--- installboot.c 20 Jul 2021 14:51:56 -0000 1.14
+++ installboot.c 18 Aug 2022 18:51:45 -0000
@@ -31,17 +31,16 @@ int prepare;
int stages;
int verbose;
-char *root = "/";
+char *root;
char *stage1;
char *stage2;
static __dead void
usage(void)
{
- extern char *__progname;
-
- fprintf(stderr, "usage: %s [-npv] [-r root] disk [stage1%s]\n",
- __progname, (stages >= 2) ? " [stage2]" : "");
+ fprintf(stderr, "usage:\t%1$s [-nv] [-r root] disk [stage1%2$s]\n"
+ "\t%1$s [-nv] -p disk\n",
+ getprogname(), (stages >= 2) ? " [stage2]" : "");
exit(1);
}
@@ -63,9 +62,7 @@ main(int argc, char **argv)
prepare = 1;
break;
case 'r':
- root = strdup(optarg);
- if (root == NULL)
- err(1, "strdup");
+ root = optarg;
break;
case 'v':
verbose = 1;
@@ -80,6 +77,8 @@ main(int argc, char **argv)
if (argc < 1 || argc > stages + 1)
usage();
+ if (prepare && (root != NULL || argc > 1))
+ usage();
dev = argv[0];
if (argc > 1)
@@ -87,7 +86,18 @@ main(int argc, char **argv)
if (argc > 2)
stage2 = argv[2];
+ if ((devfd = opendev(dev, (nowrite ? O_RDONLY : O_RDWR), OPENDEV_PART,
+ &realdev)) == -1)
+ err(1, "open: %s", realdev);
+
+ if (prepare) {
+ md_prepareboot(devfd, realdev);
+ return 0;
+ }
+
/* Prefix stages with root, unless they were user supplied. */
+ if (root == NULL)
+ root = "/";
if (verbose)
fprintf(stderr, "Using %s as root\n", root);
if (argc <= 1 && stage1 != NULL) {
@@ -101,10 +111,6 @@ main(int argc, char **argv)
exit(1);
}
- if ((devfd = opendev(dev, (nowrite ? O_RDONLY : O_RDWR), OPENDEV_PART,
- &realdev)) == -1)
- err(1, "open: %s", realdev);
-
if (verbose) {
fprintf(stderr, "%s bootstrap on %s\n",
(nowrite ? "would install" : "installing"), realdev);
@@ -118,11 +124,6 @@ main(int argc, char **argv)
}
md_loadboot();
-
- if (prepare) {
- md_prepareboot(devfd, realdev);
- return 0;
- }
#ifdef SOFTRAID
sr_installboot(devfd, dev);
