[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

  Hi,


Moving the handlers to a separate struct is clearly a incremental
cleanup which can follow later.  Using enable/disable flags will
probably simplify the interfaces for the non-blocking mode and thus
simplify the whole patch series so I think this should be done now.


Agree -- but it looks to be a big patch.  I have some initial work done,
and hence am not converting anything other than unix/tcp backends.  The
proposed interface here is local to this file, and just a couple of
callers.  No big deal to change it once this is in.

(The struct for that will look like:

struct fd_handler {
int fd;

IOHandler *read;
IOHandler *write;
IOCanReadHandler *read_poll;

bool read_enabled, write_enabled, read_poll_enabled;

void (*set_read_handler)(IOHandler *read_handler);
void (*set_write_handler)(IOHandler *write_handler);
void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler);
}


No, that isn't what I have in mind ...

I'm thinking more about this:

  (1) IOHandlerRecord gets variables to enable/disable the handlers
  (a bunch of bools, a bitmask, whatever).  They default to enabled
  to maintain backward compatibility.
  (2) One or more functions are added to enable/disable the handlers,
  something like qemu_fd_check_read(int fd, bool enable);
  (3) main_loop_wait() will check whenever the handler is actually
  enabled before adding it to the file handle set for the select()
  call.

This shouldn't be that big.  And with this initial stuff in place you 
can go forward with the non-blocking stuff.  No need to pass around the 
write handler or have callbacks just to enable/disable the checking for 
a writable fd, the non-blocking code can just call 
qemu_fd_check_write(fd, true/false) to do it.


Additional cleanups possible:

  (1) switch everybody who registers/unregisters handlers to the new
  enable/disable interface.
  (2) move the function pointers from IOHandlerRecord to a separate
  struct (say IOHandlerOps).


Also: we also want to be able to select() on all fds so that we can
detect disconnection events as they happen.  So we also need an array
somewhere.)


I think you can't do that without switching from select() to poll().

cheers,
  Gerd

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

On 01/11/11 12:10, Amit Shah wrote:

Introduce a char-specific wrapper to qemu_set_fd_handler functions.
This wrapper is useful to add / remove a write handler easily.  Write
handlers are only used when the backend is blocked and cannot receive
any more input.


I'd suggest to add flags to enable/disable handlers to IOHandlerRecord 
instead.  And helper functions to set/clear them of course.


With that in place you also can move the handlers to a separate struct 
simliar to the new QemuChrHandlers struct from patch #1.


cheers,
  Gerd

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

On 01/11/11 16:38, Amit Shah wrote:

On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:

On 01/11/11 12:10, Amit Shah wrote:

Introduce a char-specific wrapper to qemu_set_fd_handler functions.
This wrapper is useful to add / remove a write handler easily.  Write
handlers are only used when the backend is blocked and cannot receive
any more input.


I'd suggest to add flags to enable/disable handlers to
IOHandlerRecord instead.  And helper functions to set/clear them of
course.

With that in place you also can move the handlers to a separate
struct simliar to the new QemuChrHandlers struct from patch #1.


I'm planning to do that later -- when more backends get involved, which
have multiple fds (one for in, one for out).


Moving the handlers to a separate struct is clearly a incremental 
cleanup which can follow later.  Using enable/disable flags will 
probably simplify the interfaces for the non-blocking mode and thus 
simplify the whole patch series so I think this should be done now.


cheers,
  Gerd

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Amit Shah]

On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:
 On 01/11/11 12:10, Amit Shah wrote:
 Introduce a char-specific wrapper to qemu_set_fd_handler functions.
 This wrapper is useful to add / remove a write handler easily.  Write
 handlers are only used when the backend is blocked and cannot receive
 any more input.
 
 I'd suggest to add flags to enable/disable handlers to
 IOHandlerRecord instead.  And helper functions to set/clear them of
 course.
 
 With that in place you also can move the handlers to a separate
 struct simliar to the new QemuChrHandlers struct from patch #1.

I'm planning to do that later -- when more backends get involved, which
have multiple fds (one for in, one for out).

Are you OK with this for now (to solve the immediate bugs of guests
freezing if host can't flush data) and doing this cleanup later as we
progress?

Amit

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Amit Shah]

On (Tue) Jan 11 2011 [16:54:48], Gerd Hoffmann wrote:
 On 01/11/11 16:38, Amit Shah wrote:
 On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:
 On 01/11/11 12:10, Amit Shah wrote:
 Introduce a char-specific wrapper to qemu_set_fd_handler functions.
 This wrapper is useful to add / remove a write handler easily.  Write
 handlers are only used when the backend is blocked and cannot receive
 any more input.
 
 I'd suggest to add flags to enable/disable handlers to
 IOHandlerRecord instead.  And helper functions to set/clear them of
 course.
 
 With that in place you also can move the handlers to a separate
 struct simliar to the new QemuChrHandlers struct from patch #1.
 
 I'm planning to do that later -- when more backends get involved, which
 have multiple fds (one for in, one for out).
 
 Moving the handlers to a separate struct is clearly a incremental
 cleanup which can follow later.  Using enable/disable flags will
 probably simplify the interfaces for the non-blocking mode and thus
 simplify the whole patch series so I think this should be done now.

Agree -- but it looks to be a big patch.  I have some initial work done,
and hence am not converting anything other than unix/tcp backends.  The
proposed interface here is local to this file, and just a couple of
callers.  No big deal to change it once this is in.

(The struct for that will look like:

struct fd_handler {
int fd;

IOHandler *read;
IOHandler *write;
IOCanReadHandler *read_poll;

bool read_enabled, write_enabled, read_poll_enabled;

void (*set_read_handler)(IOHandler *read_handler);
void (*set_write_handler)(IOHandler *write_handler);
void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler);
}

This has to be embedded in the CharDriverState for each fd for each
backend.

Also: we also want to be able to select() on all fds so that we can
detect disconnection events as they happen.  So we also need an array
somewhere.)

Amit