C library wrapper #71

phansGithub · 2023-08-01T15:28:35Z

This is to wrap the go client library for c using cgo, however there are two commits caused by syncing the fork, so maybe I can try squashing the commits together because one of the commits is just a merge.

garyloug · 2023-08-02T09:47:25Z

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/go-ozzo/ozzo-validation/v4 v4.3.0
 	github.com/google/gofuzz v1.1.0
 	github.com/google/uuid v1.3.0
+	github.com/intel/afxdp-plugins-for-kubernetes/pkg/goclient v0.0.0-20230728160820-93d43de9a412


can you check if it works with just v0.0.0, like the import below?

garyloug · 2023-08-02T09:48:50Z

go.mod

@@ -22,3 +23,5 @@ require (
 )

 replace github.com/intel/afxdp-plugins-for-kubernetes/pkg/subfunctions => ./pkg/subfunctions
+
+replace github.com/intel/afxdp-plugins-for-kubernetes/pkg/goclient => ./pkg/goclient


NIT: remove blank line between "replace" lines and always try have a blank line at the very bottom of the file, i.e. just under last "replace" line

garyloug · 2023-08-02T09:51:09Z

pkg/cclient/README.md

@@ -0,0 +1,67 @@
+#Compiling the C Library


Just a general comment on your readme - your headings aren't working, I think you need a space after the # symbols
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/07ae9f57207f0fb19d517734bd9055b051e6b2ef/pkg/cclient/README.md

garyloug · 2023-08-02T09:52:16Z

pkg/cclient/README.md

@@ -0,0 +1,67 @@
+#Compiling the C Library
+
+>NOTE: You may have to run ```go get github.com/intel/afxdp-plugins-for-kubernetes/pkg/goclient@93d43de```


You can remove this, was just a note for you as I had only just merged your goclient

garyloug · 2023-08-02T09:59:48Z

pkg/cclient/README.md

+```go build -o lib_udsclient.so -buildmode=c-shared cclient.go```
+	- this creates .h and .so files from the cclient.go file.
+
+```cp lib_udsclient.so /lib```


You've gone above and beyond here on build documentation, no need for anything beyond how to generate the .so .a and .h files. A C developer will know how to take it from there.

All the "test app" stuff I just sent to yourself for your own info and testing.

This build section of the readme will be very short...

garyloug · 2023-08-02T10:42:05Z

pkg/cclient/cclient.go

+		return C.CString(response), -1
+	}
+
+	cleaners[0] = function


These can be removed in favor of a single global variable.
We should also be able to assign that variable directly when we're calling the goclient, see the code snippet in the comment at line 34.

garyloug · 2023-08-02T10:43:55Z

pkg/cclient/cclient.go

+		return 0, errVal
+	}
+
+	cleaners[1] = function


These can be removed in favor of a single global variable.

garyloug · 2023-08-02T10:43:56Z

pkg/cclient/cclient.go

+ServerVersion is an exported version for c of goclient's XskMapFd()
+*/
+//export XskMapFd
+func XskMapFd(device *C.char) (fd, errVal C.int) {


Again, watch the function name and we need a single return value, the fd which is a C.int
Check your error and handle it just like the version function.
Return a -1 int if there are any problems.

garyloug · 2023-08-02T10:50:16Z

pkg/cclient/cclient.go

+ServerVersion is an exported version for c of goclient's RequestBusyPoll()
+*/
+//export RequestBusyPoll
+func RequestBusyPoll(busyTimeout, busyBudget, fd int) C.int {


This function is fine, good name, single return, handling the error from the Go client (maybe just update the error handling with error message like the rest).

I think the variables you're passing in busyTimeout, busyBudget, fd all need to be of type C.int ?

It might complain about type mismatch, not sure. You might then need to convert/cast them to Go ints to pass to your Go client. I think it'll look something like:
CleanupFunc, err := goclient.RequestBusyPoll(int(busyTimeout), int(busyBudget), int(fd))

garyloug · 2023-08-02T11:03:28Z

pkg/cclient/cclient.go

+Pass in one of the available function names to clean up the connection after use.
+*/
+//export CleanUpConnection
+func CleanUpConnection(function *C.char) {


Cleanup function is way too complicated. I think you need to simply:

check the global variable, I'm referring to it as CleanupFunc, is not nil

call it

All of the go client functions return a cleanup function, but it's the exact same function they're all returning, so I think it's fine just to have a single variable to that function, rather than an array of functions.

They all just return this:
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/uds/uds.go#L297

Again "CloseUdsConnection" might be a little more readable in C

phansGithub · 2023-08-03T13:27:07Z

Quick thing: The changes to the UDS Server seem to have been overwritten by my first force-push, I think it's because the branch I was working on didn't branch off of main before I changed the UDS server

Signed-off-by: phansGithub <[email protected]>

maryamtahhan · 2023-09-20T10:18:25Z

pkg/cclient/README.md

@@ -0,0 +1,36 @@
+# Compiling the C Library
+
+## Creating the .h and the .so files:


Suggested change

## Creating the .h and the .so files:

## Creating the shared library and header files:

pkg/cclient/README.md

maryamtahhan · 2023-09-20T10:19:12Z

pkg/cclient/README.md

+## Creating the .h and the .so files:
+
+
+```go build -o lib_udsclient.so -buildmode=c-shared cclient.go```


Suggested change

```go build -o lib_udsclient.so -buildmode=c-shared cclient.go```

```cmd

go build -o lib_udsclient.so -buildmode=c-shared cclient.go

Again I'm surprised the linter didn't flag this

maryamtahhan · 2023-09-20T10:20:10Z

pkg/cclient/README.md

+	- this creates .h and .so files from the cclient.go file.
+
+
+## Creating the .h and the .a files:


Suggested change

## Creating the .h and the .a files:

## Creating the static library and header files:

maryamtahhan · 2023-09-20T10:20:28Z

pkg/cclient/README.md

+
+## Creating the .h and the .a files:
+
+```go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go```


Suggested change

```go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go```

```cmd

go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go

maryamtahhan · 2023-09-20T10:24:51Z

pkg/cclient/README.md

+This returns the version of our handshake on the server/host.
+
+```int RequestXskMapFd(char* device)```
+This requests an Xsk map Fd for a specifed device.


Suggested change

This requests an Xsk map Fd for a specifed device.

This requests an xskmap Fd for a specified device.

maryamtahhan · 2023-09-20T10:25:33Z

pkg/cclient/README.md

+
+```void CleanUpConnection()```
+After all the other functions are called, this needs to be called after all the work you need to do is done,
+so that the connection to the Uds can be cleaned up.


Suggested change

so that the connection to the Uds can be cleaned up.

so that the connection to the UDS can be cleaned up.

maryamtahhan · 2023-09-20T10:27:05Z

pkg/cclient/cclient.go

+var cleaner uds.CleanupFunc
+
+/*
+GetClientVersion is an exported version for c of goclient's GetClientVersion()


Suggested change

GetClientVersion is an exported version for c of goclient's GetClientVersion()

GetClientVersion is an exported version for c of the goclient GetClientVersion()

same throughout the file... I don't think goclient's is grammatically correct

maryamtahhan · 2023-09-20T10:28:11Z

pkg/cclient/cclient.go

+*/
+//export RequestXskMapFd
+func RequestXskMapFd(device *C.char) (fd C.int) {
+	fdVal, function, err := goclient.RequestXSKmapFD(C.GoString(device))


should you check device isn't Nil before passing it along to RequestXSKmapFD

maryamtahhan · 2023-09-20T10:29:18Z

pkg/cclient/cclient.go

+*/
+//export RequestBusyPoll
+func RequestBusyPoll(busyTimeout, busyBudget, fd C.int) C.int {
+	function, err := goclient.RequestBusyPoll(int(busyTimeout), int(busyBudget), int(fd))


should probably do some param error checking before calling RequestBusyPoll

Signed-off-by: phansGithub <[email protected]>

garyloug suggested changes Aug 2, 2023

View reviewed changes

phansGithub force-pushed the cLibraryWrapper branch 2 times, most recently from d951400 to 03efbad Compare August 3, 2023 09:02

phansGithub force-pushed the cLibraryWrapper branch from 03efbad to 7a86b34 Compare August 9, 2023 09:21

feat: Created the go C Wrapper files

32ac1d3

Signed-off-by: phansGithub <[email protected]>

phansGithub force-pushed the cLibraryWrapper branch from 7a86b34 to 32ac1d3 Compare August 9, 2023 09:23

maryamtahhan suggested changes Sep 20, 2023

View reviewed changes

Made changes to README instructions and cclient error checking.

89ebed7

Signed-off-by: phansGithub <[email protected]>

phansGithub force-pushed the cLibraryWrapper branch 2 times, most recently from ae286cc to 89ebed7 Compare October 4, 2023 14:38

garyloug approved these changes Oct 12, 2023

View reviewed changes

garyloug merged commit db2fd90 into intel:main Oct 12, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C library wrapper #71

C library wrapper #71

phansGithub commented Aug 1, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

garyloug Aug 2, 2023

phansGithub commented Aug 3, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

maryamtahhan Sep 20, 2023

		@@ -0,0 +1,67 @@
		#Compiling the C Library

		>NOTE: You may have to run ```go get github.com/intel/afxdp-plugins-for-kubernetes/pkg/goclient@93d43de```

		@@ -0,0 +1,36 @@
		# Compiling the C Library

		## Creating the .h and the .so files:

	## Creating the .h and the .so files:
	## Creating the shared library and header files:

		## Creating the .h and the .so files:


		```go build -o lib_udsclient.so -buildmode=c-shared cclient.go```

	```go build -o lib_udsclient.so -buildmode=c-shared cclient.go```
	```cmd
	go build -o lib_udsclient.so -buildmode=c-shared cclient.go

		- this creates .h and .so files from the cclient.go file.


		## Creating the .h and the .a files:

	## Creating the .h and the .a files:
	## Creating the static library and header files:


		## Creating the .h and the .a files:

		```go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go```

	```go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go```
	```cmd
	go build -o lib_udsclient.a -buildmode=c-archive ./cclient.go

	This requests an Xsk map Fd for a specifed device.
	This requests an xskmap Fd for a specified device.

	so that the connection to the Uds can be cleaned up.
	so that the connection to the UDS can be cleaned up.

	GetClientVersion is an exported version for c of goclient's GetClientVersion()
	GetClientVersion is an exported version for c of the goclient GetClientVersion()

C library wrapper #71

C library wrapper #71

Conversation

phansGithub commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansGithub commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment