[Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 12/03/10 19:03, Michael Roth wrote:
> This allows us to implement an i/o loop outside of vl.c that can
> interact with objects that use qemu_set_fd_handler()
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

This commit message really tells us nothing. Please be more specific
about what is in the commit.

> diff --git a/qemu-ioh.c b/qemu-ioh.c
> new file mode 100644
> index 0000000..cc71470
> --- /dev/null
> +++ b/qemu-ioh.c
> @@ -0,0 +1,115 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:

Is this moved or new code? If the former, fine, but if it is new code,
you might want to leave your own name on the (c). I presume at least
some of the changes are (c) 2010?

> +/* XXX: fd_read_poll should be suppressed, but an API change is
> +   necessary in the character devices to suppress fd_can_read(). */

XXX in the comment isn't really of much use. Please make it more
explicit, or put your name in if it is a statement you wish to make.

> +int qemu_set_fd_handler3(void *ioh_record_list,
> +                         int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque)

I am not happy with this addition of numbers to these functions, it
doesn't tell us why we have a 3 and how it differs from 2. If 3 is
really the backend for implementing 2, maybe it would be better to name
it __qemu_set_fd_handler2() and then have macros/wrappers calling into it.

Cheers,
Jes