Thanks for taking a look.  I'll update the RB, but there are a few questions for you...

Have I taken care of all the issues you raised?

Would like to "move this along"...

Hi Gordon, I have a few outstanding questions on some of the test bits, but otherwise the main parts here look good. Thanks.

I should have updated the prior comment too.

Now that we're using the internal address here

and not translating, that comment is stale.
The error check was for the possibility the

the translation from external to internal

form might fail.  No longer relevant.
Can I just delete the comment above?

2854-2858, or are you looking for more?

Was mostly just looking to understand what had been going on.

Sure.

It's the norm in this code to use these AF_UNIX socket addresses

even while they may be "changing under foot". i.e. see comments

(i.e. at socktpi.c line 3711) about the source address changing.
If the destination address changes under foot, the worst that

happens is we attempt sending the datagram to some grabled

destination that doesn't exist, so it ends up getting dropped.

That would only happen with a concurrent connect and sendmsg

on the same socket, so I'm not worried about it.

OK, I see. I guess this is one of those things that's up to the user, unfortunately. Regardless of what'd make the most sense, it doesn't make sense to tie into this for now.

I was just trying to clarify that AF_UNIX does not go down that path.

Should I delete the comment? or say "Not for AF_UNIX"?

The change is good, thanks.

Yeah, I'll take a stab at better comments on that.

How about in the comment above sotpi_sendmsg?

Thanks, what you have now is useful.

I thought of adding flags validation here (i.e. return EINVAL on bogus flags) but the code didn't do that before, so it seemed "off topic" to change that as part of this work.

If we're contining to ignore invalid flags, we need to clear this "internal-only" one.

Hmm. Fair enough, I think that makes sense, unfortunately. With our luck, stricter checking would now break a lot of stuff.

Yeah, I'll put a longer explanation in the block above this funciton,

and perhaps a reference to it a these lines.

Huh.  I ran cstyle...

Yeah... all of these are "internal use only" (as opposed to being specifically for ksockets).

You OK with the comment changing to something like "Only used internally in the kernel"?

Yeah, that's fine. Thanks.

I see what you mean, but here the passed in name==NULL, and the functions sosend_dgram(), sosend_dgramcmsg() assert name!=NULL and I didn't really want to change that.  That's why I chose to largely "borrow" the logic around the sti->>sti_faddr_noxlate flag.

Sorry for not stating clearly, I meant changing that block like the following:
        if (so->so_family == AF_UNIX) {
            flags |= MSG_SENDTO_NOXLATE;
        }
        ASSERT(sti->sti_faddr_sa);
        name = sti->sti_faddr_sa;
        namelen = (t_uscalar_t)sti->sti_faddr_len;

Oh, I see.  In the AF_UNIX case, sti_faddr_sa needs to be the "external" name, so that getpeername works correctly.

I don't think we can easily put the internal form there, which is why this uses sti_ux_faddr instead.

I am not seeing why it won't work:
@@ -3702,6 +3704,15 @@ sosend_dgramcmsg(struct sonode so, struct sockaddr name, socklen_t namelen,

                        addrlen = namelen;

                        src = NULL;

                        srclen = 0;

+               } else if (flags & MSG_SENDTO_NOXLATE) {

+                       /

+                        * Have an internal form dest. address.

+                        * Pass the source address as usual.

+                        /

+                       addr = &sti->sti_ux_faddr;

+                       addrlen = sizeof (sti->sti_ux_faddr);

+                       src = sti->sti_laddr_sa;

+                       srclen = (socklen_t)sti->sti_laddr_len;

                } else {

                        /

                         * Pass the sockaddr_un source address as an option

@@ -4022,6 +4048,15 @@ sosend_dgram(struct sonode so, struct sockaddr  name, socklen_t namelen,

                        addrlen = namelen;

                        src = NULL;

                        srclen = 0;

+               } else if (flags & MSG_SENDTO_NOXLATE) {

+                       /

+                        * Have an internal form dest. address.

+                        * Pass the source address as usual.

+                        /

+                       addr = &sti->sti_ux_faddr;

+                       addrlen = sizeof (sti->sti_ux_faddr);

+                       src = sti->sti_laddr_sa;

+                       srclen = (socklen_t)sti->sti_laddr_len;

                } else {

                        /

                         * Pass the sockaddr_un source address as an option

Thanks.

I'm finding it difficult to understand your proposed modification.  Could you perhaps send me a complete patch?

Also, my initial impression is that the change looks equivalent.  What makes it better?

Thanks.

Hi Gordon,
Sorry for the confusion. I've sent you the complete patch. You're right, technically what I proposed is equivalent to yours, but it will make the code less confusing.
Thanks,
--Youzhong

With your patch I now see what you're getting at.

Please see the comment I added at socktpi.c line 3713

That was an oversight.  Will fix.  Thanks.

Well, I copied the file from ../poll/Makefile

(if I recall correctly:)

When we copy things, we're supposed to keep the top matter...

Is there something else you'd like to see there?

OK, well, if you did just copy and not write it, then I guess so.

That's just (in general) what a listener should do.

Yes, in this test there will be only one connect.

Any reason to do this differently?

Generally, I would expect a listener to call listen once, but to call accept once per connection in a loop. However, digging into the source a bit more, it looks like we'll let you call listen() multiple times in a row to potentially change the backlog value. It's weird and not something you find in most network code, but not something worth arguing over and it looks like it's not going to break anything.

I would just say that if it's important for the test to call listen() multiple times for some of the privilege checking, you probably want to make sure that's commented, because that's the kind of thing most folks would be confused by.

D'oh!  I looked at that multiple times and my brain saw an "accept" loop, not a "listen" loop.

Sorry. Yeah, I'll fix that.

Huh.  I thought I c-styled all of this.

I'll check again.  Thanks.

Reading this does make a sense, but my experience about this area is quite low so the "weight" of this "shipit!" is probably not that high;)

Well... as this is someone else's work, I was trying to

"keep a light touch" on it.  Does this bother you a lot?

I decided to go ahead and cleanup that comment.

A lot of it no longer really applied to our use here.

Ditto.  It's really documentation about this particular

use of the send vs sendto interfaces.
Can you suggest a way to preserve the "documenting"

aspect of this while fixing whatever is offensive here?
If I change that to a plain old comment, would you

find that satisfactory?