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

Support compile with clang on MacOS #285

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

rabits
Copy link
Contributor

@rabits rabits commented Feb 17, 2021

A number of changes to make the code compile on MacOS with clang.

The other patches in series:

@stgraber
Copy link
Contributor

Needs a few tweaks to make travis happy :)

@stgraber
Copy link
Contributor

Can you rebase this into a single commit?

@rabits
Copy link
Contributor Author

rabits commented Feb 17, 2021

Oh, sure)

@stgraber
Copy link
Contributor

What's defining those variables?
I'm not seeing matching autotools changes which would detect and set:

  • HAVE_SYS_UCRED_H
  • HAVE_DECL_LOCAL_PEERCRED
  • HAVE_DECL_SO_PEERCRED

@rabits
Copy link
Contributor Author

rabits commented Feb 17, 2021

Good catch! My bad, will adjust the configs

@rabits
Copy link
Contributor Author

rabits commented Feb 17, 2021

Yep, actually need to rewrite the bsd-part, it compiled just because definitions was not set...

@rabits
Copy link
Contributor Author

rabits commented Feb 17, 2021

@stgraber could you please suggest how to test it properly? I use go-dqlite, so where I will see it's working or not?

src/server.c Outdated
if (fd == -1) {
return DQLITE_ERROR;
}
fcntl(fd, O_CLOEXEC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add error checking for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fixed

@stgraber
Copy link
Contributor

I'm not all that familiar with the internals of dqlite, I certainly understand the goal of this logic but am not sure what's the best way to test it. I tried connecting to a number of the pipes held by LXD here, but that's not going anywhere, presumably because those are already connected, so I'm never hitting the code that would reject the connection (just stuck waiting for accept basically).

@freeekanayaka would definitely be able to assist there, maybe @MathieuBordere somehow already looked at that part of the codebase too but he's mostly been focused on raft issues so far ;)

@rabits rabits force-pushed the patch-1 branch 3 times, most recently from 600605b to e9fea2e Compare February 17, 2021 23:08
@MathieuBordere
Copy link
Contributor

I'm not all that familiar with the internals of dqlite, I certainly understand the goal of this logic but am not sure what's the best way to test it. I tried connecting to a number of the pipes held by LXD here, but that's not going anywhere, presumably because those are already connected, so I'm never hitting the code that would reject the connection (just stuck waiting for accept basically).

@freeekanayaka would definitely be able to assist there, maybe @MathieuBordere somehow already looked at that part of the codebase too but he's mostly been focused on raft issues so far ;)

Not too familiar with this part of the codebase, will have a look eventually if this stays open :-)

@freeekanayaka
Copy link
Contributor

If you run the go-dqlite tests (in particular the ones in the go-dqlite/app sub-package`), that should be enough to test the part that you are modifying. Essentially the code path that you touch in this PR gets triggered if you tell the dqlite engine to listen to an abstract unix socket rather to a TCP port (so for example you can proxy incoming connections with TLS). In that case you want to make sure that incoming connections are coming from the same process (e.g. a Go process performing the TLS proxy dance), otherwise any process on the host would be able to connect.

@rabits
Copy link
Contributor Author

rabits commented Feb 19, 2021

Thanks, folks - figured out there is other changes required after I compiled go-dqlite demo. Will continue to test and come back with a solution)

@rabits
Copy link
Contributor Author

rabits commented Feb 20, 2021

Ok, that was a journey. To run the node and walk through demo it will also require libraft changes, and some other adjustments, but ended up with the networking issues - seems the custom aligned_alloc is not working well and causing a mess in the communication protocol... Will prepare the changes and try to find some help with debugging.

@rabits rabits changed the title Support compile with clang on BSD Support compile with clang on MacOS Feb 20, 2021
@stgraber stgraber merged commit c0699eb into canonical:master Mar 17, 2021
@rabits rabits deleted the patch-1 branch April 27, 2021 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants