Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Feb 02, 2023 at 01:51:28PM +0100, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
>> >> "manish.mishra" <manish.mishra@nutanix.com> wrote:
>> >> > MSG_PEEK peeks at the channel, The data is treated as unread and
>> >> > the next read shall still return this data. This support is
>> >> > currently added only for socket class. Extra parameter 'flags'
>> >> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
>> >> >
>> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> >> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
>> >> 
>> >> 
>> >> This change breaks RDMA migration.
>> >> 
>> >> FAILED: libcommon.fa.p/migration_rdma.c.o
>> >> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 
>> >> -I/usr/include/libpng16 -I/usr/include/spice-server 
>> >> -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 
>> >> -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 
>> >> -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC 
>> >> -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 
>> >> -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 
>> >> -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror 
>> >> -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem 
>> >> linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote 
>> >> /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread 
>> >> -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE 
>> >> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing 
>> >> -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes 
>> >> -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration 
>> >> -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k 
>> >> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs 
>> >> -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 
>> >> -Wmissing-format-attribute -Wno-missing-include-dirs 
>> >> -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE 
>> >> -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ 
>> >> libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d 
>> >> -o libcommon.fa.p/migration_rdma.c.o -c 
>> >> ../../../../mnt/code/qemu/full/migration/rdma.c
>> >> ../../../../mnt/code/qemu/full/migration/rdma.c: In function 
>> >> ‘qio_channel_rdma_class_init’:
>> >> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: 
>> >> assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  
>> >> int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const 
>> >> struct iovec *, long unsigned int,  int **, long unsigned int *, int,  
>> >> Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, 
>> >> const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long 
>> >> int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, 
>> >> long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
>> >>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
>> >>       |                         ^
>> >> cc1: all warnings being treated as errors
>> >> 
>> >> And I don't really know how to fix it, because the problem is that rdma
>> >> don't use qio_channel_readv_full() at all.
>> >
>> > Likely qio_channel_rdma_readv just adds the 'int flags' param added.
>> > It doesn't need to actually do anything with the flags as they are
>> > checked before
>> 
>> I can do that.  That would fix the compilation issue.
>> 
>> But will rdma work?  Because it fakes a qio channel, so what is going to
>> implement the MSG_PEEK functionality for it?  It don't end calling
>> recv() at all.
>
> It is no problem - the qio_channel_readv method changes in this patch
> add:
>
> +    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
> +        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> +        error_setg_errno(errp, EINVAL,
> +                         "Channel does not support peek read");
> +        return -1;
> +    }
>
>
> so it is impossible for qio_channel_rdma_readv to be invoked with
> flags having MSG_PEEK set, thus RDMA can ignore the whole concept.

And as we require MSG_PEEK to do migration, we have lost RDMA migration
in the process.

The following patch on the series use this functionality to read the
beggining of the streams in the channels.

Later, Juan.