Re: git: 91bdebc958bb - main - bsdinstall/distfetch.c: check environment variables before downloading and handle memory allocation errors

I've backed this out as well... I'd thought that the style stuff had been
worked out...

And I'll leave the pull request closed since our newer guidance would
suggest this isn't enough of a win (I've been processing the older ones
under the old guidance on this point, but now I'm questioning that)... The
cleanups aren't that bad (moduo style), but they don't really move the
needle enough and would only be acceptable in the context of other
improvements (though even then it might be tough to justify).

Warner

On Tue, Apr 23, 2024 at 6:21 PM Jessica Clarke <jrtc27@freebsd.org> wrote:

> On 23 Apr 2024, at 23:42, Warner Losh <imp@FreeBSD.org> wrote:
> >
> > The branch main has been updated by imp:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=91bdebc958bb0da03f604bad19f99e3b10e96ac7
> >
> > commit 91bdebc958bb0da03f604bad19f99e3b10e96ac7
> > Author:     rilysh <nightquick@proton.me>
> > AuthorDate: 2024-04-23 22:40:19 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2024-04-23 22:42:38 +0000
> >
> >    bsdinstall/distfetch.c: check environment variables before
> downloading and handle memory allocation errors
> >
> >    1. Currently, distfetch checks environment variables existence
> >    when it will use them or in a case (in chdir()) it doesn't check
> >    at all. As they are necessary to set before doing anything with
> >    it, check them, if they set or not, before proceeding any further.
> >    This also avoids extra cleaning when that environment variable
> >    isn't set.
> >
> >    2. Handle memory allocation error in malloc(PATH_MAX) and replace
> >    (sizeof const char *) with (sizeof char *). Both are similar and
> >    const doesn't have a size.
>
> Latter is fine I guess but they’re the same by definition, it’s churn.
>
> >    3. Indent the error message a bit in chdir().
>
> AKA violate style(9).
>
> >    Signed-off-by: rilysh <nightquick@proton.me>
> >    Reviewed by: imp
> >    Pull Request: https://github.com/freebsd/freebsd-src/pull/1071
> > ---
> > usr.sbin/bsdinstall/distfetch/distfetch.c | 33
> +++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/usr.sbin/bsdinstall/distfetch/distfetch.c
> b/usr.sbin/bsdinstall/distfetch/distfetch.c
> > index c431e187799d..094929d89ea1 100644
> > --- a/usr.sbin/bsdinstall/distfetch/distfetch.c
> > +++ b/usr.sbin/bsdinstall/distfetch/distfetch.c
> > @@ -46,7 +46,7 @@ static int fetch_files(int nfiles, char **urls);
> > int
> > main(void)
> > {
> > - char *diststring;
> > + char *diststring, *dists, *distdir, *distsite;
> > char **urls;
> > int i;
> > int ndists = 0;
> > @@ -54,17 +54,24 @@ main(void)
> > char error[PATH_MAX + 512];
> > struct bsddialog_conf conf;
> >
> > - if (getenv("DISTRIBUTIONS") == NULL)
> > + if ((dists = getenv("DISTRIBUTIONS")) == NULL)
> > errx(EXIT_FAILURE, "DISTRIBUTIONS variable is not set");
> >
> > - diststring = strdup(getenv("DISTRIBUTIONS"));
> > + if ((distdir = getenv("BSDINSTALL_DISTDIR")) == NULL)
> > + errx(EXIT_FAILURE, "BSDINSTALL_DISTDIR variable is not set");
> > +
> > + if ((distsite = getenv("BSDINSTALL_DISTSITE")) == NULL)
> > + errx(EXIT_FAILURE, "BSDINSTALL_DISTSITE variable is not set");
> > +
> > + if ((diststring = strdup(dists)) == NULL)
> > + errx(EXIT_FAILURE, "Error: diststring variable out of memory!");
> > +
> > for (i = 0; diststring[i] != 0; i++)
> > if (isspace(diststring[i]) && !isspace(diststring[i+1]))
> > ndists++;
> > ndists++; /* Last one */
> >
> > - urls = calloc(ndists, sizeof(const char *));
> > - if (urls == NULL) {
> > + if ((urls = calloc(ndists, sizeof(char *))) == NULL) {
>
> Moving into the if is pointless.
>
> > free(diststring);
> > errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!");
> > }
> > @@ -78,15 +85,21 @@ main(void)
> > bsddialog_backtitle(&conf, OSNAME " Installer");
> >
> > for (i = 0; i < ndists; i++) {
> > - urls[i] = malloc(PATH_MAX);
> > + if ((urls[i] = malloc(PATH_MAX)) == NULL) {
>
> Moving into the if is pointless.
>
> > + free(urls);
> > + free(diststring);
>
> Like I said in the review, reviewing urls and diststring but not the
> elements of urls is a stupid halfway house, and it’s all a waste of
> time when you’re about to call errx. If one wants to be super pedantic
> and free memory prior to calling errx, fine, but do it in full, don’t
> do an arbitrary subset.
>
> > + bsddialog_end();
> > + errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!");
> > + }
> > +
> > snprintf(urls[i], PATH_MAX, "%s/%s",
> > -    getenv("BSDINSTALL_DISTSITE"), strsep(&diststring, " \t"));
> > + distsite, strsep(&diststring, " \t"));
>
> Breaks style(9).
>
> > }
> >
> > - if (chdir(getenv("BSDINSTALL_DISTDIR")) != 0) {
> > + if (chdir(distdir) != 0) {
> > snprintf(error, sizeof(error),
> > -    "Could not change to directory %s: %s\n",
> > -    getenv("BSDINSTALL_DISTDIR"), strerror(errno));
> > + "Could not change to directory %s: %s\n",
> > + distdir, strerror(errno));
>
> Breaks style(9).
>
> Jess
>
> > conf.title = "Error";
> > bsddialog_msgbox(&conf, error, 0, 0);
> > bsddialog_end();
>
>