Golang binding

[zhiburt]

Hello @cyphar, that is a great crate.
This PR is going to close #3

I was tried to have done it with considerable safety but I am not sure, that I accomplished that :)

Not covered features (still have not managed all the information profoundly)

• duplicate
• from_fd
• intofd

I would like to mention that it was barely tested, I might should write a bunch of tests.

[cyphar]

Wow, awesome! I wasn't expecting this at all (I have draft Go bindings on my laptop I've been meaning to polish and push). I'll review this when I get home -- but one quick thing is that I need to you add Signed-off-by: [Your Name] <your email> to all of your commits. This is to certify that you agree with the Developer Certificate of Origin -- this is not a CLA, it's just a way for you to formally say that the PR is your own work (or is not your own work and you have the right to publish it under the relevant license). This is the same process Linux and many other large projects use.

[cyphar]

As for tests, I have plans for binding-agnostic tests (see #6) so we can wait for a while before adding tests for individual bindings (though I would suggest that you should add an example program in examples/ -- as a form of documentation for the bindings oh you already added a Go example).

[cyphar]

This is my initial review, I'll take a closer look when I get home.

[zhiburt]

That were the reasonable observations.

Signed-off-by: [Your Name] <your email>. The latest commits were provided with it but the initial ones are untouched since it seems I will have to make a force push.

And according the fact that the commit list have slightly grown may I just reopen it after the review process with the foregoing line?

What is more I have recently found out that commit style of the repo is different to mine, apparently that is my fault :(

[cyphar]

@zhiburt

The latest commits were provided with it but the initial ones are untouched since it seems I will have to make a force push.

Please just force-push to your branch. I would also prefer if you squashed the commits down into a few small pieces. If you've never used git rebase -i before, here's a video that might help.

Personally I find it helpful to also have commit messages that have a body (that way the git commit log contains information about why your patch does things in a particular way), but you don't need to bother with that too much if you don't personally find it helpful.

Also please note that all of your commits must have a Signed-off-by -- it's a legal requirement to avoid a bunch of potential problems in the future.

And according the fact that the commit list have slightly grown may I just reopen it after the review process with the foregoing line?

You don't need to open a new PR, just force push to your current PR's branch with fixed up commits.

What is more I have recently found out that commit style of the repo is different to mine, apparently that is my fault.

Don't worry about it too much. If it helps, the commit style of this repo is loosely based on the Linux kernel commit style or the style of several other container runtime projects like Docker and runc (and LXC, and LXD, and ...).

[cyphar]

Here are some more comments. It is looking a lot better, though there does appear to be a lot of needless wrapping (or re-definition) of things which are cgo-generated. We should opt to use the cgo-generated structures as much as possible -- not only will it make the code shorter it will make it somewhat more obvious which objects are from the C FFI interface and which objects are part of the binding.

I'm also quite worried about the pointer casting you're doing to a Go struct with a C pointer. That feels incredibly fragile to me.

[zhiburt]

Thank you for your helpful comments. I suppose the vast majority of the issues were resolved.

But I truly assume that it is not thread safe now. (If I understand error handling correctly, there can appear an issue) (I will check it and then change the comment in the nearest)

And it is not covered configure function yet

[cyphar]

@zhiburt You're right that technically the current error handling is not thread-safe. Maybe we should use thread-local storage to fix that (then again, @brauner complained very loudly when I suggested this a while ago). But the underlying libpathrs operations are thread-safe so I'll need to think about how to deal with this...

[cyphar]

I reckon this is my last set of reviews before I merge it. Please note that I am probably going to rewrite a few portions of it after merging (since I don't want to make you spend too much time cleaning up things I can just clean up myself). The binding looks pretty good otherwise.

Ignore the things I've written as NOTE TO SELF.

[cyphar]

Alright, the code looks good enough to me to merge -- thanks so much for contributing! 😸 There are still some small things that I think should be fixed up a little bit (such as doc comments and some of the function names), but I can do those myself.

However, there is still a bit of cleanup needed with your commits. c96b886 is missing a DCO Signed-off-by. Also the commits should be squashed more (ideally there should be just one commit to add the binding -- possibly with some information in the commit message, and one commit to add the example that uses the binding).

[cyphar]

This looks great. Let me know if you want to do any final cleanups before I merge.

[zhiburt]

It is definitely better rather than it was initially.
Thank you @cyphar, for your utterly helpful observations and reviews.
I hope it has been followed according to your expectations. And this PR has not affected any of your plans.

Thank you for this chance to participate in this awesome library.

---

Might it is ready to be merged, but I have to bring one question up, as I see duplicate does not have to consume object as well as we can see in rust example. But through ffi appears an issue with that (go example).

    let root = pathrs::Root::open(rootPath).unwrap();
    let root1 = root.try_clone().unwrap();
    let root2 = root.try_clone().unwrap();

    root.resolve(path).unwrap();

	root, err := pathrs.Open(rootPath)
	if err != nil {
		printPathError(err)
	}
	defer root.Close()

	root1, err := root.Clone()
	if err != nil {
		printPathError(err)
	}
	
	root2, err := root.Clone()
	if err != nil {
		printPathError(err)
	}

There will be produced an error in the second Clone call which statement is Error invalid ptr argument: invalid pathrs object, the same will goes after calling some method for instance Resolve on the original root variable (look bellow). When we call it on the cloned one it goes well.

	root, err := pathrs.Open(rootPath)
	if err != nil {
		printPathError(err)
	}
	defer root.Close()

	_, err = root.Clone()
	if err != nil {
		printPathError(err)
	}
	
        handle, err := root.Resolve(path)
	if err != nil {
		printPathError(err)
	}

I cannot bet that it is normal or not.

[cyphar]

Error invalid ptr argument: invalid pathrs object

That means that the internal pointer has been zeroed... I will look into that, but it's probably a bug in pathrs_duplicate. I can fix that later.

EDIT: Yeah, it's a bug in pathrs_duplicate. I stupidly used Option::take rather than Option::as_ref. b210aac should fix the issue.

[zhiburt]

That means that the internal pointer has been zeroed... I will look into that, but it's probably a bug in pathrs_duplicate. I can fix that later.

Great, I hope it is not an error of this binding since I tried to check everything.

This looks great. Let me know if you want to do any final cleanups before I merge.

I will check it up one more time a bit later, I hope it will have gone well. If I will not write anything by evening you can be sure I had checked it one more time and merge it.

[cyphar]

Yeah, it was a bug in pathrs_duplicate. I pushed a fix to master. But I also forgot to respond to your original comment. I hope that I wasn't too much of a perfectionist when giving reviews (I often struggle when reviewing PRs when to say "it's alright as-is, and I can fix any nits later"). Thanks so much for contributing, it's great to see someone contributing to something that is still a work-in-progress.

[cyphar]

I only just noticed this now (and I'm fixing it), but the usage of C.CString in this binding actually leaks memory because there is no call to C.free.