SUN_LEN

SUN_LEN

[小林悠]

Hello.

Could you please add SUN_LEN() to sys/un.h?

diff --git a/include/sys/un.h b/include/sys/un.h
index 769dac6..5289227 100644
--- a/include/sys/un.h
+++ b/include/sys/un.h
@@ -10,4 +10,9 @@ struct sockaddr_un
        char sun_path[108];
 };

+#ifndef SUN_LEN
+#include <string.h>
+#define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
+ strlen((ptr)->sun_path))
+#endif
+
 #endif

-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[John Spencer]

On 08/21/2013 11:57 AM, 小林悠 wrote:
> Hello.
>
> Could you please add SUN_LEN() to sys/un.h?

which program needs it ?

>
> diff --git a/include/sys/un.h b/include/sys/un.h
> index 769dac6..5289227 100644
> --- a/include/sys/un.h
> +++ b/include/sys/un.h
> @@ -10,4 +10,9 @@ struct sockaddr_un
>          char sun_path[108];
>   };
>
> +#ifndef SUN_LEN
> +#include<string.h>

this looks like a namespace violation to me

> +#define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
> + strlen((ptr)->sun_path))

are these 2 lines meant to be connected with a \ ?
as-is, this won't work...

in glibc, it is defined as
# define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path) 
       \
                       + strlen ((ptr)->sun_path))


> +#endif
> +
>   #endif
>

Re: SUN_LEN

[小林悠]

Hello.

> which program needs it ?

It is used in here.
https://android.googlesource.com/platform/system/core/+/master/libcutils/properties.c

> are these 2 lines meant to be connected with a \ ?

Yes. Sorry, I'm using Gmail and the line was too long.
Gmail splits a long line automatically.


2013/8/21 John Spencer <maillist-musl@barfooze.de>:
> On 08/21/2013 11:57 AM, 小林悠 wrote:
>>
>> Hello.
>>
>> Could you please add SUN_LEN() to sys/un.h?
>
>
> which program needs it ?
>
>
>>
>> diff --git a/include/sys/un.h b/include/sys/un.h
>> index 769dac6..5289227 100644
>> --- a/include/sys/un.h
>> +++ b/include/sys/un.h
>> @@ -10,4 +10,9 @@ struct sockaddr_un
>>          char sun_path[108];
>>   };
>>
>> +#ifndef SUN_LEN
>> +#include<string.h>
>
>
> this looks like a namespace violation to me
>
>
>> +#define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
>> + strlen((ptr)->sun_path))
>
>
> are these 2 lines meant to be connected with a \ ?
> as-is, this won't work...
>
> in glibc, it is defined as
> # define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
> \
>
>                       + strlen ((ptr)->sun_path))
>
>
>> +#endif
>> +
>>   #endif
>>
>



-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[John Spencer]

On 08/21/2013 02:23 PM, 小林悠 wrote:
> Hello.
> 
>> which program needs it ?
> 
> It is used in here.
> https://android.googlesource.com/platform/system/core/+/master/libcutils/properties.c

well, maybe we should get that fixed then ?
they're using it unconditionally but should instead define it themselves
if it isn't already.

Re: SUN_LEN

[小林悠]

>> It is used in here.
>> https://android.googlesource.com/platform/system/core/+/master/libcutils/properties.c
>
> well, maybe we should get that fixed then ?
> they're using it unconditionally but should instead define it themselves
> if it isn't already.

I'm not Google person and I cannot change Android git repository,
and also If you don't change sys/un.h,
I think it is safer to use my personally modified sys/un.h.

So is this mean, you don't want to add glibc non-standard APIs to musl?

-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[Szabolcs Nagy]

* ?$B>.NSM* <yukoba@gmail.com> [2013-08-21 22:27:22 +0900]:
> 
> So is this mean, you don't want to add glibc non-standard APIs to musl?

i think SUN_LEN can be added, but it should be under
_BSD_SOURCE because it violates the posix namespace

#if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
#define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
#endif

Re: SUN_LEN

[小林悠]

> i think SUN_LEN can be added, but it should be under
> _BSD_SOURCE because it violates the posix namespace
>
> #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> #define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
> #endif

OK. I tested and I think this is correct. So it is 110 - 108 + strlen.

-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[Rich Felker]

On Wed, Aug 21, 2013 at 04:45:52PM +0200, Szabolcs Nagy wrote:
> * ?$B>.NSM* <yukoba@gmail.com> [2013-08-21 22:27:22 +0900]:
  ^^^^^^^^^^^

The fact that gmail seems to be using ISO-2022-JP by default for
encoding Japanese headers suggests that maybe we need to add stateful
encodings to iconv... :(

> > 
> > So is this mean, you don't want to add glibc non-standard APIs to musl?

This is a difficult question to answer directly. The factors that
contribute to whether we want to add non-standard APIs are things
like:

- Does it meet a need that's otherwise unmet?

- Is it ugly namespace pollution? (While there's no requirement to
  protect the namespace in _BSD_SOURCE or _GNU_SOURCE profiles, we try
  to avoid extreme ugliness.)

- Does it have major historical precedent?

- Does it have conflicting historical definitions on different
  systems?

- Would it improve compatibility with binary apps/libraries or just
  build-time compatibility?

- Is the number of apps affected so small that we could just recommend
  patches for them?

For SUN_LEN, I think based on the above factors, it's okay for
inclusion (with the proper namespace protection of course).

> i think SUN_LEN can be added, but it should be under
> _BSD_SOURCE because it violates the posix namespace
> 
> #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> #define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
> #endif

This is insufficient, since sys/un.h does not expose strlen. We could
either add a conditional declaration of strlen under this #if, or make
an inline function for SUN_LEN that just does the strlen-like loop
manually. I'm not sure what's best.

Rich

Re: SUN_LEN

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-08-21 12:23:39 -0400]:
> > #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> > #define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
> > #endif
> 
> This is insufficient, since sys/un.h does not expose strlen. We could
> either add a conditional declaration of strlen under this #if, or make
> an inline function for SUN_LEN that just does the strlen-like loop
> manually. I'm not sure what's best.

declare strlen

but i would not worry about this much, if you use such a
macro you are already outside the well-defined zone..
and musl does not need to be nice there

Re: SUN_LEN

[Rich Felker]

On Wed, Aug 21, 2013 at 06:34:29PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@aerifal.cx> [2013-08-21 12:23:39 -0400]:
> > > #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> > > #define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
> > > #endif
> > 
> > This is insufficient, since sys/un.h does not expose strlen. We could
> > either add a conditional declaration of strlen under this #if, or make
> > an inline function for SUN_LEN that just does the strlen-like loop
> > manually. I'm not sure what's best.
> 
> declare strlen
> 
> but i would not worry about this much, if you use such a
> macro you are already outside the well-defined zone..
> and musl does not need to be nice there

Declaring strlen is not easy because size_t is not defined. So I guess
we also need an additional #if before including bits/alltypes.h to
define __NEED_size_t...

This is why I thought a function might be less ugly.

Rich

Re: SUN_LEN

[Rich Felker]

On Wed, Aug 21, 2013 at 12:36:45PM -0400, Rich Felker wrote:
> On Wed, Aug 21, 2013 at 06:34:29PM +0200, Szabolcs Nagy wrote:
> > * Rich Felker <dalias@aerifal.cx> [2013-08-21 12:23:39 -0400]:
> > > > #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> > > > #define SUN_LEN(s) (sizeof *(s) - sizeof (s)->sun_path + strlen((s)->sun_path))
> > > > #endif
> > > 
> > > This is insufficient, since sys/un.h does not expose strlen. We could
> > > either add a conditional declaration of strlen under this #if, or make
> > > an inline function for SUN_LEN that just does the strlen-like loop
> > > manually. I'm not sure what's best.
> > 
> > declare strlen
> > 
> > but i would not worry about this much, if you use such a
> > macro you are already outside the well-defined zone..
> > and musl does not need to be nice there
> 
> Declaring strlen is not easy because size_t is not defined. So I guess
> we also need an additional #if before including bits/alltypes.h to
> define __NEED_size_t...
> 
> This is why I thought a function might be less ugly.

Here's a sketch of what it could be:

static __inline unsigned long __SUN_LEN(struct sockaddr_un *__s)
{
	unsigned long __i;
	for (__i=0; __s->sun_path[__i]; __i++);
	return __i+2;
}
#define SUN_LEN(s) __SUN_LEN(s)

Rich

Re: SUN_LEN

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-08-21 12:42:20 -0400]:
> > Declaring strlen is not easy because size_t is not defined. So I guess
> > we also need an additional #if before including bits/alltypes.h to
> > define __NEED_size_t...
> > 
> > This is why I thought a function might be less ugly.
> 
> Here's a sketch of what it could be:
> 
> static __inline unsigned long __SUN_LEN(struct sockaddr_un *__s)
> {
> 	unsigned long __i;
> 	for (__i=0; __s->sun_path[__i]; __i++);
> 	return __i+2;
> }
> #define SUN_LEN(s) __SUN_LEN(s)

you could include stddef.h, but this works as well

Re: SUN_LEN

[Yu Kobayashi]

> The fact that gmail seems to be using ISO-2022-JP by default

Sorry, I switched to the English name.
I think UTF-8 or US-ASCII is used.

> This is insufficient, since sys/un.h does not expose strlen.

Why is it a bad thing to include <string.h>?
Worry about cyclic dependency?

-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[Rich Felker]

On Thu, Aug 22, 2013 at 02:11:52AM +0900, Yu Kobayashi wrote:
> > The fact that gmail seems to be using ISO-2022-JP by default
> 
> Sorry, I switched to the English name.
> I think UTF-8 or US-ASCII is used.
> 
> > This is insufficient, since sys/un.h does not expose strlen.
> 
> Why is it a bad thing to include <string.h>?
> Worry about cyclic dependency?

Well it puts a lot more stuff into the namespace and has a much larger
parsing cost (for the 99% of applications using sys/un.h that don't
need SUN_LEN). I think just using strlen and adding the __NEED_size_t
and prototype might be the least ugly though, now... after all none of
the options are clean.

Rich

Re: SUN_LEN

[Yu Kobayashi]

>> Why is it a bad thing to include <string.h>?
>> Worry about cyclic dependency?
> Well it puts a lot more stuff into the namespace and has a much larger
> parsing cost

I see. You are worrying this.

-- 

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[Rich Felker]

On Thu, Aug 22, 2013 at 02:11:52AM +0900, Yu Kobayashi wrote:
> > The fact that gmail seems to be using ISO-2022-JP by default
> 
> Sorry, I switched to the English name.
> I think UTF-8 or US-ASCII is used.

I wasn't complaining, just noting that this is an enhancement musl
probably needs. BTW, gmail has an option to tell it to always send
UTF-8 instead of choosing a legacy encoding that works for the
content.

Rich

Re: SUN_LEN

[Yu Kobayashi]

>> Why is it a bad thing to include <string.h>?
>> Worry about cyclic dependency?
> Well it puts a lot more stuff into the namespace and has a much larger
> parsing cost

How about this.

#if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
#define SUN_LEN(s) __sun_len(s)
#endif

And add __sun_len.c like this.

#include <string.h>
#include <sys/un.h>

size_t __sun_len(struct sockaddr_un *s)
{
    return sizeof(sa_family_t) + strlen(s->sun_path);
}

I think this solve the problem.

--

Yu Kobayashi <yukoba@gmail.com>

Re: SUN_LEN

[Rich Felker]

On Thu, Aug 22, 2013 at 02:58:28AM +0900, Yu Kobayashi wrote:
> >> Why is it a bad thing to include <string.h>?
> >> Worry about cyclic dependency?
> > Well it puts a lot more stuff into the namespace and has a much larger
> > parsing cost
> 
> How about this.
> 
> #if defined(_BSD_SOURCE) || defined(_GNU_SOURCE)
> #define SUN_LEN(s) __sun_len(s)
> #endif
> 
> And add __sun_len.c like this.
> 
> #include <string.h>
> #include <sys/un.h>
> 
> size_t __sun_len(struct sockaddr_un *s)
> {
>     return sizeof(sa_family_t) + strlen(s->sun_path);
> }
> 
> I think this solve the problem.

No, this is worse. It enlarges libc.so for an interface that's
essentially never-used, and more importantly, adds a new symbol to the
ABI that we have to keep forever.

Rich