Re-adding the mailing list, please don't drop the list in
replies to discussions.
On Wed, Jul 20, 2022 at 02:08:23AM +0530, Het Gala wrote:
>
> On 13/07/22 3:10 pm, Het Gala wrote:
> >
> > On 16/06/22 11:09 pm, Daniel P. Berrangé wrote:
> > > On Thu, Jun 09, 2022 at 07:33:04AM +0000, Het Gala wrote:
> > > > i) Binding of the socket to source ip address and port on the
> > > > non-default
> > > > interface has been implemented for multi-FD connection,
> > > > which was not
> > > > necessary earlier because the binding was on the default
> > > > interface itself.
> > > >
> > > > ii) Created an end to end connection between all multi-FD source and
> > > > destination pairs.
> > > >
> > > > Suggested-by: Manish Mishra <manish.mis...@nutanix.com>
> > > > Signed-off-by: Het Gala <het.g...@nutanix.com>
> > > > ---
> > > > chardev/char-socket.c | 4 +-
> > > > include/io/channel-socket.h | 26 ++++++-----
> > > > include/qemu/sockets.h | 6 ++-
> > > > io/channel-socket.c | 50 ++++++++++++++------
> > > > migration/socket.c | 15 +++---
> > > > nbd/client-connection.c | 2 +-
> > > > qemu-nbd.c | 4 +-
> > > > scsi/pr-manager-helper.c | 1 +
> > > > tests/unit/test-char.c | 8 ++--
> > > > tests/unit/test-io-channel-socket.c | 4 +-
> > > > tests/unit/test-util-sockets.c | 16 +++----
> > > > ui/input-barrier.c | 2 +-
> > > > ui/vnc.c | 3 +-
> > > > util/qemu-sockets.c | 71
> > > > ++++++++++++++++++++---------
> > > > 14 files changed, 135 insertions(+), 77 deletions(-)
> > > >
> > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > index dc4e218eeb..f3725238c5 100644
> > > > --- a/chardev/char-socket.c
> > > > +++ b/chardev/char-socket.c
> > > > @@ -932,7 +932,7 @@ static int
> > > > tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
> > > > QIOChannelSocket *sioc = qio_channel_socket_new();
> > > > tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
> > > > tcp_chr_set_client_ioc_name(chr, sioc);
> > > > - if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> > > > + if (qio_channel_socket_connect_sync(sioc, s->addr, NULL,
> > > > errp) < 0) {
> > > > tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
> > > > object_unref(OBJECT(sioc));
> > > > return -1;
> > > > @@ -1120,7 +1120,7 @@ static void
> > > > tcp_chr_connect_client_task(QIOTask *task,
> > > > SocketAddress *addr = opaque;
> > > > Error *err = NULL;
> > > > - qio_channel_socket_connect_sync(ioc, addr, &err);
> > > > + qio_channel_socket_connect_sync(ioc, addr, NULL, &err);
> > > > qio_task_set_error(task, err);
> > > > }
> > > > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > > > index 513c428fe4..59d5b1b349 100644
> > > > --- a/include/io/channel-socket.h
> > > > +++ b/include/io/channel-socket.h
> > > > @@ -83,41 +83,45 @@ qio_channel_socket_new_fd(int fd,
> > > > /**
> > > > * qio_channel_socket_connect_sync:
> > > > * @ioc: the socket channel object
> > > > - * @addr: the address to connect to
> > > > + * @dst_addr: the destination address to connect to
> > > > + * @src_addr: the source address to be connected
> > > > * @errp: pointer to a NULL-initialized error object
> > > > *
> > > > - * Attempt to connect to the address @addr. This method
> > > > - * will run in the foreground so the caller will not regain
> > > > - * execution control until the connection is established or
> > > > + * Attempt to connect to the address @dst_addr with @src_addr.
> > > > + * This method will run in the foreground so the caller will not
> > > > + * regain execution control until the connection is established or
> > > > * an error occurs.
> > > > */
> > > > int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
> > > > - SocketAddress *addr,
> > > > + SocketAddress *dst_addr,
> > > > + SocketAddress *src_addr,
> > > > Error **errp);
> > > > /**
> > > > * qio_channel_socket_connect_async:
> > > > * @ioc: the socket channel object
> > > > - * @addr: the address to connect to
> > > > + * @dst_addr: the destination address to connect to
> > > > * @callback: the function to invoke on completion
> > > > * @opaque: user data to pass to @callback
> > > > * @destroy: the function to free @opaque
> > > > * @context: the context to run the async task. If %NULL, the default
> > > > * context will be used.
> > > > + * @src_addr: the source address to be connected
> > > > *
> > > > - * Attempt to connect to the address @addr. This method
> > > > - * will run in the background so the caller will regain
> > > > + * Attempt to connect to the address @dst_addr with the @src_addr.
> > > > + * This method will run in the background so the caller will regain
> > > > * execution control immediately. The function @callback
> > > > - * will be invoked on completion or failure. The @addr
> > > > + * will be invoked on completion or failure. The @dst_addr
> > > > * parameter will be copied, so may be freed as soon
> > > > * as this function returns without waiting for completion.
> > > > */
> > > > void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
> > > > - SocketAddress *addr,
> > > > + SocketAddress *dst_addr,
> > > > QIOTaskFunc callback,
> > > > gpointer opaque,
> > > > GDestroyNotify destroy,
> > > > - GMainContext *context);
> > > > + GMainContext *context,
> > > > + SocketAddress *src_addr);
> > > Lets avoid modifying these two methods, and thus avoid
> > > updating countless callers.
> > >
> > > In common with typical pattern in QIO code, lets add
> > > a second variant
> > >
> > > qio_channel_socket_connect_full
> > > qio_channel_socket_connect_full_async
> > >
> > > which has the extra parameters, then make the existing
> > > simpler methods call the new ones.
> > >
> > > ie qio_channel_socket_connect should call
> > > qio_channel_socket_connect_full, pasing NULL for the
> > > src_addr.
> > >
> > > Thanks for the suggestion Daniel. Will modify the same structure as
> >
> > suggested above in the v2 patchset.
>
> > Hi Daniel. I agree with your suggestion here, but I have couple of doubts
> in implementing this type.
>
> 1. You meant to say qio_channel_socket_connect_async calls ->
> qio_channel_socket_connect_all_async and the later function would have a
> extra parameter for src_addr as NULL right. But if you see this approach
> works well for connecting non-multifd channels where source uri is passed as
> NULL, but for multifd channels, as you see the function
> socket_send_channel_create also calls qio_channel_socket_connect_async, but
> this time instead of NULL, it should actually pass a src_addr parameter. So
> in my opion, whatever function multifd function is calling it should have
> extra parameter to pass src_addr.
>
> 2. Same goes for qio_channel_socket_connect_sync func, for multifd path, it
> should be passed with src_addr instead of NULL.
>
> 3. I agree, modifying these methods would lead to updating endless callers
> from test cases. But I don't see a better way that this at the moment. And
> out of the two methods, one method is called only for single unit test case
> in qemu.
>
> We would love to have suggestions from your side Daniel.
Do not modify this existing method signature at all:
int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
Error **errp);
Only add a new method:
int qio_channel_socket_connect_full_sync(QIOChannelSocket *ioc,
SocketAddress *dst_addr,
SocketAddress *src_addr,
Error **errp);
Internally the former method calls the latter, assing NULL for
src_addr.
Externally, only the migration code needs to use the new method,
all the rest of QEMU code must remain unchanged calling the simpler
method.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
