On Mon, Aug 17, 2020 at 03:47:27AM -0700, Jeremy wrote:
> Then should the functions which use the global variable, "term", in st.c
> accept a Term pointer instead?
>
> In the case of st, I believe this may affect readability slightly,
> however, it would allow for some extensibility(if dvtm, for example,
> wanted to rewrite its terminal implementation).
>
> I'm not concerned about race conditions because the global "server"
> is copied via fork before any request-handling code has the chance to
> modify it.
>
> I agree with the changes, just not the reasoning - though I'd like to
> know what you think
>
This is off-topic to quark, but:
In theory for st it might also be useful for example to write automated tests
for terminal emulation and testing its state.
When the X split happened in st there were promises by people to have more
back-ends also and possibly have tests, but this didn't happen.
It is a common behaviour in programming to try to "make things generic", but
then only add more abstractions and then not taking advantage of these
abstractions, making it harder to understand the code. It is a balance.
> On 08/17/20 11:40AM, git_AT_suckless.org wrote:
> > commit 65600ffe7a2868e95cf172550c85aa074e209e0d
> > Author: Laslo Hunhold <dev_AT_frign.de>
> > AuthorDate: Mon Aug 17 11:37:25 2020 +0200
> > Commit: Laslo Hunhold <dev_AT_frign.de>
> > CommitDate: Mon Aug 17 11:37:25 2020 +0200
> >
> > Reduce global state by localizing the server-struct
> >
> > The server-struct variable s was global, which made it readable and
> > modifiable from any point in the code. Making it a local variable in
> > main() instead and passing it as a pointer to constant memory to each
> > function needing it makes much more sense and allows the compiler to
> > warn us if we do try to modify it, which it wouldn't have before.
> >
> > Signed-off-by: Laslo Hunhold <dev_AT_frign.de>
> >
> > diff --git a/http.c b/http.c
> > index 6bf4264..c9cf4dc 100644
> > --- a/http.c
> > +++ b/http.c
> > _AT_@ -528,7 +528,7 @@ parse_range(const char *str, size_t size, size_t *lower, size_t *upper)
> > #define RELPATH(x) ((!*(x) || !strcmp(x, "/")) ? "." : ((x) + 1))
> >
> > enum status
> > -http_send_response(int fd, const struct request *req)
> > +http_send_response(int fd, const struct request *req, const struct server *s)
> > {
> > enum status returnstatus;
> > struct in6_addr addr;
> > _AT_@ -547,27 +547,27 @@ http_send_response(int fd, const struct request *req)
> >
> > /* match vhost */
> > vhostmatch = NULL;
> > - if (s.vhost) {
> > - for (i = 0; i < s.vhost_len; i++) {
> > + if (s->vhost) {
> > + for (i = 0; i < s->vhost_len; i++) {
> > /* switch to vhost directory if there is a match */
> > - if (!regexec(&s.vhost[i].re, req->field[REQ_HOST], 0,
> > + if (!regexec(&(s->vhost[i].re), req->field[REQ_HOST], 0,
> > NULL, 0)) {
> > - if (chdir(s.vhost[i].dir) < 0) {
> > + if (chdir(s->vhost[i].dir) < 0) {
> > return http_send_status(fd, (errno == EACCES) ?
> > S_FORBIDDEN : S_NOT_FOUND);
> > }
> > - vhostmatch = s.vhost[i].chost;
> > + vhostmatch = s->vhost[i].chost;
> > break;
> > }
> > }
> > - if (i == s.vhost_len) {
> > + if (i == s->vhost_len) {
> > return http_send_status(fd, S_NOT_FOUND);
> > }
> >
> > /* if we have a vhost prefix, prepend it to the target */
> > - if (s.vhost[i].prefix) {
> > + if (s->vhost[i].prefix) {
> > if (esnprintf(tmptarget, sizeof(tmptarget), "%s%s",
> > - s.vhost[i].prefix, realtarget)) {
> > + s->vhost[i].prefix, realtarget)) {
> > return http_send_status(fd, S_REQUEST_TOO_LARGE);
> > }
> > memcpy(realtarget, tmptarget, sizeof(realtarget));
> > _AT_@ -575,19 +575,19 @@ http_send_response(int fd, const struct request *req)
> > }
> >
> > /* apply target prefix mapping */
> > - for (i = 0; i < s.map_len; i++) {
> > - len = strlen(s.map[i].from);
> > - if (!strncmp(realtarget, s.map[i].from, len)) {
> > + for (i = 0; i < s->map_len; i++) {
> > + len = strlen(s->map[i].from);
> > + if (!strncmp(realtarget, s->map[i].from, len)) {
> > /* match canonical host if vhosts are enabled and
> > * the mapping specifies a canonical host */
> > - if (s.vhost && s.map[i].chost &&
> > - strcmp(s.map[i].chost, vhostmatch)) {
> > + if (s->vhost && s->map[i].chost &&
> > + strcmp(s->map[i].chost, vhostmatch)) {
> > continue;
> > }
> >
> > /* swap out target prefix */
> > if (esnprintf(tmptarget, sizeof(tmptarget), "%s%s",
> > - s.map[i].to, realtarget + len)) {
> > + s->map[i].to, realtarget + len)) {
> > return http_send_status(fd, S_REQUEST_TOO_LARGE);
> > }
> > memcpy(realtarget, tmptarget, sizeof(realtarget));
> > _AT_@ -628,7 +628,7 @@ http_send_response(int fd, const struct request *req)
> > }
> >
> > /* redirect if targets differ, host is non-canonical or we prefixed */
> > - if (strcmp(req->target, realtarget) || (s.vhost && vhostmatch &&
> > + if (strcmp(req->target, realtarget) || (s->vhost && vhostmatch &&
> > strcmp(req->field[REQ_HOST], vhostmatch))) {
> > res.status = S_MOVED_PERMANENTLY;
> >
> > _AT_@ -636,14 +636,14 @@ http_send_response(int fd, const struct request *req)
> > encode(realtarget, tmptarget);
> >
> > /* determine target location */
> > - if (s.vhost) {
> > + if (s->vhost) {
> > /* absolute redirection URL */
> > targethost = req->field[REQ_HOST][0] ? vhostmatch ?
> > - vhostmatch : req->field[REQ_HOST] : s.host ?
> > - s.host : "localhost";
> > + vhostmatch : req->field[REQ_HOST] : s->host ?
> > + s->host : "localhost";
> >
> > /* do we need to add a port to the Location? */
> > - hasport = s.port && strcmp(s.port, "80");
> > + hasport = s->port && strcmp(s->port, "80");
> >
> > /* RFC 2732 specifies to use brackets for IPv6-addresses
> > * in URLs, so we need to check if our host is one and
> > _AT_@ -661,7 +661,7 @@ http_send_response(int fd, const struct request *req)
> > ipv6host ? "[" : "",
> > targethost,
> > ipv6host ? "]" : "", hasport ? ":" : "",
> > - hasport ? s.port : "", tmptarget)) {
> > + hasport ? s->port : "", tmptarget)) {
> > return http_send_status(fd, S_REQUEST_TOO_LARGE);
> > }
> > } else {
> > _AT_@ -679,16 +679,16 @@ http_send_response(int fd, const struct request *req)
> > if (S_ISDIR(st.st_mode)) {
> > /* append docindex to target */
> > if (esnprintf(realtarget, sizeof(realtarget), "%s%s",
> > - req->target, s.docindex)) {
> > + req->target, s->docindex)) {
> > return http_send_status(fd, S_REQUEST_TOO_LARGE);
> > }
> >
> > /* stat the docindex, which must be a regular file */
> > if (stat(RELPATH(realtarget), &st) < 0 || !S_ISREG(st.st_mode)) {
> > - if (s.listdirs) {
> > + if (s->listdirs) {
> > /* remove index suffix and serve dir */
> > realtarget[strlen(realtarget) -
> > - strlen(s.docindex)] = '\0';
> > + strlen(s->docindex)] = '\0';
> > return resp_dir(fd, RELPATH(realtarget), req);
> > } else {
> > /* reject */
> > diff --git a/http.h b/http.h
> > index b6ba073..563d9fd 100644
> > --- a/http.h
> > +++ b/http.h
> > _AT_@ -4,6 +4,8 @@
> >
> > #include <limits.h>
> >
> > +#include "util.h"
> > +
> > #define HEADER_MAX 4096
> > #define FIELD_MAX 200
> >
> > _AT_@ -69,6 +71,7 @@ struct response {
> > enum status http_send_header(int, const struct response *);
> > enum status http_send_status(int, enum status);
> > int http_get_request(int, struct request *);
> > -enum status http_send_response(int, const struct request *);
> > +enum status http_send_response(int, const struct request *,
> > + const struct server *);
> >
> > #endif /* HTTP_H */
> > diff --git a/main.c b/main.c
> > index 0542fab..ddd8468 100644
> > --- a/main.c
> > +++ b/main.c
> > _AT_@ -23,7 +23,7 @@
> > static char *udsname;
> >
> > static void
> > -serve(int infd, const struct sockaddr_storage *in_sa)
> > +serve(int infd, const struct sockaddr_storage *in_sa, const struct server *s)
> > {
> > struct request req;
> > time_t t;
> > _AT_@ -38,7 +38,7 @@ serve(int infd, const struct sockaddr_storage *in_sa)
> >
> > /* handle request */
> > if (!(status = http_get_request(infd, &req))) {
> > - status = http_send_response(infd, &req);
> > + status = http_send_response(infd, &req, s);
> > }
> >
> > /* write output to log */
> > _AT_@ -177,6 +177,9 @@ main(int argc, char *argv[])
> > struct group *grp = NULL;
> > struct passwd *pwd = NULL;
> > struct rlimit rlim;
> > + struct server s = {
> > + .docindex = "index.html",
> > + };
> > struct sockaddr_storage in_sa;
> > size_t i;
> > socklen_t in_sa_len;
> > _AT_@ -190,13 +193,6 @@ main(int argc, char *argv[])
> > char *user = "nobody";
> > char *group = "nogroup";
> >
> > - s.host = s.port = NULL;
> > - s.vhost = NULL;
> > - s.map = NULL;
> > - s.vhost_len = s.map_len = 0;
> > - s.docindex = "index.html";
> > - s.listdirs = 0;
> > -
> > ARGBEGIN {
> > case 'd':
> > servedir = EARGF(usage());
> > _AT_@ -372,7 +368,7 @@ main(int argc, char *argv[])
> > /* fork and handle */
> > switch (fork()) {
> > case 0:
> > - serve(infd, &in_sa);
> > + serve(infd, &in_sa, &s);
> > exit(0);
> > break;
> > case -1:
> > diff --git a/util.c b/util.c
> > index 7ad512f..518a3eb 100644
> > --- a/util.c
> > +++ b/util.c
> > _AT_@ -16,7 +16,6 @@
> > #include "util.h"
> >
> > char *argv0;
> > -struct server s;
> >
> > static void
> > verr(const char *fmt, va_list ap)
> > diff --git a/util.h b/util.h
> > index b23a192..21c18f8 100644
> > --- a/util.h
> > +++ b/util.h
> > _AT_@ -23,7 +23,7 @@ struct map {
> > char *to;
> > };
> >
> > -extern struct server {
> > +struct server {
> > char *host;
> > char *port;
> > char *docindex;
> > _AT_@ -32,7 +32,7 @@ extern struct server {
> > size_t vhost_len;
> > struct map *map;
> > size_t map_len;
> > -} s;
> > +};
> >
> > #undef MIN
> > #define MIN(x,y) ((x) < (y) ? (x) : (y))
> >
>
--
Kind regards,
Hiltjo
Received on Mon Aug 17 2020 - 15:22:53 CEST