Re: [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong <akong@redhat.com>]

On 14/03/12 22:58, Michael Roth wrote:
> On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote:
>> On 14/03/12 00:39, Michael Roth wrote:
>>> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
>>>> Introduce tcp_server_start() by moving original code in
>>>> tcp_start_incoming_migration().
>>>>
>>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>>> ---
>>>>   net.c         |   28 ++++++++++++++++++++++++++++
>>>>   qemu_socket.h |    2 ++
>>>>   2 files changed, 30 insertions(+), 0 deletions(-)
>>>>
>>>> +int tcp_server_start(const char *str, int *fd)
>>>> +{

>>> I would combine this with patch 2, since it provides context for why
>>> this function is being added. Would also do the same for 3 and 4.
>>>
>>> I see client the client implementation you need to pass fd back by
>>> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,
>>
>> ret restores 0 or -socket_error()
>>   success: 0, -EINPROGRESS
>>   fail   : ret<  0&&  ret !=-EINTR&&  ret != -EWOULDBLOCK
>>
>> , it should be -EINPROGRESS
>
> I see, I think I was confued by patch #4 where you do a
>
> +    ret = tcp_client_start(host_port,&s->fd);
> +    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> +        DPRINTF("connect in progress");
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>
> If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to
> return EWOULDBLOCK), we should fail it there rather than passing it on to
> tcp_wait_for_connect().

You are right, it should be :
        if (ret == -EINPROGRESS) {

>>> Also, is there any reason we can't re-use
>>> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
>>> serve the same purpose, and already include some of the work from your
>>> PATCH #6.
>>
>> We could not directly use it, there are some difference,
>> such as tcp_start_incoming_migration() doesn't set socket no-blocked,
>> but net_socket_listen_init() sets socket no-blocked.
>
> I think adding a common function with blocking/non-blocking flag and
> having inet_listen_opts()/socket_listen_opts() call it with a wrapper
> would be reasonable.

> A lot of code is being introduced here to solve problems that are
> already handled in qemu-sockets.c. inet_listen()/inet_connect()
> already handles backeted-enclosed ipv6 addrs, getting port numbers when
> there's more than one colon, getaddrinfo()-based connections, and most
> importantly it's had ipv6 support from day 1.

> Not 100% sure it'll work for what you're doing, but qemu-sockets.c was
> specifically added for this type of use-case and is heavilly used
> currently (vnc, nbd, Chardev users), so I think we should use it unless
> there's a good reason not to.

There are many special request for migration, which is not implemented in
inet_listen_opts()/socket_listen_opts(), but many codes can be reused,
I would re-write patches.

Thanks, Amos