Commit 56739c80 authored by Rusty Russell's avatar Rusty Russell

lguest: cleanup passing of /dev/lguest fd around example launcher.

We hand the /dev/lguest fd everywhere; it's far neater to just make it
a global (it already is, in fact, hidden in the waker_fds struct).
Signed-off-by: default avatarRusty Russell <rusty@rustcorp.com.au>
parent 713b15b3
...@@ -79,7 +79,6 @@ static bool verbose; ...@@ -79,7 +79,6 @@ static bool verbose;
/* File descriptors for the Waker. */ /* File descriptors for the Waker. */
struct { struct {
int pipe[2]; int pipe[2];
int lguest_fd;
} waker_fds; } waker_fds;
/* The pointer to the start of guest memory. */ /* The pointer to the start of guest memory. */
...@@ -89,6 +88,8 @@ static unsigned long guest_limit, guest_max; ...@@ -89,6 +88,8 @@ static unsigned long guest_limit, guest_max;
/* The pipe for signal hander to write to. */ /* The pipe for signal hander to write to. */
static int timeoutpipe[2]; static int timeoutpipe[2];
static unsigned int timeout_usec = 500; static unsigned int timeout_usec = 500;
/* The /dev/lguest file descriptor. */
static int lguest_fd;
/* a per-cpu variable indicating whose vcpu is currently running */ /* a per-cpu variable indicating whose vcpu is currently running */
static unsigned int __thread cpu_id; static unsigned int __thread cpu_id;
...@@ -139,7 +140,7 @@ struct device ...@@ -139,7 +140,7 @@ struct device
/* If handle_input is set, it wants to be called when this file /* If handle_input is set, it wants to be called when this file
* descriptor is ready. */ * descriptor is ready. */
int fd; int fd;
bool (*handle_input)(int fd, struct device *me); bool (*handle_input)(struct device *me);
/* Any queues attached to this device */ /* Any queues attached to this device */
struct virtqueue *vq; struct virtqueue *vq;
...@@ -169,7 +170,7 @@ struct virtqueue ...@@ -169,7 +170,7 @@ struct virtqueue
u16 last_avail_idx; u16 last_avail_idx;
/* The routine to call when the Guest pings us, or timeout. */ /* The routine to call when the Guest pings us, or timeout. */
void (*handle_output)(int fd, struct virtqueue *me, bool timeout); void (*handle_output)(struct virtqueue *me, bool timeout);
/* Outstanding buffers */ /* Outstanding buffers */
unsigned int inflight; unsigned int inflight;
...@@ -509,21 +510,16 @@ static void concat(char *dst, char *args[]) ...@@ -509,21 +510,16 @@ static void concat(char *dst, char *args[])
* saw the arguments it expects when we looked at initialize() in lguest_user.c: * saw the arguments it expects when we looked at initialize() in lguest_user.c:
* the base of Guest "physical" memory, the top physical page to allow and the * the base of Guest "physical" memory, the top physical page to allow and the
* entry point for the Guest. */ * entry point for the Guest. */
static int tell_kernel(unsigned long start) static void tell_kernel(unsigned long start)
{ {
unsigned long args[] = { LHREQ_INITIALIZE, unsigned long args[] = { LHREQ_INITIALIZE,
(unsigned long)guest_base, (unsigned long)guest_base,
guest_limit / getpagesize(), start }; guest_limit / getpagesize(), start };
int fd;
verbose("Guest: %p - %p (%#lx)\n", verbose("Guest: %p - %p (%#lx)\n",
guest_base, guest_base + guest_limit, guest_limit); guest_base, guest_base + guest_limit, guest_limit);
fd = open_or_die("/dev/lguest", O_RDWR); lguest_fd = open_or_die("/dev/lguest", O_RDWR);
if (write(fd, args, sizeof(args)) < 0) if (write(lguest_fd, args, sizeof(args)) < 0)
err(1, "Writing to /dev/lguest"); err(1, "Writing to /dev/lguest");
/* We return the /dev/lguest file descriptor to control this Guest */
return fd;
} }
/*:*/ /*:*/
...@@ -583,21 +579,18 @@ static int waker(void *unused) ...@@ -583,21 +579,18 @@ static int waker(void *unused)
} }
/* Send LHREQ_BREAK command to snap the Launcher out of it. */ /* Send LHREQ_BREAK command to snap the Launcher out of it. */
pwrite(waker_fds.lguest_fd, args, sizeof(args), cpu_id); pwrite(lguest_fd, args, sizeof(args), cpu_id);
} }
return 0; return 0;
} }
/* This routine just sets up a pipe to the Waker process. */ /* This routine just sets up a pipe to the Waker process. */
static void setup_waker(int lguest_fd) static void setup_waker(void)
{ {
/* This pipe is closed when Launcher dies, telling Waker. */ /* This pipe is closed when Launcher dies, telling Waker. */
if (pipe(waker_fds.pipe) != 0) if (pipe(waker_fds.pipe) != 0)
err(1, "Creating pipe for Waker"); err(1, "Creating pipe for Waker");
/* Waker also needs to know the lguest fd */
waker_fds.lguest_fd = lguest_fd;
if (clone(waker, malloc(4096) + 4096, CLONE_VM | SIGCHLD, NULL) == -1) if (clone(waker, malloc(4096) + 4096, CLONE_VM | SIGCHLD, NULL) == -1)
err(1, "Creating Waker"); err(1, "Creating Waker");
} }
...@@ -727,7 +720,7 @@ static void add_used(struct virtqueue *vq, unsigned int head, int len) ...@@ -727,7 +720,7 @@ static void add_used(struct virtqueue *vq, unsigned int head, int len)
} }
/* This actually sends the interrupt for this virtqueue */ /* This actually sends the interrupt for this virtqueue */
static void trigger_irq(int fd, struct virtqueue *vq) static void trigger_irq(struct virtqueue *vq)
{ {
unsigned long buf[] = { LHREQ_IRQ, vq->config.irq }; unsigned long buf[] = { LHREQ_IRQ, vq->config.irq };
...@@ -737,16 +730,15 @@ static void trigger_irq(int fd, struct virtqueue *vq) ...@@ -737,16 +730,15 @@ static void trigger_irq(int fd, struct virtqueue *vq)
return; return;
/* Send the Guest an interrupt tell them we used something up. */ /* Send the Guest an interrupt tell them we used something up. */
if (write(fd, buf, sizeof(buf)) != 0) if (write(lguest_fd, buf, sizeof(buf)) != 0)
err(1, "Triggering irq %i", vq->config.irq); err(1, "Triggering irq %i", vq->config.irq);
} }
/* And here's the combo meal deal. Supersize me! */ /* And here's the combo meal deal. Supersize me! */
static void add_used_and_trigger(int fd, struct virtqueue *vq, static void add_used_and_trigger(struct virtqueue *vq, unsigned head, int len)
unsigned int head, int len)
{ {
add_used(vq, head, len); add_used(vq, head, len);
trigger_irq(fd, vq); trigger_irq(vq);
} }
/* /*
...@@ -770,7 +762,7 @@ struct console_abort ...@@ -770,7 +762,7 @@ struct console_abort
}; };
/* This is the routine which handles console input (ie. stdin). */ /* This is the routine which handles console input (ie. stdin). */
static bool handle_console_input(int fd, struct device *dev) static bool handle_console_input(struct device *dev)
{ {
int len; int len;
unsigned int head, in_num, out_num; unsigned int head, in_num, out_num;
...@@ -804,7 +796,7 @@ static bool handle_console_input(int fd, struct device *dev) ...@@ -804,7 +796,7 @@ static bool handle_console_input(int fd, struct device *dev)
} }
/* Tell the Guest about the new input. */ /* Tell the Guest about the new input. */
add_used_and_trigger(fd, dev->vq, head, len); add_used_and_trigger(dev->vq, head, len);
/* Three ^C within one second? Exit. /* Three ^C within one second? Exit.
* *
...@@ -824,7 +816,7 @@ static bool handle_console_input(int fd, struct device *dev) ...@@ -824,7 +816,7 @@ static bool handle_console_input(int fd, struct device *dev)
close(waker_fds.pipe[1]); close(waker_fds.pipe[1]);
/* Just in case Waker is blocked in BREAK, send /* Just in case Waker is blocked in BREAK, send
* unbreak now. */ * unbreak now. */
write(fd, args, sizeof(args)); write(lguest_fd, args, sizeof(args));
exit(2); exit(2);
} }
abort->count = 0; abort->count = 0;
...@@ -839,7 +831,7 @@ static bool handle_console_input(int fd, struct device *dev) ...@@ -839,7 +831,7 @@ static bool handle_console_input(int fd, struct device *dev)
/* Handling output for console is simple: we just get all the output buffers /* Handling output for console is simple: we just get all the output buffers
* and write them to stdout. */ * and write them to stdout. */
static void handle_console_output(int fd, struct virtqueue *vq, bool timeout) static void handle_console_output(struct virtqueue *vq, bool timeout)
{ {
unsigned int head, out, in; unsigned int head, out, in;
int len; int len;
...@@ -850,7 +842,7 @@ static void handle_console_output(int fd, struct virtqueue *vq, bool timeout) ...@@ -850,7 +842,7 @@ static void handle_console_output(int fd, struct virtqueue *vq, bool timeout)
if (in) if (in)
errx(1, "Input buffers in output queue?"); errx(1, "Input buffers in output queue?");
len = writev(STDOUT_FILENO, iov, out); len = writev(STDOUT_FILENO, iov, out);
add_used_and_trigger(fd, vq, head, len); add_used_and_trigger(vq, head, len);
} }
} }
...@@ -879,7 +871,7 @@ static void block_vq(struct virtqueue *vq) ...@@ -879,7 +871,7 @@ static void block_vq(struct virtqueue *vq)
* and write them (ignoring the first element) to this device's file descriptor * and write them (ignoring the first element) to this device's file descriptor
* (/dev/net/tun). * (/dev/net/tun).
*/ */
static void handle_net_output(int fd, struct virtqueue *vq, bool timeout) static void handle_net_output(struct virtqueue *vq, bool timeout)
{ {
unsigned int head, out, in, num = 0; unsigned int head, out, in, num = 0;
int len; int len;
...@@ -893,7 +885,7 @@ static void handle_net_output(int fd, struct virtqueue *vq, bool timeout) ...@@ -893,7 +885,7 @@ static void handle_net_output(int fd, struct virtqueue *vq, bool timeout)
len = writev(vq->dev->fd, iov, out); len = writev(vq->dev->fd, iov, out);
if (len < 0) if (len < 0)
err(1, "Writing network packet to tun"); err(1, "Writing network packet to tun");
add_used_and_trigger(fd, vq, head, len); add_used_and_trigger(vq, head, len);
num++; num++;
} }
...@@ -917,7 +909,7 @@ static void handle_net_output(int fd, struct virtqueue *vq, bool timeout) ...@@ -917,7 +909,7 @@ static void handle_net_output(int fd, struct virtqueue *vq, bool timeout)
/* This is where we handle a packet coming in from the tun device to our /* This is where we handle a packet coming in from the tun device to our
* Guest. */ * Guest. */
static bool handle_tun_input(int fd, struct device *dev) static bool handle_tun_input(struct device *dev)
{ {
unsigned int head, in_num, out_num; unsigned int head, in_num, out_num;
int len; int len;
...@@ -946,7 +938,7 @@ static bool handle_tun_input(int fd, struct device *dev) ...@@ -946,7 +938,7 @@ static bool handle_tun_input(int fd, struct device *dev)
err(1, "reading network"); err(1, "reading network");
/* Tell the Guest about the new packet. */ /* Tell the Guest about the new packet. */
add_used_and_trigger(fd, dev->vq, head, len); add_used_and_trigger(dev->vq, head, len);
verbose("tun input packet len %i [%02x %02x] (%s)\n", len, verbose("tun input packet len %i [%02x %02x] (%s)\n", len,
((u8 *)iov[1].iov_base)[0], ((u8 *)iov[1].iov_base)[1], ((u8 *)iov[1].iov_base)[0], ((u8 *)iov[1].iov_base)[1],
...@@ -959,18 +951,18 @@ static bool handle_tun_input(int fd, struct device *dev) ...@@ -959,18 +951,18 @@ static bool handle_tun_input(int fd, struct device *dev)
/*L:215 This is the callback attached to the network and console input /*L:215 This is the callback attached to the network and console input
* virtqueues: it ensures we try again, in case we stopped console or net * virtqueues: it ensures we try again, in case we stopped console or net
* delivery because Guest didn't have any buffers. */ * delivery because Guest didn't have any buffers. */
static void enable_fd(int fd, struct virtqueue *vq, bool timeout) static void enable_fd(struct virtqueue *vq, bool timeout)
{ {
add_device_fd(vq->dev->fd); add_device_fd(vq->dev->fd);
/* Snap the Waker out of its select loop. */ /* Snap the Waker out of its select loop. */
write(waker_fds.pipe[1], "", 1); write(waker_fds.pipe[1], "", 1);
} }
static void net_enable_fd(int fd, struct virtqueue *vq, bool timeout) static void net_enable_fd(struct virtqueue *vq, bool timeout)
{ {
/* We don't need to know again when Guest refills receive buffer. */ /* We don't need to know again when Guest refills receive buffer. */
vq->vring.used->flags |= VRING_USED_F_NO_NOTIFY; vq->vring.used->flags |= VRING_USED_F_NO_NOTIFY;
enable_fd(fd, vq, timeout); enable_fd(vq, timeout);
} }
/* When the Guest tells us they updated the status field, we handle it. */ /* When the Guest tells us they updated the status field, we handle it. */
...@@ -1011,7 +1003,7 @@ static void update_device_status(struct device *dev) ...@@ -1011,7 +1003,7 @@ static void update_device_status(struct device *dev)
} }
/* This is the generic routine we call when the Guest uses LHCALL_NOTIFY. */ /* This is the generic routine we call when the Guest uses LHCALL_NOTIFY. */
static void handle_output(int fd, unsigned long addr) static void handle_output(unsigned long addr)
{ {
struct device *i; struct device *i;
struct virtqueue *vq; struct virtqueue *vq;
...@@ -1039,7 +1031,7 @@ static void handle_output(int fd, unsigned long addr) ...@@ -1039,7 +1031,7 @@ static void handle_output(int fd, unsigned long addr)
if (strcmp(vq->dev->name, "console") != 0) if (strcmp(vq->dev->name, "console") != 0)
verbose("Output to %s\n", vq->dev->name); verbose("Output to %s\n", vq->dev->name);
if (vq->handle_output) if (vq->handle_output)
vq->handle_output(fd, vq, false); vq->handle_output(vq, false);
return; return;
} }
} }
...@@ -1053,7 +1045,7 @@ static void handle_output(int fd, unsigned long addr) ...@@ -1053,7 +1045,7 @@ static void handle_output(int fd, unsigned long addr)
strnlen(from_guest_phys(addr), guest_limit - addr)); strnlen(from_guest_phys(addr), guest_limit - addr));
} }
static void handle_timeout(int fd) static void handle_timeout(void)
{ {
char buf[32]; char buf[32];
struct device *i; struct device *i;
...@@ -1071,14 +1063,14 @@ static void handle_timeout(int fd) ...@@ -1071,14 +1063,14 @@ static void handle_timeout(int fd)
vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY; vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY;
vq->blocked = false; vq->blocked = false;
if (vq->handle_output) if (vq->handle_output)
vq->handle_output(fd, vq, true); vq->handle_output(vq, true);
} }
} }
} }
/* This is called when the Waker wakes us up: check for incoming file /* This is called when the Waker wakes us up: check for incoming file
* descriptors. */ * descriptors. */
static void handle_input(int fd) static void handle_input(void)
{ {
/* select() wants a zeroed timeval to mean "don't wait". */ /* select() wants a zeroed timeval to mean "don't wait". */
struct timeval poll = { .tv_sec = 0, .tv_usec = 0 }; struct timeval poll = { .tv_sec = 0, .tv_usec = 0 };
...@@ -1100,7 +1092,7 @@ static void handle_input(int fd) ...@@ -1100,7 +1092,7 @@ static void handle_input(int fd)
* descriptors and a method of handling them. */ * descriptors and a method of handling them. */
for (i = devices.dev; i; i = i->next) { for (i = devices.dev; i; i = i->next) {
if (i->handle_input && FD_ISSET(i->fd, &fds)) { if (i->handle_input && FD_ISSET(i->fd, &fds)) {
if (i->handle_input(fd, i)) if (i->handle_input(i))
continue; continue;
/* If handle_input() returns false, it means we /* If handle_input() returns false, it means we
...@@ -1114,7 +1106,7 @@ static void handle_input(int fd) ...@@ -1114,7 +1106,7 @@ static void handle_input(int fd)
/* Is this the timeout fd? */ /* Is this the timeout fd? */
if (FD_ISSET(timeoutpipe[0], &fds)) if (FD_ISSET(timeoutpipe[0], &fds))
handle_timeout(fd); handle_timeout();
} }
} }
...@@ -1163,7 +1155,7 @@ static struct lguest_device_desc *new_dev_desc(u16 type) ...@@ -1163,7 +1155,7 @@ static struct lguest_device_desc *new_dev_desc(u16 type)
/* Each device descriptor is followed by the description of its virtqueues. We /* Each device descriptor is followed by the description of its virtqueues. We
* specify how many descriptors the virtqueue is to have. */ * specify how many descriptors the virtqueue is to have. */
static void add_virtqueue(struct device *dev, unsigned int num_descs, static void add_virtqueue(struct device *dev, unsigned int num_descs,
void (*handle_output)(int, struct virtqueue *, bool)) void (*handle_output)(struct virtqueue *, bool))
{ {
unsigned int pages; unsigned int pages;
struct virtqueue **i, *vq = malloc(sizeof(*vq)); struct virtqueue **i, *vq = malloc(sizeof(*vq));
...@@ -1249,7 +1241,7 @@ static void set_config(struct device *dev, unsigned len, const void *conf) ...@@ -1249,7 +1241,7 @@ static void set_config(struct device *dev, unsigned len, const void *conf)
* *
* See what I mean about userspace being boring? */ * See what I mean about userspace being boring? */
static struct device *new_device(const char *name, u16 type, int fd, static struct device *new_device(const char *name, u16 type, int fd,
bool (*handle_input)(int, struct device *)) bool (*handle_input)(struct device *))
{ {
struct device *dev = malloc(sizeof(*dev)); struct device *dev = malloc(sizeof(*dev));
...@@ -1678,7 +1670,7 @@ static int io_thread(void *_dev) ...@@ -1678,7 +1670,7 @@ static int io_thread(void *_dev)
/* Now we've seen the I/O thread, we return to the Launcher to see what happens /* Now we've seen the I/O thread, we return to the Launcher to see what happens
* when that thread tells us it's completed some I/O. */ * when that thread tells us it's completed some I/O. */
static bool handle_io_finish(int fd, struct device *dev) static bool handle_io_finish(struct device *dev)
{ {
char c; char c;
...@@ -1688,12 +1680,12 @@ static bool handle_io_finish(int fd, struct device *dev) ...@@ -1688,12 +1680,12 @@ static bool handle_io_finish(int fd, struct device *dev)
exit(1); exit(1);
/* It did some work, so trigger the irq. */ /* It did some work, so trigger the irq. */
trigger_irq(fd, dev->vq); trigger_irq(dev->vq);
return true; return true;
} }
/* When the Guest submits some I/O, we just need to wake the I/O thread. */ /* When the Guest submits some I/O, we just need to wake the I/O thread. */
static void handle_virtblk_output(int fd, struct virtqueue *vq, bool timeout) static void handle_virtblk_output(struct virtqueue *vq, bool timeout)
{ {
struct vblk_info *vblk = vq->dev->priv; struct vblk_info *vblk = vq->dev->priv;
char c = 0; char c = 0;
...@@ -1771,7 +1763,7 @@ static void setup_block_file(const char *filename) ...@@ -1771,7 +1763,7 @@ static void setup_block_file(const char *filename)
* console is the reverse. * console is the reverse.
* *
* The same logic applies, however. */ * The same logic applies, however. */
static bool handle_rng_input(int fd, struct device *dev) static bool handle_rng_input(struct device *dev)
{ {
int len; int len;
unsigned int head, in_num, out_num, totlen = 0; unsigned int head, in_num, out_num, totlen = 0;
...@@ -1800,7 +1792,7 @@ static bool handle_rng_input(int fd, struct device *dev) ...@@ -1800,7 +1792,7 @@ static bool handle_rng_input(int fd, struct device *dev)
} }
/* Tell the Guest about the new input. */ /* Tell the Guest about the new input. */
add_used_and_trigger(fd, dev->vq, head, totlen); add_used_and_trigger(dev->vq, head, totlen);
/* Everything went OK! */ /* Everything went OK! */
return true; return true;
...@@ -1841,7 +1833,7 @@ static void __attribute__((noreturn)) restart_guest(void) ...@@ -1841,7 +1833,7 @@ static void __attribute__((noreturn)) restart_guest(void)
/*L:220 Finally we reach the core of the Launcher which runs the Guest, serves /*L:220 Finally we reach the core of the Launcher which runs the Guest, serves
* its input and output, and finally, lays it to rest. */ * its input and output, and finally, lays it to rest. */
static void __attribute__((noreturn)) run_guest(int lguest_fd) static void __attribute__((noreturn)) run_guest(void)
{ {
for (;;) { for (;;) {
unsigned long args[] = { LHREQ_BREAK, 0 }; unsigned long args[] = { LHREQ_BREAK, 0 };
...@@ -1855,7 +1847,7 @@ static void __attribute__((noreturn)) run_guest(int lguest_fd) ...@@ -1855,7 +1847,7 @@ static void __attribute__((noreturn)) run_guest(int lguest_fd)
/* One unsigned long means the Guest did HCALL_NOTIFY */ /* One unsigned long means the Guest did HCALL_NOTIFY */
if (readval == sizeof(notify_addr)) { if (readval == sizeof(notify_addr)) {
verbose("Notify on address %#lx\n", notify_addr); verbose("Notify on address %#lx\n", notify_addr);
handle_output(lguest_fd, notify_addr); handle_output(notify_addr);
continue; continue;
/* ENOENT means the Guest died. Reading tells us why. */ /* ENOENT means the Guest died. Reading tells us why. */
} else if (errno == ENOENT) { } else if (errno == ENOENT) {
...@@ -1875,7 +1867,7 @@ static void __attribute__((noreturn)) run_guest(int lguest_fd) ...@@ -1875,7 +1867,7 @@ static void __attribute__((noreturn)) run_guest(int lguest_fd)
continue; continue;
/* Service input, then unset the BREAK to release the Waker. */ /* Service input, then unset the BREAK to release the Waker. */
handle_input(lguest_fd); handle_input();
if (pwrite(lguest_fd, args, sizeof(args), cpu_id) < 0) if (pwrite(lguest_fd, args, sizeof(args), cpu_id) < 0)
err(1, "Resetting break"); err(1, "Resetting break");
} }
...@@ -1911,8 +1903,8 @@ int main(int argc, char *argv[]) ...@@ -1911,8 +1903,8 @@ int main(int argc, char *argv[])
/* Memory, top-level pagetable, code startpoint and size of the /* Memory, top-level pagetable, code startpoint and size of the
* (optional) initrd. */ * (optional) initrd. */
unsigned long mem = 0, start, initrd_size = 0; unsigned long mem = 0, start, initrd_size = 0;
/* Two temporaries and the /dev/lguest file descriptor. */ /* Two temporaries. */
int i, c, lguest_fd; int i, c;
/* The boot information for the Guest. */ /* The boot information for the Guest. */
struct boot_params *boot; struct boot_params *boot;
/* If they specify an initrd file to load. */ /* If they specify an initrd file to load. */
...@@ -2030,15 +2022,15 @@ int main(int argc, char *argv[]) ...@@ -2030,15 +2022,15 @@ int main(int argc, char *argv[])
/* We tell the kernel to initialize the Guest: this returns the open /* We tell the kernel to initialize the Guest: this returns the open
* /dev/lguest file descriptor. */ * /dev/lguest file descriptor. */
lguest_fd = tell_kernel(start); tell_kernel(start);
/* We clone off a thread, which wakes the Launcher whenever one of the /* We clone off a thread, which wakes the Launcher whenever one of the
* input file descriptors needs attention. We call this the Waker, and * input file descriptors needs attention. We call this the Waker, and
* we'll cover it in a moment. */ * we'll cover it in a moment. */
setup_waker(lguest_fd); setup_waker();
/* Finally, run the Guest. This doesn't return. */ /* Finally, run the Guest. This doesn't return. */
run_guest(lguest_fd); run_guest();
} }
/*:*/ /*:*/
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment