[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone da
From: |
Jes Sorensen |
Subject: |
Re: [Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone daemon skeleton |
Date: |
Thu, 18 Nov 2010 12:04:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.6 |
On 11/16/10 02:15, Michael Roth wrote:
> Daemon to be run in guest, or on host in standalone mode.
> (re-)implements some qemu utility functions used by core virtproxy.c
> code via wrapper functions. For built-in virtproxy code we will define
> these wrapper functions in terms of qemu's built-in implementations.
>
> Main logic will come in a later patch.
>
> Signed-off-by: Michael Roth <address@hidden>
Hi Michael,
A couple of comments:
> +/* mirror qemu I/O-related code for standalone daemon */
> +typedef struct IOHandlerRecord {
> + int fd;
> + IOCanReadHandler *fd_read_poll;
> + IOHandler *fd_read;
> + IOHandler *fd_write;
> + int deleted;
> + void *opaque;
> + /* temporary data */
> + struct pollfd *ufd;
> + QLIST_ENTRY(IOHandlerRecord) next;
> +} IOHandlerRecord;
Please move this to a header file. Any chance you could avoid some of
all those ugly typedefs too? I know we have way too many of the already,
but if it doesn't need to be a typedef, it's better not to make it one.
> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> + QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +int vp_set_fd_handler2(int fd,
> + IOCanReadHandler *fd_read_poll,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
The formatting here is really odd, please make sure to align all
arguments, and maybe compress the lines a bit so it doesn't take up as
much realestate when you read the code.
> +int vp_send_all(int fd, const void *buf, int len1)
> +{
> + int ret, len;
> +
> + len = len1;
> + while (len > 0) {
> + ret = write(fd, buf, len);
> + if (ret < 0) {
> + if (errno != EINTR && errno != EAGAIN) {
> + warn("write() failed");
> + return -1;
Is -1 really an ideal error value to return here?
> +static void main_loop_wait(int nonblocking)
> +{
> + IOHandlerRecord *ioh;
> + fd_set rfds, wfds, xfds;
> + int ret, nfds;
> + struct timeval tv;
No good, please use qemu_timeval and friends for compatibility.
> + int timeout = 1000;
> +
> + if (nonblocking) {
> + timeout = 0;
> + }
> +
> + /* poll any events */
> + nfds = -1;
> + FD_ZERO(&rfds);
> + FD_ZERO(&wfds);
> + FD_ZERO(&xfds);
> + QLIST_FOREACH(ioh, &io_handlers, next) {
> + if (ioh->deleted)
> + continue;
Missing braces.
> + if (ioh->fd_read &&
> + (!ioh->fd_read_poll ||
> + ioh->fd_read_poll(ioh->opaque) != 0)) {
Put the || arguments on the same line so it is easier to read.
> + FD_SET(ioh->fd, &rfds);
> + if (ioh->fd > nfds)
> + nfds = ioh->fd;
Missing braces.
> + }
> + if (ioh->fd_write) {
> + FD_SET(ioh->fd, &wfds);
> + if (ioh->fd > nfds)
> + nfds = ioh->fd;
and again.
Cheers,
Jes
- Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants, (continued)
[Qemu-devel] [RFC][PATCH v3 04/21] virtproxy: list look-up functions conns/oforwards/iforwards, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone daemon skeleton, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 05/21] virtproxy, add vp_channel_send_all, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 06/21] virtproxy: add accept handler for communication channel, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 07/21] virtproxy: add read handler for communication channel, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 08/21] virtproxy: add vp_new() VPDriver constructor, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 09/21] virtproxy: interfaces to set/remove/handle VPOForwards, Michael Roth, 2010/11/15
[Qemu-devel] [RFC][PATCH v3 10/21] virtproxy: add handler for data packets, Michael Roth, 2010/11/15