Commit bf6a341b4cf51c8eef9f23765a6f89a380d1d999
1 parent
3d0057d4
changed header hashing to use btree (GNU only). @TODO: make this conditional for…
… other systems. Removed the qsort calls on server->fds making O(2nlogn) to O(n)
Showing
16 changed files
with
157 additions
and
71 deletions
| 1 | +2012-02-12 00:05:13 +0100 Georg Hopp | ||
| 2 | + | ||
| 3 | + * changed header hashing to use btree (GNU only). @TODO: make this conditional for other systems. Removed the qsort calls on server->fds making O(2nlogn) to O(n) (HEAD, master) | ||
| 4 | + | ||
| 5 | +2012-02-11 13:52:32 +0100 Georg Hopp | ||
| 6 | + | ||
| 7 | + * daemonize testserver now (origin/master, origin/HEAD) | ||
| 8 | + | ||
| 1 | 2012-02-11 12:47:01 +0100 Georg Hopp | 9 | 2012-02-11 12:47:01 +0100 Georg Hopp |
| 2 | 10 | ||
| 3 | - * fix seaks and hangs after adding response object (mostly not related with the response object but how i integated it into serverRun (HEAD, master) | 11 | + * fix seaks and hangs after adding response object (mostly not related with the response object but how i integated it into serverRun |
| 4 | 12 | ||
| 5 | 2012-02-10 19:57:57 +0100 Georg Hopp | 13 | 2012-02-10 19:57:57 +0100 Georg Hopp |
| 6 | 14 | ||
| 7 | - * started a response handler and changed serverRun to use it for its response (origin/master, origin/HEAD) | 15 | + * started a response handler and changed serverRun to use it for its response |
| 8 | 16 | ||
| 9 | 2012-02-10 12:42:04 +0100 Georg Hopp | 17 | 2012-02-10 12:42:04 +0100 Georg Hopp |
| 10 | 18 |
| @@ -9,13 +9,14 @@ CLASS(HttpRequest) { | @@ -9,13 +9,14 @@ CLASS(HttpRequest) { | ||
| 9 | char * uri; | 9 | char * uri; |
| 10 | char * version; | 10 | char * version; |
| 11 | 11 | ||
| 12 | - HttpHeader header[128]; | ||
| 13 | - int nheader; | 12 | + HttpHeader header; |
| 14 | 13 | ||
| 15 | char * body; | 14 | char * body; |
| 16 | int nbody; | 15 | int nbody; |
| 17 | }; | 16 | }; |
| 18 | 17 | ||
| 18 | +void httpHeaderAdd(HttpHeader *, HttpHeader); | ||
| 19 | +char * XhttpHeaderGet(HttpHeader *, const char *); | ||
| 19 | char * httpRequestHeaderGet(HttpRequest, const char *); | 20 | char * httpRequestHeaderGet(HttpRequest, const char *); |
| 20 | char httpRequestHasKeepAlive(HttpRequest); | 21 | char httpRequestHasKeepAlive(HttpRequest); |
| 21 | 22 |
| 1 | +#ifndef _GNU_SOURCE | ||
| 2 | +#define _GNU_SOURCE | ||
| 3 | +#endif | ||
| 4 | + | ||
| 1 | #include <stdlib.h> | 5 | #include <stdlib.h> |
| 2 | #include <stdarg.h> | 6 | #include <stdarg.h> |
| 7 | +#include <search.h> | ||
| 3 | 8 | ||
| 4 | #include "class.h" | 9 | #include "class.h" |
| 5 | #include "interface/class.h" | 10 | #include "interface/class.h" |
| 6 | 11 | ||
| 7 | #include "http/request.h" | 12 | #include "http/request.h" |
| 8 | 13 | ||
| 14 | + | ||
| 9 | static | 15 | static |
| 16 | +inline | ||
| 10 | void | 17 | void |
| 11 | _free(void ** data) | 18 | _free(void ** data) |
| 12 | { | 19 | { |
| @@ -16,6 +23,14 @@ _free(void ** data) | @@ -16,6 +23,14 @@ _free(void ** data) | ||
| 16 | } | 23 | } |
| 17 | 24 | ||
| 18 | static | 25 | static |
| 26 | +inline | ||
| 27 | +void | ||
| 28 | +tDelete(void * node) | ||
| 29 | +{ | ||
| 30 | + delete(&node); | ||
| 31 | +} | ||
| 32 | + | ||
| 33 | +static | ||
| 19 | void | 34 | void |
| 20 | ctor(void * _this, va_list * params) {} | 35 | ctor(void * _this, va_list * params) {} |
| 21 | 36 | ||
| @@ -24,16 +39,17 @@ void | @@ -24,16 +39,17 @@ void | ||
| 24 | dtor(void * _this) | 39 | dtor(void * _this) |
| 25 | { | 40 | { |
| 26 | HttpRequest this = _this; | 41 | HttpRequest this = _this; |
| 27 | - int i; | ||
| 28 | 42 | ||
| 29 | _free((void **)&(this->version)); | 43 | _free((void **)&(this->version)); |
| 30 | _free((void **)&(this->uri)); | 44 | _free((void **)&(this->uri)); |
| 31 | _free((void **)&(this->method)); | 45 | _free((void **)&(this->method)); |
| 32 | 46 | ||
| 33 | - for (i=0; i<128; i++) { | ||
| 34 | - if (NULL == (this->header)[i]) break; | ||
| 35 | - delete(&(this->header)[i]); | ||
| 36 | - } | 47 | + /** |
| 48 | + * this is a GNU extension...anyway on most non | ||
| 49 | + * GNUish systems i would not use tsearch anyway | ||
| 50 | + * as the trees will be unbalanced. | ||
| 51 | + */ | ||
| 52 | + tdestroy(this->header, tDelete); | ||
| 37 | 53 | ||
| 38 | _free((void **)&(this->body)); | 54 | _free((void **)&(this->body)); |
| 39 | } | 55 | } |
| @@ -11,7 +11,7 @@ httpRequestHasKeepAlive(HttpRequest request) | @@ -11,7 +11,7 @@ httpRequestHasKeepAlive(HttpRequest request) | ||
| 11 | char * header; | 11 | char * header; |
| 12 | char * header_ptr; | 12 | char * header_ptr; |
| 13 | 13 | ||
| 14 | - header = httpHeaderGet(request->header, request->nheader, "connection"); | 14 | + header = XhttpHeaderGet(&(request->header), "connection"); |
| 15 | 15 | ||
| 16 | if (NULL == header) { | 16 | if (NULL == header) { |
| 17 | return 0; | 17 | return 0; |
| @@ -3,44 +3,10 @@ | @@ -3,44 +3,10 @@ | ||
| 3 | 3 | ||
| 4 | #include "http/request.h" | 4 | #include "http/request.h" |
| 5 | 5 | ||
| 6 | -static | ||
| 7 | -inline | ||
| 8 | -unsigned long | ||
| 9 | -sdbm(const unsigned char * str) | ||
| 10 | -{ | ||
| 11 | - unsigned long hash = 0; | ||
| 12 | - int c; | ||
| 13 | - | ||
| 14 | - while ((c = tolower(*str++))) | ||
| 15 | - hash = c + (hash << 6) + (hash << 16) - hash; | ||
| 16 | - | ||
| 17 | - return hash; | ||
| 18 | -} | ||
| 19 | - | ||
| 20 | -static | ||
| 21 | -inline | ||
| 22 | -int | ||
| 23 | -comp (const void * _a, const void * _b) | ||
| 24 | -{ | ||
| 25 | - unsigned long a = *(unsigned long *)_a; | ||
| 26 | - const struct HttpRequestHeader * b = _b; | ||
| 27 | - return (a < b->hash)? -1 : (a > b->hash)? 1 : 0; | ||
| 28 | -} | ||
| 29 | - | ||
| 30 | char * | 6 | char * |
| 31 | httpRequestHeaderGet(HttpRequest this, const char * name) | 7 | httpRequestHeaderGet(HttpRequest this, const char * name) |
| 32 | { | 8 | { |
| 33 | - unsigned long hash = sdbm((unsigned char *)name); | ||
| 34 | - struct HttpRequestHeader * header; | ||
| 35 | - | ||
| 36 | - header = bsearch( | ||
| 37 | - &hash, | ||
| 38 | - this->header, | ||
| 39 | - this->nheader, | ||
| 40 | - sizeof(struct HttpRequestHeader), | ||
| 41 | - comp); | ||
| 42 | - | ||
| 43 | - return (NULL != header)? header->value : NULL; | 9 | + return XhttpHeaderGet(&(this->header), name); |
| 44 | } | 10 | } |
| 45 | 11 | ||
| 46 | // vim: set ts=4 sw=4: | 12 | // vim: set ts=4 sw=4: |
| 1 | +#include <search.h> | ||
| 2 | +#include <ctype.h> | ||
| 3 | +#include <stdio.h> | ||
| 4 | + | ||
| 1 | #include "class.h" | 5 | #include "class.h" |
| 2 | #include "interface/class.h" | 6 | #include "interface/class.h" |
| 3 | #include "http/header.h" | 7 | #include "http/header.h" |
| 4 | #include "http/request.h" | 8 | #include "http/request.h" |
| 5 | 9 | ||
| 10 | +static | ||
| 11 | +inline | ||
| 12 | +unsigned long | ||
| 13 | +sdbm(const unsigned char * str) | ||
| 14 | +{ | ||
| 15 | + unsigned long hash = 0; | ||
| 16 | + int c; | ||
| 17 | + | ||
| 18 | + while ((c = tolower(*str++))) | ||
| 19 | + hash = c + (hash << 6) + (hash << 16) - hash; | ||
| 20 | + | ||
| 21 | + return hash; | ||
| 22 | +} | ||
| 23 | + | ||
| 24 | +static | ||
| 25 | +inline | ||
| 26 | +int | ||
| 27 | +comp(const void * _a, const void * _b) | ||
| 28 | +{ | ||
| 29 | + HttpHeader a = (HttpHeader)_a; | ||
| 30 | + HttpHeader b = (HttpHeader)_b; | ||
| 31 | + return (a->hash < b->hash)? -1 : (a->hash > b->hash)? 1 : 0; | ||
| 32 | +} | ||
| 33 | + | ||
| 6 | void | 34 | void |
| 7 | httpRequestParserGetHeader(HttpRequest request, char * line) | 35 | httpRequestParserGetHeader(HttpRequest request, char * line) |
| 8 | { | 36 | { |
| @@ -12,7 +40,29 @@ httpRequestParserGetHeader(HttpRequest request, char * line) | @@ -12,7 +40,29 @@ httpRequestParserGetHeader(HttpRequest request, char * line) | ||
| 12 | *(value++) = 0; | 40 | *(value++) = 0; |
| 13 | for (; *value == ' ' && *value != 0; value++); | 41 | for (; *value == ' ' && *value != 0; value++); |
| 14 | 42 | ||
| 15 | - (request->header)[request->nheader++] = new(HttpHeader, name, value); | 43 | + httpHeaderAdd(&(request->header), new(HttpHeader, name, value)); |
| 44 | +} | ||
| 45 | + | ||
| 46 | +void | ||
| 47 | +httpHeaderAdd(HttpHeader * root, HttpHeader header) | ||
| 48 | +{ | ||
| 49 | + HttpHeader * found = tsearch(header, (void **)root, comp); | ||
| 50 | + | ||
| 51 | + if (*found != header) { | ||
| 52 | + puts("uhh, duplicate header set. " | ||
| 53 | + "This is not implemented right now. " | ||
| 54 | + "Keep the first one found."); | ||
| 55 | + delete(&header); | ||
| 56 | + } | ||
| 57 | +} | ||
| 58 | + | ||
| 59 | +char * | ||
| 60 | +XhttpHeaderGet(HttpHeader * root, const char * name) | ||
| 61 | +{ | ||
| 62 | + struct c_HttpHeader search = {sdbm((const unsigned char*)name), NULL, NULL}; | ||
| 63 | + HttpHeader * found = tfind(&search, (void **)root, comp); | ||
| 64 | + | ||
| 65 | + return (NULL != found)? (*found)->value : NULL; | ||
| 16 | } | 66 | } |
| 17 | 67 | ||
| 18 | // vim: set ts=4 sw=4: | 68 | // vim: set ts=4 sw=4: |
| @@ -82,15 +82,12 @@ httpRequestParserParse(HttpRequestParser this) | @@ -82,15 +82,12 @@ httpRequestParserParse(HttpRequestParser this) | ||
| 82 | break; | 82 | break; |
| 83 | 83 | ||
| 84 | case HTTP_REQUEST_HEADERS_DONE: | 84 | case HTTP_REQUEST_HEADERS_DONE: |
| 85 | - httpHeaderSort(this->cur_request->header, this->cur_request->nheader); | ||
| 86 | - | ||
| 87 | { | 85 | { |
| 88 | char * nbody; | 86 | char * nbody; |
| 89 | 87 | ||
| 90 | if (0 == this->cur_request->nbody) { | 88 | if (0 == this->cur_request->nbody) { |
| 91 | - nbody = httpHeaderGet( | ||
| 92 | - this->cur_request->header, | ||
| 93 | - this->cur_request->nheader, | 89 | + nbody = XhttpHeaderGet( |
| 90 | + &(this->cur_request->header), | ||
| 94 | "Content-Length"); | 91 | "Content-Length"); |
| 95 | 92 | ||
| 96 | if (NULL == nbody) { | 93 | if (NULL == nbody) { |
| 1 | #include <poll.h> /* for select system call and related */ | 1 | #include <poll.h> /* for select system call and related */ |
| 2 | #include <string.h> /* for memset and stuff */ | 2 | #include <string.h> /* for memset and stuff */ |
| 3 | #include <stdlib.h> /* for getopt */ | 3 | #include <stdlib.h> /* for getopt */ |
| 4 | +#include <unistd.h> | ||
| 5 | +#include <fcntl.h> | ||
| 4 | 6 | ||
| 5 | #include "class.h" | 7 | #include "class.h" |
| 6 | #include "server.h" | 8 | #include "server.h" |
| @@ -15,6 +17,7 @@ ctor(void * _this, va_list * params) | @@ -15,6 +17,7 @@ ctor(void * _this, va_list * params) | ||
| 15 | Server this = _this; | 17 | Server this = _this; |
| 16 | in_port_t port; | 18 | in_port_t port; |
| 17 | unsigned int backlog; | 19 | unsigned int backlog; |
| 20 | + int flags; | ||
| 18 | 21 | ||
| 19 | this->logger = va_arg(* params, Logger); | 22 | this->logger = va_arg(* params, Logger); |
| 20 | this->reader = va_arg(* params, void*); | 23 | this->reader = va_arg(* params, void*); |
| @@ -22,6 +25,10 @@ ctor(void * _this, va_list * params) | @@ -22,6 +25,10 @@ ctor(void * _this, va_list * params) | ||
| 22 | backlog = va_arg(* params, unsigned int); | 25 | backlog = va_arg(* params, unsigned int); |
| 23 | 26 | ||
| 24 | this->sock = new(Sock, this->logger, port); | 27 | this->sock = new(Sock, this->logger, port); |
| 28 | + | ||
| 29 | + flags = fcntl(this->sock->handle, F_GETFL, 0); | ||
| 30 | + fcntl(this->sock->handle, F_SETFL, flags | O_NONBLOCK); | ||
| 31 | + | ||
| 25 | socketListen(this->sock, backlog); | 32 | socketListen(this->sock, backlog); |
| 26 | 33 | ||
| 27 | (this->fds)[0].fd = this->sock->handle; | 34 | (this->fds)[0].fd = this->sock->handle; |
| 1 | +#include <stdlib.h> | ||
| 1 | #include <string.h> | 2 | #include <string.h> |
| 2 | 3 | ||
| 3 | #include "server.h" | 4 | #include "server.h" |
| @@ -17,9 +18,7 @@ serverCloseConn(Server this, unsigned int i) | @@ -17,9 +18,7 @@ serverCloseConn(Server this, unsigned int i) | ||
| 17 | } | 18 | } |
| 18 | (this->conns)[fd].keep_alive = 0; | 19 | (this->conns)[fd].keep_alive = 0; |
| 19 | 20 | ||
| 20 | - (this->fds)[i].events = 0; | ||
| 21 | - (this->fds)[i].revents = 0; | ||
| 22 | - (this->fds)[i].fd = 0; | 21 | + memset(&(this->fds[i]), 0, sizeof(struct pollfd)); |
| 23 | } | 22 | } |
| 24 | 23 | ||
| 25 | // vim: set ts=4 sw=4: | 24 | // vim: set ts=4 sw=4: |
| 1 | static | 1 | static |
| 2 | -void | 2 | +int |
| 3 | serverHandleAccept(Server this) | 3 | serverHandleAccept(Server this) |
| 4 | { | 4 | { |
| 5 | char remoteAddr[16] = ""; | 5 | char remoteAddr[16] = ""; |
| @@ -21,7 +21,7 @@ serverHandleAccept(Server this) | @@ -21,7 +21,7 @@ serverHandleAccept(Server this) | ||
| 21 | delete(&acc); | 21 | delete(&acc); |
| 22 | } | 22 | } |
| 23 | 23 | ||
| 24 | -// (this->fds)[0].revents |= POLLIN; | 24 | + return (acc)? acc->handle : -1; |
| 25 | } | 25 | } |
| 26 | 26 | ||
| 27 | // vim: set ts=4 sw=4: | 27 | // vim: set ts=4 sw=4: |
| 1 | #define POLLFD(ptr) ((struct pollfd *)(ptr)) | 1 | #define POLLFD(ptr) ((struct pollfd *)(ptr)) |
| 2 | +#define SWAP(a, b) ((a)^=(b),(b)^=(a),(a)^=(b)) | ||
| 2 | 3 | ||
| 3 | static | 4 | static |
| 4 | inline | 5 | inline |
| @@ -25,9 +26,27 @@ int | @@ -25,9 +26,27 @@ int | ||
| 25 | serverPoll(Server this) { | 26 | serverPoll(Server this) { |
| 26 | int events; | 27 | int events; |
| 27 | 28 | ||
| 28 | - qsort(this->fds, this->nfds, sizeof(struct pollfd), sortEvents); | ||
| 29 | - while((this->fds)[this->nfds].fd == 0 && this->nfds > 0) this->nfds--; | ||
| 30 | - this->nfds++; | 29 | + /** |
| 30 | + * put all closed fds to end of array in O(this->nfds) | ||
| 31 | + */ | ||
| 32 | + struct pollfd * fda = &(this->fds[1]); | ||
| 33 | + struct pollfd * fdb = &(this->fds[this->nfds-1]); | ||
| 34 | + | ||
| 35 | + while (fda <= fdb) { | ||
| 36 | + while (0 == fdb->fd && fda <= fdb) { | ||
| 37 | + fdb--; | ||
| 38 | + this->nfds--; | ||
| 39 | + } | ||
| 40 | + | ||
| 41 | + while (0 != fda->fd && fda <= fdb) fda++; | ||
| 42 | + | ||
| 43 | + if (fda < fdb) { | ||
| 44 | + memcpy(fda, fdb, sizeof(struct pollfd)); | ||
| 45 | + memset(fdb, 0, sizeof(struct pollfd)); | ||
| 46 | + fdb--; | ||
| 47 | + this->nfds--; | ||
| 48 | + } | ||
| 49 | + } | ||
| 31 | 50 | ||
| 32 | /* | 51 | /* |
| 33 | * wait for handles to become ready | 52 | * wait for handles to become ready |
| @@ -45,14 +64,9 @@ serverPoll(Server this) { | @@ -45,14 +64,9 @@ serverPoll(Server this) { | ||
| 45 | loggerLog(this->logger, LOGGER_CRIT, | 64 | loggerLog(this->logger, LOGGER_CRIT, |
| 46 | "poll systemcall failed: [%s] - service terminated", | 65 | "poll systemcall failed: [%s] - service terminated", |
| 47 | strerror(errno)); | 66 | strerror(errno)); |
| 48 | - //exit(EXIT_FAILURE); /* @TODO do real shutdown here */ | ||
| 49 | } | 67 | } |
| 50 | } | 68 | } |
| 51 | 69 | ||
| 52 | - if (-1 != events) { | ||
| 53 | - qsort(this->fds, this->nfds, sizeof(struct pollfd), sortRevents); | ||
| 54 | - } | ||
| 55 | - | ||
| 56 | return events; | 70 | return events; |
| 57 | } | 71 | } |
| 58 | 72 |
| @@ -5,6 +5,7 @@ | @@ -5,6 +5,7 @@ | ||
| 5 | #include <unistd.h> | 5 | #include <unistd.h> |
| 6 | #include <ctype.h> | 6 | #include <ctype.h> |
| 7 | #include <time.h> | 7 | #include <time.h> |
| 8 | +#include <errno.h> | ||
| 8 | 9 | ||
| 9 | #include "server.h" | 10 | #include "server.h" |
| 10 | #include "socket.h" | 11 | #include "socket.h" |
| @@ -51,22 +52,43 @@ serverRun(Server this) | @@ -51,22 +52,43 @@ serverRun(Server this) | ||
| 51 | events = serverPoll(this); | 52 | events = serverPoll(this); |
| 52 | if (doShutdown) break; | 53 | if (doShutdown) break; |
| 53 | 54 | ||
| 54 | - for (i=0; i < events; i++) { | 55 | + for (i=0; i < this->nfds; i++) { |
| 55 | int fd = (this->fds)[i].fd; | 56 | int fd = (this->fds)[i].fd; |
| 56 | - //int nreads = 0, nwrites = 0; | 57 | + int naccs = 10, nreads = 10, nwrites = 10; |
| 58 | + | ||
| 59 | + if (0 >= events) break; | ||
| 60 | + | ||
| 61 | + if (0 != ((this->fds)[i].revents & POLLIN) && 0 < nreads) { | ||
| 62 | + events--; | ||
| 57 | 63 | ||
| 58 | - if (0 != ((this->fds)[i].revents & POLLIN)) { | ||
| 59 | /** | 64 | /** |
| 60 | * handle accept | 65 | * handle accept |
| 61 | */ | 66 | */ |
| 62 | if (this->sock->handle == (this->fds)[i].fd) { | 67 | if (this->sock->handle == (this->fds)[i].fd) { |
| 63 | - serverHandleAccept(this); | 68 | + while(-1 != serverHandleAccept(this) && 0 < naccs) { |
| 69 | + naccs--; | ||
| 70 | + | ||
| 71 | + switch(errno) { | ||
| 72 | + case EAGAIN: | ||
| 73 | + loggerLog(this->logger, | ||
| 74 | + LOGGER_DEBUG, | ||
| 75 | + "server accept blocks"); | ||
| 76 | + break; | ||
| 77 | + | ||
| 78 | + default: | ||
| 79 | + loggerLog(this->logger, | ||
| 80 | + LOGGER_DEBUG, | ||
| 81 | + "server accept error"); | ||
| 82 | + break; | ||
| 83 | + } | ||
| 84 | + } | ||
| 64 | } | 85 | } |
| 65 | 86 | ||
| 66 | /** | 87 | /** |
| 67 | * handle reads | 88 | * handle reads |
| 68 | */ | 89 | */ |
| 69 | else { | 90 | else { |
| 91 | + nreads--; | ||
| 70 | /** | 92 | /** |
| 71 | * do some other processing | 93 | * do some other processing |
| 72 | * @TODO: actually this will hard assume that our stream reader | 94 | * @TODO: actually this will hard assume that our stream reader |
| @@ -126,9 +148,12 @@ serverRun(Server this) | @@ -126,9 +148,12 @@ serverRun(Server this) | ||
| 126 | /** | 148 | /** |
| 127 | * handle writes | 149 | * handle writes |
| 128 | */ | 150 | */ |
| 129 | - if (0 != ((this->fds)[i].revents & POLLOUT)) { | 151 | + if (0 != ((this->fds)[i].revents & POLLOUT) && 0 < nwrites) { |
| 130 | int size; | 152 | int size; |
| 131 | 153 | ||
| 154 | + events--; | ||
| 155 | + nwrites--; | ||
| 156 | + | ||
| 132 | size = write( | 157 | size = write( |
| 133 | (this->fds)[i].fd, | 158 | (this->fds)[i].fd, |
| 134 | (this->conns)[fd].wbuf, | 159 | (this->conns)[fd].wbuf, |
| @@ -42,7 +42,7 @@ dtor(void * _this) | @@ -42,7 +42,7 @@ dtor(void * _this) | ||
| 42 | { | 42 | { |
| 43 | Sock this = _this; | 43 | Sock this = _this; |
| 44 | 44 | ||
| 45 | - if (0 != this->handle) { | 45 | + if (STDERR_FILENO < this->handle) { |
| 46 | shutdown(this->handle, SHUT_RDWR); | 46 | shutdown(this->handle, SHUT_RDWR); |
| 47 | close(this->handle); | 47 | close(this->handle); |
| 48 | } | 48 | } |
Please
register
or
login
to post a comment