Re: [PATCH] Remove Scope IDs from IPv6 addresses.

Re: [PATCH] Remove Scope IDs from IPv6 addresses.

[Eric Wong]

(Moving to the right list, this thread was originally posted to kgio :x)

Hleb Valoshka <375gnu@gmail.com> wrote:
> 	Scoped ipv6 addresses are defined in rfc4007.
> 	Ruby doesn't support them yet and it's unknown whether it will
> 	(see http://bugs.ruby-lang.org/issues/8464).
> 	So we just remove scope ids.

Thanks.  Comments inline.

> --- a/ext/raindrops/linux_inet_diag.c
> +++ b/ext/raindrops/linux_inet_diag.c
> @@ -134,17 +134,54 @@ static int st_free_data(st_data_t key, st_data_t value, st_data_t ignored)

> +static char* remove_scope_id(const char * addr)
> +{
> +        char *newaddr, *t;
> +
> +        newaddr = strdup(addr);
> +        if (newaddr == NULL)
> +                perror("strdup");

We'll segfault on strchr below if we perrored above.  If we continue
using strdup, I would just use ruby_strdup() (works on both MRI/rbx).

But I'd rather avoid *strdup entirely.  This is unlikely to
remove, right?

> +        t = strchr(newaddr, '%');
> +
> +        if (t != NULL) {
> +                *t = '\0';
> +                t = strchr(t+1, ']');
> +                if (t == NULL)
> +                        fprintf(stderr, "Bad IPv6 address: %s!\n", addr);

Better to use rb_warn/rb_warning.
These are preferable since they allow assigning $stderr to StringIO and
such.  But if we don't use strdup, maybe we could raise instead.

> +                else
> +                        strcat(newaddr, t);
> +        }
> +
> +        return newaddr;
> +}

Perhaps something like this (totally untested, likely off-by-one errors)

static VALUE remove_scope_id(const char *addr)
{
	VALUE rv = rb_str_new2(addr);
	long len = RSTRING_LEN(rv);
	char *ptr = RSTRING_PTR(rv);
	char *pct = memchr(ptr, '%', len);

	/*
	 * remove scoped portion
	 * Ruby equivalent: rv.sub!(/%([^\]]*)\]/, "]")
	 */
	if (pct) {
		long newlen = pct - ptr;
		char *rbracket = memchr(pct, ']', len - newlen);

		if (rbracket) {
			size_t move = len - (rbracket - ptr);

			memmove(pct, rbracket, move);
			newlen += move;

			rb_str_set_len(rv, newlen);
		} else {
			/* perhaps raise here? */
			rb_warn("Bad IPv6 address: %s", addr);
		}
	}
	return rv;
}
-- 
Eric Wong

Re: Re: [PATCH] Remove Scope IDs from IPv6 addresses.

[Hleb Valoshka]

On 9/10/13, Eric Wong <normalperson@yhbt.net> wrote:
>> +        newaddr = strdup(addr);
>> +        if (newaddr == NULL)
>> +                perror("strdup");
>
> We'll segfault on strchr below if we perrored above.  If we continue
> using strdup, I would just use ruby_strdup() (works on both MRI/rbx).

I haven't use C for a long time, so I thought that strchr checks its args.

> But I'd rather avoid *strdup entirely.  This is unlikely to
> remove, right?

Frankly speaking I dislike functions similar to strdup(), because
using them you should remember to call free(), sometimes in many
places (or to use goto). But I'm unfamiliar with internal ruby api, so
it was the only variant for me.

BTW, is anything bad with that variant:
char *newaddr = alloca(...);
remove_scope_id(oldaddr, newaddr);

> Perhaps something like this (totally untested, likely off-by-one errors)

Ok, I'll test your code tomorrow.

Re: Re: [PATCH] Remove Scope IDs from IPv6 addresses.

[Eric Wong]

Hleb Valoshka <375gnu@gmail.com> wrote:
> On 9/10/13, Eric Wong <normalperson@yhbt.net> wrote:
> >> +        newaddr = strdup(addr);
> >> +        if (newaddr == NULL)
> >> +                perror("strdup");
> >
> > We'll segfault on strchr below if we perrored above.  If we continue
> > using strdup, I would just use ruby_strdup() (works on both MRI/rbx).
> 
> I haven't use C for a long time, so I thought that strchr checks its args.
> 
> > But I'd rather avoid *strdup entirely.  This is unlikely to
> > remove, right?
> 
> Frankly speaking I dislike functions similar to strdup(), because
> using them you should remember to call free(), sometimes in many
> places (or to use goto). But I'm unfamiliar with internal ruby api, so
> it was the only variant for me.

I try to avoid strdup, too.  I can help you with Ruby C API :)
The advantage of creating Ruby strings right away is it maintains
length, so one can use mem* functions instead of str* functions
(always faster and more explicit, often safer, and never less safe).

> BTW, is anything bad with that variant:
> char *newaddr = alloca(...);
> remove_scope_id(oldaddr, newaddr);

Probably OK in this case, but it would need a comment explaining to stack
checkers the size is bounded and won't overflow.

> > Perhaps something like this (totally untested, likely off-by-one errors)
> 
> Ok, I'll test your code tomorrow.

Thanks.

Re: Re: [PATCH] Remove Scope IDs from IPv6 addresses.

[Hleb Valoshka]

On 9/10/13, Eric Wong <normalperson@yhbt.net> wrote:
>> > Perhaps something like this (totally untested, likely off-by-one
>> > errors)
>> Ok, I'll test your code tomorrow.
> Thanks.

Okay, it works, but I was too busy today to test it enough. I hope tomorrow.