fpagliughi / sockpp

Modern C++ socket library.
BSD 3-Clause "New" or "Revised" License
782 stars 126 forks source link

Fixed family for sockaddr for mac #13

Closed desertkun closed 5 years ago

desertkun commented 5 years ago

Signature of mac's kernel sockaddr is different:

struct sockaddr {
    __uint8_t   sa_len;     /* total length */
    sa_family_t sa_family;  /* [XSI] address family */
    char        sa_data[14];    /* [XSI] addr value (actually larger) */
};

instead of

struct sockaddr{
    sa_family_t   sa_family       address family
    char          sa_data[]       socket address (variable-length data)
};

like the code assumes.

fpagliughi commented 5 years ago

Awesome! I was hoping to find someone to test this on Mac. Thanks for the PR.

The original implementation of the addresses was such that we couldn't always be sure that there was anything in the struct. I think with the latest implementation, we can assume that the base sockaddr is a minimum. As such, I think you're implementation of family() for Mac is better, shorter, safer... and portable! I think it's all we need and should work for both Mac and Linux:

virtual sa_family_t family() const {
    return sockaddr_ptr()->sa_family;
}

Right?

As for the connector, nice catch. I remembered to fix the acceptor, but forgot the connector. That one should simply call family() - however it's implemented:

sa_family_t domain = addr.family();

If I try these on Linux, can you confirm on Mac?

fpagliughi commented 5 years ago

I merged your PR into a branch 'sockaddr_mac' and made the mods to simplify the compatibility. The connector needed a couple other changes; I had completely forgotten to update it with the last set of changes to the address.

Have a look at let me know what you think.

fpagliughi commented 5 years ago

After looking it up, it seems the way I had implemented it was disturbingly incorrect! I will push the PR with the mods into 'develop' and proceed. I'm trying to wrap up a new release this weekend since v0.5 is pretty broken on everything except Linux.

fpagliughi commented 5 years ago

Oops. Forgot to check for NULL (if that's actually possible). Either way, it shouldn't hurt. Settled on:

    virtual sa_family_t family() const {
        auto p = sockaddr_ptr();
        return p ? p->sa_family : AF_UNSPEC;
    }