Re: [Qemu-devel] [PATCH 3/3] char-socket: delay setting fd-pass feature until connected

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 3:11 PM, Daniel P. Berrangé <address@hidden> wrote:
> On Tue, Jul 17, 2018 at 03:07:01PM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Jul 17, 2018 at 3:01 PM, Daniel P. Berrangé <address@hidden> wrote:
>> > On Tue, Jul 17, 2018 at 02:52:39PM +0200, Marc-André Lureau wrote:
>> >> Wait for QIO channel connection completion, and check the feature set
>> >> by QIO. This fixes setting fd-pass chardev feature on
>> >> SOCKET_ADDRESS_FD where fd has AF_UNIX.
>> >>
>> >> Signed-off-by: Marc-André Lureau <address@hidden>
>> >> ---
>> >>  chardev/char-socket.c | 8 ++++----
>> >>  1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> >> index 6daa8d003a..7387e632d4 100644
>> >> --- a/chardev/char-socket.c
>> >> +++ b/chardev/char-socket.c
>> >> @@ -789,6 +789,9 @@ static int tcp_chr_new_client(Chardev *chr, 
>> >> QIOChannelSocket *sioc)
>> >>
>> >>      qio_channel_set_blocking(s->ioc, false, NULL);
>> >>
>> >> +    if (qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
>> >> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>> >> +    }
>> >
>> > I really don't like this approach as it has unpleasant async/race prone
>> > semantics or the external users of the chardev.
>> >
>> > With the current approach they know that once the chardev is created,
>> > the features have been initialized.
>> >
>> > With this approach, the features are only initialized once the client
>> > connection has been completed, or once the server has started listening,
>> > which may or may not be the case once the chardev constructor completes.
>>
>> Ok, what about augmenting the feature set then?:
>>
>> keep the current qemu_chr_set_feature() in contructor for
>> ADDRESS_TYPE_UNIX, and add the feature in new_client() for
>> ADDRESS_TYPE_FD?
>
> IMHO that makes the behaviour even more non-deterministic for callers.
>
> How about we just delete the entire feature concept from chardevs and remove
> the check from the vhostuser network backend too.
>
> We have lots of users of chardevs in QEMU and vhostuser is the only one
> that has tried to do this sanity check for the "correct" chardev backend.
> The other users just assume the user / mgmt app will configure a suitable
> chardev backend.  The vhostuser sanity checks have just caused more problems
> then they've solved.

It sounds less optimal than what I proposed, but I don't mind.

What do you think of the first 2 patches?


-- 
Marc-André Lureau