Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/sys/windows: add support for SOCKADDR_BTH structure #53929

Closed
PumpkinSeed opened this issue Jul 18, 2022 · 9 comments
Closed

x/sys/windows: add support for SOCKADDR_BTH structure #53929

PumpkinSeed opened this issue Jul 18, 2022 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@PumpkinSeed
Copy link

Problem

There are several interfaces in the x/sys like these:

These all contain unexported method descriptors, so there is no way to add own implementation. We run into the issue, that we needed a windows extension and even if we can implement that, we can't use it with this solution.

Solution

Make the method descriptors exported.

@gopherbot gopherbot added this to the Proposal milestone Jul 18, 2022
@ianlancetaylor
Copy link
Contributor

Can you provide more details about why you want to define your own Sockaddr type? At first glance it only seems useful if the system supports it. And if the system supports it, we should just add it to these packages anyhow.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 18, 2022
@PumpkinSeed
Copy link
Author

PumpkinSeed commented Jul 19, 2022

I tried to implement SOCKADDR_BTH for Windows. There is an example implementation (I'm not a big expert in Windows programming at all, and I only know about the existence of these 1 week ago). But I think this is something which is missing from the x/sys. Also the Bluetooth handling is pretty weak as well. It works with Linux perfectly, it has examples as well. But Windows is tricky and MacOS implementation is not fitting into the x/sys I think.
I found out that there is NO an easy way to talk to MacOS in a simple way like linux and windows do via syscalls. On that case there is the Core Bluetooth module.

On the otherhand I think it make sense to open this interface, even if the SOCKADDR_BTH will be added. Because anytime a new protocol/anything comes in, the developers needs to open a similar issue and waiting until it will be released. Is there anything why it shouldn't be more open what I don't see right now?

@ianlancetaylor
Copy link
Contributor

If we export the method, then we have to have documentation and tests and ongoing maintenance for user defined socket types. Since new socket types are rare, that doesn't seem like a good use of maintainer time. We should just add support for SOCKADR_BTH to x/sys/windows.

@PumpkinSeed
Copy link
Author

Well, that's a solution as well.

How it would look like? Because I'm open for contribution, but I didn't have a working example yet. The repository what I linked has issues what I didn't fix yet. So I'm heavily working on it.

@PumpkinSeed
Copy link
Author

Update on that field, I have a Proof of Concept implementation. The good news that the only thing which missing is the SOCKADDR_BTH struct and it's method. So I could use the built-in functions from the x/sys/windows package. (Side-note I copy pasted the Connect function for investigation purposes.)

So what I did:

  1. Implemented the whole thing in C.
  2. Since I couldn't replicate a struct on the Go side, I just checked what is the byte array what the C implementation sends to the connect, so I printed the SOCKADDR_BTH instance as byte array.
  3. I translated this byte array to Go byte array and add it here, so I'm sending this to the Connect function as name parameter.

This is working, so it proves, that the current functions can handle this feature.

The only leftover is to figure out a struct and it's method which can create a similar byte array after the connect calls the sockaddr() method.

@PumpkinSeed
Copy link
Author

After a massive investigation I printed out the raw memory what the C implementation sends. I got invalid parameters error so I checked the differences between the same struct what I had in Go against C. I found out that the Go implementation doesn't respect the data alignment. I guess it's because missing the attribute's packed. Otherwise I compared how the data I send should look like.

    //     uint16                  | uint64                         | windows.GUID                                    | uint32
    // Go: 20 00 00 00 00 00 00 00 | d2 cd 7f 2d 81 54 00 00        | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | 06 00 00 00 00 00 00 00
    // c:  20 00                   | d2 cd 7f 2d 81 54 00 00        | 9f a7 ae 00 00 00 70 f4 9f a7 ae 00 00 00 74 f4 | 06 00 00 00

So I created a raw struct based on that and implemented the Sockaddr interface for it. I have a solution which works for me, but I don't know whether it's a good solution or not.

type SockaddrBth struct {
    BtAddr         [6]byte
    ServiceClassId windows.GUID
    Port           uint32

    raw RawSockaddrBth
}

type RawSockaddrBth struct {
    Family         [2]byte
    Addr           [8]byte
    ServiceClassId [16]byte
    Port           [4]byte
}

func (sa *SockaddrBth) sockaddr() (unsafe.Pointer, int32, error) {
    family := windows.AF_BTH
    sa.raw = RawSockaddrBth{
        Family:         *(*[2]byte)(unsafe.Pointer(&family)),
        Addr:           *(*[8]byte)(unsafe.Pointer(&sa.BtAddr)),
        Port:           *(*[4]byte)(unsafe.Pointer(&sa.Port)),
        ServiceClassId: *(*[16]byte)(unsafe.Pointer(&sa.ServiceClassId)),
    }
    return unsafe.Pointer(&sa.raw), int32(unsafe.Sizeof(sa.raw)), nil
}

This is working for me properly. Let me know your thoughts. If you agree with it, I can send a PR or similar to Gerrit.

@ianlancetaylor
Copy link
Contributor

I haven't looked at the details but that looks like the right general idea. Thanks.

@PumpkinSeed
Copy link
Author

Cool, Then I will open a PR. I have seen that the RawSockaddrAny should support it, as well. Can it be a separate PR later?

@gopherbot
Copy link

Change https://go.dev/cl/419394 mentions this issue: windows: support Windows SOCKADDR_BTH structure

@dmitshur dmitshur changed the title proposal: x/sys: Sockaddr interface should have exported method descriptor x/sys/windows: add support for SOCKADDR_BTH structure Aug 2, 2022
@dmitshur dmitshur modified the milestones: Proposal, Unreleased Aug 2, 2022
@dmitshur dmitshur removed this from Incoming in Proposals (old) Aug 2, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Aug 2, 2022
@golang golang locked and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants