Re: ftp(1): separate file:/ URL handling
On Thu, Dec 26, 2019 at 10:53:45AM +0100, Jeremie Courreges-Anglas wrote:
> On Thu, Dec 19 2019, Jeremie Courreges-Anglas wrote:
> > A bit late...
> >
> > Move file: URL handling into its own function. This simplifies
> > url_get() and would have prevented problems with bogus redirections.
> >
> > file_get() unrolls the code previously used in url_get(), except the
> > #ifndef SMALL bits were stripped out. file: support is mainly
> > (only?) used in the installer which is built with SMALL defined.
> > Resuming an incomplete file: transfer sounds nuts.
> >
> > I felt a bit guilty about copying dubious code, there are some changes
> > that ought to be applied to url_get() too:
> > - write(2) can't return 0, can it? (something old about non-blocking
> > sockets maybe?). Anyway, no need to handle 0 explicitely.
> > - allocate buf before setjmp instead of marking it volatile
> > - save_errno/warnc dance if read(2) fails
> >
> > This survived make release on amd64 and a bsd.rd upgrade with sets
> > on 'disk'. The resulting ftp(1) binary size decreases.
> >
> > Comments/ok?
>
> Still looking for feedback, else I'll resort to crowdtesting. ;)
>
> >
> > Index: fetch.c
> > ===
> > --- fetch.c.orig
> > +++ fetch.c
> > @@ -68,6 +68,7 @@ struct tls;
> > #include "ftp_var.h"
> > #include "cmds.h"
> >
> > +static int file_get(const char *, const char *);
> > static int url_get(const char *, const char *, const char *, int);
> > static int save_chunked(FILE *, struct tls *, int , char *, size_t);
> > static voidaborthttp(int);
> > @@ -182,6 +183,125 @@ tooslow(int signo)
> > }
> >
> > /*
> > + * Copy a local file (used by the OpenBSD installer).
> > + * Returns -1 on failure, 0 on success
> > + */
> > +static int
> > +file_get(const char *path, const char *outfile)
> > +{
> > + struct stat st;
> > + int fd, out, rval = -1, save_errno;
> > + volatile sig_t oldintr, oldinti;
> > + const char *savefile;
> > + char*buf = NULL, *cp;
> > + const size_t buflen = 128 * 1024;
> > + off_thashbytes;
> > + ssize_t len, wlen;
> > +
> > + direction = "received";
> > +
> > + fd = open(path, O_RDONLY);
> > + if (fd == -1) {
> > + warn("Can't open file %s", path);
> > + return -1;
> > + }
> > +
> > + if (fstat(fd, &st) == -1)
> > + filesize = -1;
> > + else
> > + filesize = st.st_size;
> > +
> > + if (outfile != NULL)
> > + savefile = outfile;
> > + else {
> > + if (path[strlen(path) - 1] == '/') /* Consider no file */
> > + savefile = NULL;/* after dir invalid. */
> > + else
> > + savefile = basename(path);
> > + }
> > +
> > + if (EMPTYSTRING(savefile)) {
> > + warnx("No filename after directory (use -o): %s", path);
> > + goto cleanup_copy;
> > + }
> > +
> > + /* Open the output file. */
> > + if (!pipeout) {
> > + out = open(savefile, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> > + if (out == -1) {
> > + warn("Can't open %s", savefile);
> > + goto cleanup_copy;
> > + }
> > + } else
> > + out = fileno(stdout);
> > +
> > + if ((buf = malloc(buflen)) == NULL)
> > + errx(1, "Can't allocate memory for transfer buffer");
> > +
> > + /* Trap signals */
> > + oldintr = NULL;
> > + oldinti = NULL;
> > + if (setjmp(httpabort)) {
> > + if (oldintr)
> > + (void)signal(SIGINT, oldintr);
> > + if (oldinti)
> > + (void)signal(SIGINFO, oldinti);
> > + goto cleanup_copy;
> > + }
> > + oldintr = signal(SIGINT, abortfile);
> > +
> > + bytes = 0;
> > + hashbytes = mark;
> > + progressmeter(-1, path);
> > +
> > + /* Finally, suck down the file. */
> > + oldinti = signal(SIGINFO, psummary);
> > + while ((len = read(fd, buf, buflen)) > 0) {
> > + bytes += len;
> > + for (cp = buf; len > 0; len -= wlen, cp += wlen) {
> > + if ((wlen = write(out, cp, len)) == -1) {
> > + warn("Writing %s", savefile);
> > + signal(SIGINFO, oldinti);
> > + goto cleanup_copy;
> > + }
> > + }
> > + if (hash && !progress) {
> > + while (bytes >= hashbytes) {
> > + (void)putc('#', ttyout);
> > + hashbytes += mark;
> > + }
> > + (void)fflush(ttyout);
> > + }
> > + }
> > + save_errno = errno;
> > + signal(SIGINFO, oldinti);
> > + if (hash && !progress && bytes > 0) {
> > + if (bytes < mark)
> > + (void)putc('#', ttyout);
> > + (void)putc('\n', ttyout);
> > +
Re: ftp(1): separate file:/ URL handling
On Thu, Dec 19 2019, Jeremie Courreges-Anglas wrote:
> A bit late...
>
> Move file: URL handling into its own function. This simplifies
> url_get() and would have prevented problems with bogus redirections.
>
> file_get() unrolls the code previously used in url_get(), except the
> #ifndef SMALL bits were stripped out. file: support is mainly
> (only?) used in the installer which is built with SMALL defined.
> Resuming an incomplete file: transfer sounds nuts.
>
> I felt a bit guilty about copying dubious code, there are some changes
> that ought to be applied to url_get() too:
> - write(2) can't return 0, can it? (something old about non-blocking
> sockets maybe?). Anyway, no need to handle 0 explicitely.
> - allocate buf before setjmp instead of marking it volatile
> - save_errno/warnc dance if read(2) fails
>
> This survived make release on amd64 and a bsd.rd upgrade with sets
> on 'disk'. The resulting ftp(1) binary size decreases.
>
> Comments/ok?
Still looking for feedback, else I'll resort to crowdtesting. ;)
>
> Index: fetch.c
> ===
> --- fetch.c.orig
> +++ fetch.c
> @@ -68,6 +68,7 @@ struct tls;
> #include "ftp_var.h"
> #include "cmds.h"
>
> +static int file_get(const char *, const char *);
> static int url_get(const char *, const char *, const char *, int);
> static int save_chunked(FILE *, struct tls *, int , char *, size_t);
> static void aborthttp(int);
> @@ -182,6 +183,125 @@ tooslow(int signo)
> }
>
> /*
> + * Copy a local file (used by the OpenBSD installer).
> + * Returns -1 on failure, 0 on success
> + */
> +static int
> +file_get(const char *path, const char *outfile)
> +{
> + struct stat st;
> + int fd, out, rval = -1, save_errno;
> + volatile sig_t oldintr, oldinti;
> + const char *savefile;
> + char*buf = NULL, *cp;
> + const size_t buflen = 128 * 1024;
> + off_thashbytes;
> + ssize_t len, wlen;
> +
> + direction = "received";
> +
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> + warn("Can't open file %s", path);
> + return -1;
> + }
> +
> + if (fstat(fd, &st) == -1)
> + filesize = -1;
> + else
> + filesize = st.st_size;
> +
> + if (outfile != NULL)
> + savefile = outfile;
> + else {
> + if (path[strlen(path) - 1] == '/') /* Consider no file */
> + savefile = NULL;/* after dir invalid. */
> + else
> + savefile = basename(path);
> + }
> +
> + if (EMPTYSTRING(savefile)) {
> + warnx("No filename after directory (use -o): %s", path);
> + goto cleanup_copy;
> + }
> +
> + /* Open the output file. */
> + if (!pipeout) {
> + out = open(savefile, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> + if (out == -1) {
> + warn("Can't open %s", savefile);
> + goto cleanup_copy;
> + }
> + } else
> + out = fileno(stdout);
> +
> + if ((buf = malloc(buflen)) == NULL)
> + errx(1, "Can't allocate memory for transfer buffer");
> +
> + /* Trap signals */
> + oldintr = NULL;
> + oldinti = NULL;
> + if (setjmp(httpabort)) {
> + if (oldintr)
> + (void)signal(SIGINT, oldintr);
> + if (oldinti)
> + (void)signal(SIGINFO, oldinti);
> + goto cleanup_copy;
> + }
> + oldintr = signal(SIGINT, abortfile);
> +
> + bytes = 0;
> + hashbytes = mark;
> + progressmeter(-1, path);
> +
> + /* Finally, suck down the file. */
> + oldinti = signal(SIGINFO, psummary);
> + while ((len = read(fd, buf, buflen)) > 0) {
> + bytes += len;
> + for (cp = buf; len > 0; len -= wlen, cp += wlen) {
> + if ((wlen = write(out, cp, len)) == -1) {
> + warn("Writing %s", savefile);
> + signal(SIGINFO, oldinti);
> + goto cleanup_copy;
> + }
> + }
> + if (hash && !progress) {
> + while (bytes >= hashbytes) {
> + (void)putc('#', ttyout);
> + hashbytes += mark;
> + }
> + (void)fflush(ttyout);
> + }
> + }
> + save_errno = errno;
> + signal(SIGINFO, oldinti);
> + if (hash && !progress && bytes > 0) {
> + if (bytes < mark)
> + (void)putc('#', ttyout);
> + (void)putc('\n', ttyout);
> + (void)fflush(ttyout);
> + }
> + if (len == -1) {
> + warnc(save_errno, "Reading from file");
> + goto cleanup_copy;
> + }
> + progressmeter(1, NULL
ftp(1): separate file:/ URL handling
A bit late...
Move file: URL handling into its own function. This simplifies
url_get() and would have prevented problems with bogus redirections.
file_get() unrolls the code previously used in url_get(), except the
#ifndef SMALL bits were stripped out. file: support is mainly
(only?) used in the installer which is built with SMALL defined.
Resuming an incomplete file: transfer sounds nuts.
I felt a bit guilty about copying dubious code, there are some changes
that ought to be applied to url_get() too:
- write(2) can't return 0, can it? (something old about non-blocking
sockets maybe?). Anyway, no need to handle 0 explicitely.
- allocate buf before setjmp instead of marking it volatile
- save_errno/warnc dance if read(2) fails
This survived make release on amd64 and a bsd.rd upgrade with sets
on 'disk'. The resulting ftp(1) binary size decreases.
Comments/ok?
Index: fetch.c
===
--- fetch.c.orig
+++ fetch.c
@@ -68,6 +68,7 @@ struct tls;
#include "ftp_var.h"
#include "cmds.h"
+static int file_get(const char *, const char *);
static int url_get(const char *, const char *, const char *, int);
static int save_chunked(FILE *, struct tls *, int , char *, size_t);
static voidaborthttp(int);
@@ -182,6 +183,125 @@ tooslow(int signo)
}
/*
+ * Copy a local file (used by the OpenBSD installer).
+ * Returns -1 on failure, 0 on success
+ */
+static int
+file_get(const char *path, const char *outfile)
+{
+ struct stat st;
+ int fd, out, rval = -1, save_errno;
+ volatile sig_t oldintr, oldinti;
+ const char *savefile;
+ char*buf = NULL, *cp;
+ const size_t buflen = 128 * 1024;
+ off_thashbytes;
+ ssize_t len, wlen;
+
+ direction = "received";
+
+ fd = open(path, O_RDONLY);
+ if (fd == -1) {
+ warn("Can't open file %s", path);
+ return -1;
+ }
+
+ if (fstat(fd, &st) == -1)
+ filesize = -1;
+ else
+ filesize = st.st_size;
+
+ if (outfile != NULL)
+ savefile = outfile;
+ else {
+ if (path[strlen(path) - 1] == '/') /* Consider no file */
+ savefile = NULL;/* after dir invalid. */
+ else
+ savefile = basename(path);
+ }
+
+ if (EMPTYSTRING(savefile)) {
+ warnx("No filename after directory (use -o): %s", path);
+ goto cleanup_copy;
+ }
+
+ /* Open the output file. */
+ if (!pipeout) {
+ out = open(savefile, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+ if (out == -1) {
+ warn("Can't open %s", savefile);
+ goto cleanup_copy;
+ }
+ } else
+ out = fileno(stdout);
+
+ if ((buf = malloc(buflen)) == NULL)
+ errx(1, "Can't allocate memory for transfer buffer");
+
+ /* Trap signals */
+ oldintr = NULL;
+ oldinti = NULL;
+ if (setjmp(httpabort)) {
+ if (oldintr)
+ (void)signal(SIGINT, oldintr);
+ if (oldinti)
+ (void)signal(SIGINFO, oldinti);
+ goto cleanup_copy;
+ }
+ oldintr = signal(SIGINT, abortfile);
+
+ bytes = 0;
+ hashbytes = mark;
+ progressmeter(-1, path);
+
+ /* Finally, suck down the file. */
+ oldinti = signal(SIGINFO, psummary);
+ while ((len = read(fd, buf, buflen)) > 0) {
+ bytes += len;
+ for (cp = buf; len > 0; len -= wlen, cp += wlen) {
+ if ((wlen = write(out, cp, len)) == -1) {
+ warn("Writing %s", savefile);
+ signal(SIGINFO, oldinti);
+ goto cleanup_copy;
+ }
+ }
+ if (hash && !progress) {
+ while (bytes >= hashbytes) {
+ (void)putc('#', ttyout);
+ hashbytes += mark;
+ }
+ (void)fflush(ttyout);
+ }
+ }
+ save_errno = errno;
+ signal(SIGINFO, oldinti);
+ if (hash && !progress && bytes > 0) {
+ if (bytes < mark)
+ (void)putc('#', ttyout);
+ (void)putc('\n', ttyout);
+ (void)fflush(ttyout);
+ }
+ if (len == -1) {
+ warnc(save_errno, "Reading from file");
+ goto cleanup_copy;
+ }
+ progressmeter(1, NULL);
+ if (verbose)
+ ptransfer(0);
+ (void)signal(SIGINT, oldintr);
+
+ rval = 0;
+
+cleanup_copy:
+ free(buf);
+ if (out >= 0 && out != fileno(stdout))
+
