Re: [PATCH v2 09/11] char-socket: Fix qemu_chr_socket_address() for abstract sockets

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Nov 03, 2020 at 07:28:20AM +0100, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 11/2/20 3:44 AM, Markus Armbruster wrote:
>> >> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
>> >> support" neglected to update qemu_chr_socket_address().  It shows
>> >> shows neither @abstract nor @tight.  Fix that.
>> >> 
>> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  chardev/char-socket.c | 10 +++++++++-
>> >>  1 file changed, 9 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> >> index 1ee5a8c295..dc1cf86ecf 100644
>> >> --- a/chardev/char-socket.c
>> >> +++ b/chardev/char-socket.c
>> >> @@ -443,10 +443,18 @@ static char *qemu_chr_socket_address(SocketChardev 
>> >> *s, const char *prefix)
>> >>                                 s->is_listen ? ",server" : "");
>> >>          break;
>> >>      case SOCKET_ADDRESS_TYPE_UNIX:
>> >> -        return g_strdup_printf("%sunix:%s%s", prefix,
>> >> +    {
>> >> +        UnixSocketAddress *sa = &s->addr->u.q_unix;
>> >> +
>> >> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
>> >>                                 s->addr->u.q_unix.path,
>> >> +                               sa->has_abstract && sa->abstract
>> >> +                               ? ",abstract" : "",
>> >> +                               sa->has_tight && sa->tight
>> >> +                               ? ",tight" : "",
>> >>                                 s->is_listen ? ",server" : "");
>> >
>> > Gets modified again in 11/11, so I can accept this as a strict
>> > improvement, even if it is not the final form.
>> 
>> You're right, PATCH 11's change is better done here already.  Will tidy
>> up if I need to respin for some other reason.
>
> I can squash in the following part of patch 11:
>
> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, 
> const char *prefix)
>          break;
>      case SOCKET_ADDRESS_TYPE_UNIX:
>      {
> +        const char *tight = "", *abstract = "";
>          UnixSocketAddress *sa = &s->addr->u.q_unix;
>
> -        return g_strdup_printf("%sunix:%s%s%s%s", prefix,
> -                               s->addr->u.q_unix.path,
> -                               sa->has_abstract && sa->abstract
> -                               ? ",abstract" : "",
> -                               sa->has_tight && sa->tight
> -                               ? ",tight" : "",
> +#ifdef CONFIG_LINUX
> +        if (sa->has_abstract && sa->abstract) {
> +            abstract = ",abstract";
> +            if (sa->has_tight && sa->tight) {
> +                tight = ",tight";
> +            }
> +        }
> +#endif
> +
> +        return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path,
> +                               abstract, tight,
>                                 s->is_listen ? ",server" : "");
>          break;
>      }
>
> but leaving out the CONFIG_LINUX ifdef until Patch 11

Appreciated!