On 10/29/20 8:38 AM, Markus Armbruster wrote:
> The abstract socket namespace is a non-portable Linux extension. An
> attempt to use it elsewhere should fail with ENOENT (the abstract
> address looks like a "" pathname, which does not resolve). We report
> this failure like
>
> Failed to connect socket abc: No such file or directory
>
> Tolerable, although ENOTSUP would be better.
>
> However, introspection lies: it has @abstract regardless of host
> support. Easy enough to fix: since Linux provides them since 2.2,
> 'if': 'defined(CONFIG_LINUX)' should do.
>
> The above failure becomes
>
> Parameter 'backend.data.addr.data.abstract' is unexpected
>
> I consider this an improvement.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> +++ b/qapi/sockets.json
> @@ -74,18 +74,20 @@
> # Captures a socket address in the local ("Unix socket") namespace.
> #
> # @path: filesystem path to use
> -# @tight: pass a socket address length confined to the minimum length of the
> -# abstract string, rather than the full sockaddr_un record length
> -# (only matters for abstract sockets, default true). (Since 5.1)
> -# @abstract: whether this is an abstract address, default false. (Since 5.1)
> +# @abstract: if true, this is a Linux abstract socket address. @path
> +# will be prefixed by a null byte, and optionally padded
> +# with null bytes. Defaults to false. (Since 5.1)
> +# @tight: if false, pad an abstract socket address with enough null
> +# bytes to make it fill struct sockaddr_un member sun_path.
> +# Defaults to true. (Since 5.1)
Do we need to mention that @tight is ignored (or even make it an error)
if @abstract is false?
> #
> # Since: 1.3
> ##
> { 'struct': 'UnixSocketAddress',
> 'data': {
> 'path': 'str',
> - '*tight': 'bool',
> - '*abstract': 'bool' } }
> + '*tight': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' },
> + '*abstract': { 'type': 'bool', 'if': 'defined(CONFIG_LINUX)' } } }
So we document @abstract before @tight, but declare them in reverse
order. I guess our doc generator doesn't care?
>
> ##
> # @VsockSocketAddress:
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index dc1cf86ecf..1d2b2efb13 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s,
> const char *prefix)
> break;
> case SOCKET_ADDRESS_TYPE_UNIX:
> {
> +#ifdef CONFIG_LINUX
> UnixSocketAddress *sa = &s->addr->u.q_unix;
> +#endif
>
> return g_strdup_printf("%sunix:%s%s%s%s", prefix,
> s->addr->u.q_unix.path,
Why did we need the #ifdef above, which means we can't we use sa here?
> +#ifdef CONFIG_LINUX
> sa->has_abstract && sa->abstract
I hate mid-()-expression #ifdefs. If g_strdup_printf() were itself a
macro expansion, things break. Can you come up with a saner way of
writing this?
> ? ",abstract" : "",
> sa->has_tight && sa->tight
> ? ",tight" : "",
> +#else
> + "", "",
> +#endif
> s->is_listen ? ",server" : "");
I suggest:
const char *tight = "", *abstract = "";
UnixSocketAddress *sa = &s->addr->u.q_unix;
#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" : "");
> +++ b/util/qemu-sockets.c
> @@ -854,10 +854,29 @@ static int vsock_parse(VsockSocketAddress *addr, const
> char *str,
>
> #ifndef _WIN32
>
> +static bool saddr_is_abstract(UnixSocketAddress *saddr)
> +{
> +#ifdef CONFIG_LINUX
> + return saddr->abstract;
> +#else
> + return false;
> +#endif
> +}
> +
> +static bool saddr_is_tight(UnixSocketAddress *saddr)
> +{
> +#ifdef CONFIG_LINUX
> + return !saddr->has_tight || saddr->tight;
Should this also look at abstract?
> +#else
> + return false;
> +#endif
> +}
> +
Is it any easier to split the patch, first into the introduction of
saddr_is_* and adjusting all clients, and second into adding the 'if' to
the QAPI declaration?
But the idea makes sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
