in_pcbsetport(), in6_pcbsetport()

[Elad Efrat]

Hi,

Attached is a diff that implements the following changes:

- in6_pcbbind_{addr,port}() become static (I should have done this
before).

- in_pcbsetport(), in6_pcbsetport() properly authorize port binding.
This is just a first part, I'm about to add a new request to
indicate "auto-assign", but it shouldn't stand in the way of the
changes here.

- Pass sockaddr_in6 to in6_pcbsetport(), similarly to its netinet
counterpart. Craft one in udp6_output() where we don't have it in
our context.

- Don't pass "laddr" to the aforementioned functions, as it's
redundant now that both take a sockaddr_in{,6}.

Please review. No change in functionality is expected, but I'd like
eyes on this from people who are more familiar with the IPv6 stack.

Thanks,

-e.

[David Young]

Post by Elad Efrat
Hi,
- in6_pcbbind_{addr,port}() become static (I should have done this
before).
- in_pcbsetport(), in6_pcbsetport() properly authorize port binding.
This is just a first part, I'm about to add a new request to
indicate "auto-assign", but it shouldn't stand in the way of the
changes here.
- Pass sockaddr_in6 to in6_pcbsetport(), similarly to its netinet
counterpart. Craft one in udp6_output() where we don't have it in
our context.
- Don't pass "laddr" to the aforementioned functions, as it's
redundant now that both take a sockaddr_in{,6}.
Please review. No change in functionality is expected, but I'd like
eyes on this from people who are more familiar with the IPv6 stack.

I see at least one serious problem with this patch. I need to go back
and review your previous patches in this area, too. Please hold off
from committing this until I have had a closer look. Thanks.

Dave

[Manuel Bouyer]

Post by Elad Efrat
Hi,
- in6_pcbbind_{addr,port}() become static (I should have done this
before).
- in_pcbsetport(), in6_pcbsetport() properly authorize port binding.
This is just a first part, I'm about to add a new request to
indicate "auto-assign", but it shouldn't stand in the way of the
changes here.
- Pass sockaddr_in6 to in6_pcbsetport(), similarly to its netinet
counterpart. Craft one in udp6_output() where we don't have it in
our context.
- Don't pass "laddr" to the aforementioned functions, as it's
redundant now that both take a sockaddr_in{,6}.
Please review. No change in functionality is expected, but I'd like
eyes on this from people who are more familiar with the IPv6 stack.

It looks like in udp6_output.c you're going to change in6_any.sin6_addr,
I can't see how this can be right (if pr_domain is const there's probably
a reason :). You should copy the content to a local sockaddr_in6 instead.

BTW, you did a similar change to the netinet/ code, with a UNCONST here too.
While it may be safe here (it seems you're not writing to the pointer),
it would be better to not use UNCONST() here and make the pointer's
copy const instead.

[Elad Efrat]

Post by Manuel Bouyer
It looks like in udp6_output.c you're going to change in6_any.sin6_addr,
I can't see how this can be right (if pr_domain is const there's probably
a reason :). You should copy the content to a local sockaddr_in6 instead.
BTW, you did a similar change to the netinet/ code, with a UNCONST here too.
While it may be safe here (it seems you're not writing to the pointer),
it would be better to not use UNCONST() here and make the pointer's
copy const instead.

Good catch! :)

Would something like the following be okay?

struct sockaddr_in6 lsin6 =
*((struct sockaddr_in6
*)__UNCONST(in6p->in6p_socket->so_proto->pr_domain->dom_sa_any));
lsin6.sin6_addr = *laddr;

Thanks,

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[David Young]

Post by Elad Efrat

Post by Manuel Bouyer
It looks like in udp6_output.c you're going to change in6_any.sin6_addr,
I can't see how this can be right (if pr_domain is const there's probably
a reason :). You should copy the content to a local sockaddr_in6 instead.
BTW, you did a similar change to the netinet/ code, with a UNCONST here too.
While it may be safe here (it seems you're not writing to the pointer),
it would be better to not use UNCONST() here and make the pointer's
copy const instead.

Good catch! :)
Would something like the following be okay?
struct sockaddr_in6 lsin6 =
*((struct sockaddr_in6
*)__UNCONST(in6p->in6p_socket->so_proto->pr_domain->dom_sa_any));
lsin6.sin6_addr = *laddr;

There's no need for __UNCONST(). Let the appearance of __UNCONST() in
your code be your cue to revisit your assumptions or your design.

If this is not in a fast path, then this is fairly concise and correct:

struct sockaddr_in6 lsin6 =
*satocsin6(sockaddr_any_by_family(AF_INET6));
lsin6.sin6_addr = *laddr;

but I suggest:

struct sockaddr_in6 lsin6;
sockaddr_in6_init(&lsin6, laddr, 0, 0, 0);

Dave

[Manuel Bouyer]

Post by Elad Efrat

Post by Manuel Bouyer
It looks like in udp6_output.c you're going to change in6_any.sin6_addr,
I can't see how this can be right (if pr_domain is const there's probably
a reason :). You should copy the content to a local sockaddr_in6 instead.
BTW, you did a similar change to the netinet/ code, with a UNCONST here too.
While it may be safe here (it seems you're not writing to the pointer),
it would be better to not use UNCONST() here and make the pointer's
copy const instead.

Good catch! :)
Would something like the following be okay?
struct sockaddr_in6 lsin6 =
*((struct sockaddr_in6
*)__UNCONST(in6p->in6p_socket->so_proto->pr_domain->dom_sa_any));
lsin6.sin6_addr = *laddr;

Why do you still need the __UNCONST here ?

[Elad Efrat]

Post by Manuel Bouyer

Post by Elad Efrat
Would something like the following be okay?
    struct sockaddr_in6 lsin6 =
        *((struct sockaddr_in6
*)__UNCONST(in6p->in6p_socket->so_proto->pr_domain->dom_sa_any));
    lsin6.sin6_addr = *laddr;

Why do you still need the __UNCONST here ?

Oh yeah. Removed, I now use just

struct sockaddr_in6 lsin6 =
*((const struct sockaddr_in6
*)in6p->in6p_socket->so_proto->pr_domain->dom_sa_any);

Anything else?

Thanks,

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by Elad Efrat

Post by Manuel Bouyer

Post by Elad Efrat
Would something like the following be okay?
    struct sockaddr_in6 lsin6 =
        *((struct sockaddr_in6
*)__UNCONST(in6p->in6p_socket->so_proto->pr_domain->dom_sa_any));
    lsin6.sin6_addr = *laddr;

Why do you still need the __UNCONST here ?

Oh yeah. Removed, I now use just
struct sockaddr_in6 lsin6 =
*((const struct sockaddr_in6
*)in6p->in6p_socket->so_proto->pr_domain->dom_sa_any);
Anything else?

looks good now. Can look at removing the similar __UNCONST in the
netinet source ?

[Elad Efrat]

Post by Manuel Bouyer
looks good now. Can look at removing the similar __UNCONST in the
netinet source ?

Oh, there I have a local variable ("struct sockaddr_in lsin") that I
use to copy dom_sa_any into, and then use its address when setting
sin:

struct sockaddr_in lsin; /* entire function scope */
[...]
} else {
/* no nam */
lsin = *((const struct sockaddr_in *)
inp->inp_socket->so_proto->pr_domain->dom_sa_any);
sin = &lsin;
}

Thanks,

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by Elad Efrat

Post by Manuel Bouyer
looks good now. Can look at removing the similar __UNCONST in the
netinet source ?

Oh, there I have a local variable ("struct sockaddr_in lsin") that I
use to copy dom_sa_any into, and then use its address when setting
struct sockaddr_in lsin; /* entire function scope */
[...]
} else {
/* no nam */
lsin = *((const struct sockaddr_in *)
inp->inp_socket->so_proto->pr_domain->dom_sa_any);
sin = &lsin;
}
Thanks,

that's better. thanks !