-
Notifications
You must be signed in to change notification settings - Fork 53
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
mobileproxy clib #226
base: main
Are you sure you want to change the base?
mobileproxy clib #226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to use unsafe.Pointer
as the type. cgo.Handle
only allows you to pass object without pinning.
Co-authored-by: J. Yi <[email protected]>
Co-authored-by: J. Yi <[email protected]>
Co-authored-by: J. Yi <[email protected]>
Co-authored-by: J. Yi <[email protected]>
x/mobileproxy/clib/clib.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to see this in action to be confident it works.
Please add a program that uses this library.
Some possibilities would be a c program, or a go program that calls this library via cgo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, great! We're already at that stage I guess!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of timing, maybe I can move this into experimental for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean with moving to experimental.
The binary can be in the examples folder, but we need it in order to know it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant examples! Okay, sure, I'll commit the binary to examples, and we can go from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like to use Abseil for flags, etc: https://abseil.io/docs/cpp/quickstart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying we shouldn't have a program! Just that given timing, I'd like to get the library in so a test program can be written around it, rather than it just hanging in the air. Do you feel a program blocks this PR @jyyi1 or can it be added in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have this code checked in only when we have proof it works. I see no harm in keeping it in a branch if we can't produce a program that can use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Can't" is a strong word, we certainly can, but maybe not by the end of the week since I'm learning C as we go here: I'll see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have started the program based on @jyyi1's initial draft and will review that with him today.
x/examples/mobileproxy-clib/main.go
Outdated
proxyObject, err := mobileproxy.RunProxy(C.GoString(address), &dialerObject) | ||
|
||
if err != nil { | ||
// TODO: print something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must have a way to return errors. Go Mobile returns a struct instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyyi1 you can return a tuple right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can return a tuple and paste the header here.
@fortuna, do you mean a more C-friendly way of returning errors, like below?
Proxy *proxy;
int err = CreateProxy(&proxy);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to return an error meesage.
Also, which design should we use?
- Return error (proxy reference)
- Return proxy (error reference)
- Return tuple
- return nothing (error and proxy reference)
I think Go Mobile on ios returns the value and writes the error to an input reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lean towards tuple with two pointers in it. Since we're throwing around unsafe pointers, with the tuple approach the caller doesn't have to do any guesswork around what kind of pointer they've received. But maybe there's a C way to do it? @jyyi1 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyyi1 should we be using errno? https://www.geeksforgeeks.org/error-handling-in-c/#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errno
is very Unix specific. But sure, we can introduce our own errno
. That's actually my code above, we'll return an int
, and put the proxy into a "output parameter".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to return an error message too. Errno is not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going with tuple w/ error message as second optional argument
// Signal (i.e. Ctrl+C) is a Unix/Linux only API, Windows doesn't use it | ||
// So it also depends on whether we need the program to be cross-platform | ||
printf("Running proxy on 127.0.0.1:1234. Press <Enter> to terminate: "); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no call to read the Enter here.
Consider sigaction
: https://stackoverflow.com/a/19059462
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this file is in progress. Converting this PR back to draft!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigaction
is not cross-platform. I'd rather use a standard library function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It's in the POSIX standard: https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html
Does Windows not implement it? I guess signal is probably fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sigaction
, but signal
is good.
StreamDialerPtr *dialer; | ||
ProxyPtr *proxy; | ||
|
||
dialer = NewStreamDialerFromConfig("split:3"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, converting this to draft as we figure out that piece.
Command to run:
cd outline-sdk CGO_ENABLED=1 go build -buildmode=c-shared ./x/examples/mobileproxy-clib
Header file output:
TODOs:
Enter
and shut down the server