Eric Blake
2023-Jul-26 17:50 UTC
[Libguestfs] [libnbd PATCH 4/7] Revert "generator/Go.ml: Simplify copy_uint32_array"
This reverts commit 6725fa0e129f9a60d7b89707ef8604e0aeeeaf43, although
with a more modern style.
Casting a C array to a Go slice just to benefit from potential
optimizations in Go's copy(), is rather complex to understand,
especially when we are still copying things (the main reason to treat
a C array as a Go slice is when avoiding a copy has a benefit).
Without a benchmark showing measurable difference in runtime speed,
and considering that network transit time probably dominates the time
spent on block status and its callback, it is not worth the
complexity. Furthermore, an upcoming patch wants to add a similar
helper function for converting between a list of C and Go structs,
where the copy() trick will not work; and having the two helpers look
alike is beneficial.
Suggested-by: Laszlo Ersek <lersek at redhat.com>
CC: Nir Soffer <nsoffer at redhat.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---
generator/GoLang.ml | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 0aa83bdc..77dacadb 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -509,12 +509,14 @@ let
/* Closures. */
-func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
- ret := make([]uint32, int (count))
- // See
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
- // TODO: Use unsafe.Slice() when we require Go 1.17.
- s := (*[1<<30]uint32)(unsafe.Pointer(entries))[:count:count]
- copy(ret, s)
+func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
+ ret := make([]uint32, int(count))
+ addr := uintptr(unsafe.Pointer(entries))
+ for i := 0; i < int(count); i++ {
+ ptr := (*C.uint32_t)(unsafe.Pointer(addr))
+ ret[i] = uint32(*ptr)
+ addr += unsafe.Sizeof(*ptr)
+ }
return ret
}
";
base-commit: 5c2fc3cc7e14146d000b65b191e70d9a0585a395
prerequisite-patch-id: 4db98f7b211c0de9a4095b300970e1973cf0716c
prerequisite-patch-id: 0bb320af5109c1c21e5b76d44e6ec1e7e685fd9f
prerequisite-patch-id: 205525d8ea09e77ea13f43d0720153ed5904dbcd
--
2.41.0
Eric Blake
2023-Jul-26 17:50 UTC
[Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files
I ran:
gofmt -s -w $(git ls-files -- '**/*.go')
then touched up a few comments in test 590 where it mis-interpreted
our intentions of having a single sentence that occupies more than 80
columns.
Most of the changes are whitespace fixes (consistent use of TAB,
altering the whitespace around operators), using preferred ordering
for imports, and eliding excess syntax (unneeded () or ;).
This patch only adjusts files directly in git control. Later patches
will tackle (most) Go style problems in the generated files, then
automate a way to ensure we don't regress.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
golang/configure/test.go | 14 ++++----
golang/examples/aio_copy/aio_copy.go | 7 ++--
golang/examples/simple_copy/simple_copy.go | 3 +-
golang/libnbd_220_opt_list_test.go | 4 +--
golang/libnbd_240_opt_list_meta_test.go | 32 +++++++++----------
.../libnbd_245_opt_list_meta_queries_test.go | 14 ++++----
golang/libnbd_250_opt_set_meta_test.go | 2 +-
.../libnbd_255_opt_set_meta_queries_test.go | 4 +--
golang/libnbd_405_pread_structured_test.go | 10 +++---
golang/libnbd_590_aio_copy_test.go | 21 ++++++++----
golang/libnbd_620_stats_test.go | 12 +++----
11 files changed, 65 insertions(+), 58 deletions(-)
diff --git a/golang/configure/test.go b/golang/configure/test.go
index 11911377..fe742f2b 100644
--- a/golang/configure/test.go
+++ b/golang/configure/test.go
@@ -31,11 +31,11 @@ func main() {
fmt.Println(runtime.Version())
/* XXX Check for minimum runtime.Version() >= "go1.1.1"
- * Unfortunately go version numbers are not easy to parse.
- * They have the 3 formats "goX.Y.Z", "release.rN"
or
- * "weekly.YYYY-MM-DD". The latter two formats are mostly
- * useless, and the first one is hard to parse. See also
- * cmpGoVersion in
- *
http://web.archive.org/web/20130402235148/http://golang.org/src/cmd/go/get.go?m=text
- */
+ * Unfortunately go version numbers are not easy to parse.
+ * They have the 3 formats "goX.Y.Z", "release.rN" or
+ * "weekly.YYYY-MM-DD". The latter two formats are mostly
+ * useless, and the first one is hard to parse. See also
+ * cmpGoVersion in
+ *
http://web.archive.org/web/20130402235148/http://golang.org/src/cmd/go/get.go?m=text
+ */
}
diff --git a/golang/examples/aio_copy/aio_copy.go
b/golang/examples/aio_copy/aio_copy.go
index e3093da8..1de115b1 100644
--- a/golang/examples/aio_copy/aio_copy.go
+++ b/golang/examples/aio_copy/aio_copy.go
@@ -36,8 +36,7 @@
//
// Example:
//
-// ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
-//
+// ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
package main
import (
@@ -66,8 +65,8 @@
// libnbd, until the command reach the front of the queue and can be writen to
// the output.
type command struct {
- buf libnbd.AioBuffer
- ready bool
+ buf libnbd.AioBuffer
+ ready bool
}
func main() {
diff --git a/golang/examples/simple_copy/simple_copy.go
b/golang/examples/simple_copy/simple_copy.go
index aa9e89b4..7c4182f6 100644
--- a/golang/examples/simple_copy/simple_copy.go
+++ b/golang/examples/simple_copy/simple_copy.go
@@ -36,8 +36,7 @@
//
// Example:
//
-// ./simple_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
-//
+// ./simple_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
package main
import (
diff --git a/golang/libnbd_220_opt_list_test.go
b/golang/libnbd_220_opt_list_test.go
index 264a50a7..ca888829 100644
--- a/golang/libnbd_220_opt_list_test.go
+++ b/golang/libnbd_220_opt_list_test.go
@@ -19,10 +19,10 @@
package libnbd
import (
+ "fmt"
"os"
"os/exec"
"testing"
- "fmt"
)
var exports []string
@@ -94,7 +94,7 @@ func Test220OptList(t *testing.T) {
if count != 2 {
t.Fatalf("unexpected count after opt_list")
}
- if len(exports) != 2 || exports[0] != "a" || exports[1] !=
"b" {
+ if len(exports) != 2 || exports[0] != "a" || exports[1] !=
"b" {
t.Fatalf("unexpected exports contents after opt_list")
}
})
diff --git a/golang/libnbd_240_opt_list_meta_test.go
b/golang/libnbd_240_opt_list_meta_test.go
index 011b5704..afbeb01b 100644
--- a/golang/libnbd_240_opt_list_meta_test.go
+++ b/golang/libnbd_240_opt_list_meta_test.go
@@ -19,7 +19,7 @@
package libnbd
import (
- "fmt";
+ "fmt"
"testing"
)
@@ -31,7 +31,7 @@ func listmetaf(user_data int, name string) int {
panic("expected user_data == 42")
}
list_count++
- if (name == CONTEXT_BASE_ALLOCATION) {
+ if name == CONTEXT_BASE_ALLOCATION {
list_seen = true
}
return 0
@@ -62,7 +62,7 @@ func Test240OptListMeta(t *testing.T) {
list_count = 0
list_seen = false
r, err := h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
@@ -80,7 +80,7 @@ func Test240OptListMeta(t *testing.T) {
t.Fatalf("could not request add_meta_context: %s", err)
}
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
@@ -111,7 +111,7 @@ func Test240OptListMeta(t *testing.T) {
t.Fatalf("wrong result of get_meta_context: %s", *tmp)
}
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
@@ -121,7 +121,7 @@ func Test240OptListMeta(t *testing.T) {
}
/* Fourth pass: opt_list_meta_context is stateless, so it should
- * not wipe status learned during opt_info
+ * not wipe status learned during opt_info
*/
list_count = 0
list_seen = false
@@ -137,13 +137,13 @@ func Test240OptListMeta(t *testing.T) {
if err != nil {
t.Fatalf("opt_info failed unexpectedly: %s", err)
}
- size, err := h.GetSize()
+ size, err := h.GetSize()
if err != nil {
t.Fatalf("get_size failed unexpectedly: %s", err)
}
- if size != 1048576 {
+ if size != 1048576 {
t.Fatalf("get_size gave wrong size")
- }
+ }
meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION)
if err != nil {
t.Fatalf("can_meta_context failed unexpectedly: %s", err)
@@ -160,7 +160,7 @@ func Test240OptListMeta(t *testing.T) {
t.Fatalf("could not request add_meta_context: %s", err)
}
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
@@ -168,13 +168,13 @@ func Test240OptListMeta(t *testing.T) {
if r != 0 || r != list_count || list_seen {
t.Fatalf("unexpected count after opt_list_meta_context")
}
- size, err = h.GetSize()
+ size, err = h.GetSize()
if err != nil {
t.Fatalf("get_size failed unexpectedly: %s", err)
}
- if size != 1048576 {
+ if size != 1048576 {
t.Fatalf("get_size gave wrong size")
- }
+ }
meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION)
if err != nil {
t.Fatalf("can_meta_context failed unexpectedly: %s", err)
@@ -192,7 +192,7 @@ func Test240OptListMeta(t *testing.T) {
t.Fatalf("could not request add_meta_context: %s", err)
}
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
@@ -241,7 +241,7 @@ func Test240OptListMeta(t *testing.T) {
list_count = 0
list_seen = false
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
bytes2, err2 := h.StatsBytesSent()
@@ -268,7 +268,7 @@ func Test240OptListMeta(t *testing.T) {
list_count = 0
list_seen = false
r, err = h.OptListMetaContext(func(name string) int {
- return listmetaf(42, name)
+ return listmetaf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context: %s", err)
diff --git a/golang/libnbd_245_opt_list_meta_queries_test.go
b/golang/libnbd_245_opt_list_meta_queries_test.go
index 3ae2d854..1451b2fc 100644
--- a/golang/libnbd_245_opt_list_meta_queries_test.go
+++ b/golang/libnbd_245_opt_list_meta_queries_test.go
@@ -28,7 +28,7 @@ func listmetaqf(user_data int, name string) int {
panic("expected user_data == 42")
}
listq_count++
- if (name == CONTEXT_BASE_ALLOCATION) {
+ if name == CONTEXT_BASE_ALLOCATION {
listq_seen = true
}
return 0
@@ -64,9 +64,9 @@ func Test245OptListMetaQueries(t *testing.T) {
if err != nil {
t.Fatalf("could not request add_meta_context: %s", err)
}
- r, err := h.OptListMetaContextQueries([]string{ },
+ r, err := h.OptListMetaContextQueries([]string{},
func(name string) int {
- return listmetaqf(42, name)
+ return listmetaqf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context_queries: %s",
err)
@@ -82,9 +82,9 @@ func(name string) int {
if err != nil {
t.Fatalf("could not request add_meta_context: %s", err)
}
- r, err = h.OptListMetaContextQueries([]string{ "x-nosuch:" },
+ r, err = h.OptListMetaContextQueries([]string{"x-nosuch:"},
func(name string) int {
- return listmetaqf(42, name)
+ return listmetaqf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context_queries: %s",
err)
@@ -97,9 +97,9 @@ func(name string) int {
listq_count = 0
listq_seen = false
r, err = h.OptListMetaContextQueries([]string{
- "x-nosuch:", CONTEXT_BASE_ALLOCATION },
+ "x-nosuch:", CONTEXT_BASE_ALLOCATION},
func(name string) int {
- return listmetaqf(42, name)
+ return listmetaqf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_list_meta_context_queries: %s",
err)
diff --git a/golang/libnbd_250_opt_set_meta_test.go
b/golang/libnbd_250_opt_set_meta_test.go
index 4263ede5..d2068716 100644
--- a/golang/libnbd_250_opt_set_meta_test.go
+++ b/golang/libnbd_250_opt_set_meta_test.go
@@ -28,7 +28,7 @@ func setmetaf(user_data int, name string) int {
panic("expected user_data == 42")
}
set_count++
- if (name == CONTEXT_BASE_ALLOCATION) {
+ if name == CONTEXT_BASE_ALLOCATION {
set_seen = true
}
return 0
diff --git a/golang/libnbd_255_opt_set_meta_queries_test.go
b/golang/libnbd_255_opt_set_meta_queries_test.go
index 232560bc..bdfacd63 100644
--- a/golang/libnbd_255_opt_set_meta_queries_test.go
+++ b/golang/libnbd_255_opt_set_meta_queries_test.go
@@ -28,7 +28,7 @@ func setmetaqf(user_data int, name string) int {
panic("expected user_data == 42")
}
setq_count++
- if (name == CONTEXT_BASE_ALLOCATION) {
+ if name == CONTEXT_BASE_ALLOCATION {
setq_seen = true
}
return 0
@@ -101,7 +101,7 @@ func(name string) int {
r, err = h.OptSetMetaContextQueries([]string{
"x-nosuch:context", CONTEXT_BASE_ALLOCATION},
func(name string) int {
- return setmetaqf(42, name)
+ return setmetaqf(42, name)
})
if err != nil {
t.Fatalf("could not request opt_set_meta_context_queries: %s", err)
diff --git a/golang/libnbd_405_pread_structured_test.go
b/golang/libnbd_405_pread_structured_test.go
index ac4cb3e5..ac7d834a 100644
--- a/golang/libnbd_405_pread_structured_test.go
+++ b/golang/libnbd_405_pread_structured_test.go
@@ -26,7 +26,7 @@
var expected405 = make([]byte, 512)
func psf(user_data int, buf2 []byte, offset uint64, status uint,
- error *int) int {
+ error *int) int {
if user_data != 42 {
panic("expected user_data == 42")
}
@@ -68,10 +68,10 @@ func Test405PReadStructured(t *testing.T) {
buf := make([]byte, 512)
err = h.PreadStructured(buf, 0,
- func(buf2 []byte, offset uint64, status uint,
- error *int) int {
- return psf(42, buf2, offset, status, error)
- }, nil)
+ func(buf2 []byte, offset uint64, status uint,
+ error *int) int {
+ return psf(42, buf2, offset, status, error)
+ }, nil)
if err != nil {
t.Fatalf("%s", err)
}
diff --git a/golang/libnbd_590_aio_copy_test.go
b/golang/libnbd_590_aio_copy_test.go
index c22653a7..e8c32dc0 100644
--- a/golang/libnbd_590_aio_copy_test.go
+++ b/golang/libnbd_590_aio_copy_test.go
@@ -28,8 +28,11 @@
var bytes_read = uint(0)
var bytes_written = uint(0)
-/* Functions to handle FdSet.
- XXX These probably only work on 64 bit platforms. */
+/*
+Functions to handle FdSet.
+
+ XXX These probably only work on 64 bit platforms.
+*/
func fdset_set(set *syscall.FdSet, fd int) {
(*set).Bits[fd/64] |= 1 << (uintptr(fd) % 64)
}
@@ -56,8 +59,11 @@ type wbuf struct {
var writes []wbuf
-/* Called whenever any asynchronous pread command from
- the source has completed. */
+/*
+Called whenever any asynchronous pread command from
+
+ the source has completed.
+*/
func read_completed(buf AioBuffer, offset uint64) int {
bytes_read += buf.Size
/* Move the AIO buffer to the write queue. */
@@ -66,8 +72,11 @@ func read_completed(buf AioBuffer, offset uint64) int {
return 1
}
-/* Called whenever any asynchronous pwrite command to the
- destination has completed. */
+/*
+Called whenever any asynchronous pwrite command to the
+
+ destination has completed.
+*/
func write_completed(buf AioBuffer) int {
bytes_written += buf.Size
/* Now we have to manually free the AIO buffer. */
diff --git a/golang/libnbd_620_stats_test.go b/golang/libnbd_620_stats_test.go
index 7b4ba009..63667160 100644
--- a/golang/libnbd_620_stats_test.go
+++ b/golang/libnbd_620_stats_test.go
@@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) {
t.Fatalf("%s", err)
}
- if bs2 != bs1 + 28 {
+ if bs2 != bs1+28 {
t.Fatalf("unexpected value for bs2")
}
- if cs2 != cs1 + 1 {
+ if cs2 != cs1+1 {
t.Fatalf("unexpected value for cs2")
}
- if br2 != br1 + 16 { /* assumes nbdkit uses simple reply */
+ if br2 != br1+16 { /* assumes nbdkit uses simple reply */
t.Fatalf("unexpected value for br2")
}
- if cr2 != cr1 + 1 {
+ if cr2 != cr1+1 {
t.Fatalf("unexpected value for cr2")
}
@@ -169,13 +169,13 @@ func Test620Stats(t *testing.T) {
if bs3 <= bs2 {
t.Fatalf("unexpected value for bs3")
}
- if cs3 != cs2 + 1 {
+ if cs3 != cs2+1 {
t.Fatalf("unexpected value for cs3")
}
if br3 < br2 {
t.Fatalf("unexpected value for br3")
}
- if cr3 != cr2 + slop {
+ if cr3 != cr2+slop {
t.Fatalf("unexpected value for cr3")
}
}
--
2.41.0
Eric Blake
2023-Jul-26 17:50 UTC
[Libguestfs] [libnbd PATCH 6/7] golang: Improve whitespace style in generated bindings
Use Go-preferred whitespace: TAB for indent, and no space before
opening '(', better use of blank lines. These changes reduce the size
of a diff produced by gofmt, although there are still other places
where we are not idiomatic in what we generate (mainly in lining up
columns of = in const() blocks).
Signed-off-by: Eric Blake <eblake at redhat.com>
---
generator/GoLang.ml | 244 ++++++++++++++++++++++----------------------
1 file changed, 123 insertions(+), 121 deletions(-)
diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 77dacadb..a38aa19f 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -175,6 +175,7 @@ let
let print_binding (name, { args; optargs; ret; shortdesc }) let cname =
camel_case name in
+ pr "\n";
(* Tedious method of passing optional arguments in golang. *)
if optargs <> [] then (
pr "/* Struct carrying optional arguments for %s. */\n" cname;
@@ -182,10 +183,10 @@ let
List.iter (
fun optarg ->
let fname = go_name_of_optarg optarg in
- pr " /* %s field is ignored unless %sSet == true. */\n"
+ pr "\t/* %s field is ignored unless %sSet == true. */\n"
fname fname;
- pr " %sSet bool\n" fname;
- pr " %s " fname;
+ pr "\t%sSet bool\n" fname;
+ pr "\t%s " fname;
(match optarg with
| OClosure { cbname } -> pr "%sCallback" (camel_case
cbname)
| OFlags (_, {flag_prefix}, _) -> pr "%s" (camel_case
flag_prefix)
@@ -198,7 +199,7 @@ let
(* Define the golang function which calls the C wrapper. *)
pr "/* %s: %s */\n" cname shortdesc;
- pr "func (h *Libnbd) %s (" cname;
+ pr "func (h *Libnbd) %s(" cname;
let comma = ref false in
List.iter (
fun arg ->
@@ -217,103 +218,103 @@ let
| Some t -> pr "(%s, error)" t
);
pr " {\n";
- pr " if h.h == nil {\n";
+ pr "\tif h.h == nil {\n";
(match go_ret_error ret with
- | None -> pr " return closed_handle_error
(\"%s\")\n" name
- | Some v -> pr " return %s, closed_handle_error
(\"%s\")\n" v name
+ | None -> pr "\t\treturn
closed_handle_error(\"%s\")\n" name
+ | Some v -> pr "\t\treturn %s,
closed_handle_error(\"%s\")\n" v name
);
- pr " }\n";
+ pr "\t}\n";
pr "\n";
- pr " var c_err C.struct_error\n";
+ pr "\tvar c_err C.struct_error\n";
List.iter (
function
| Bool n ->
- pr " c_%s := C.bool (%s)\n" n n
+ pr "\tc_%s := C.bool(%s)\n" n n
| BytesIn (n, len) ->
- pr " c_%s := unsafe.Pointer (&%s[0])\n" n n;
- pr " c_%s := C.size_t (len (%s))\n" len n;
+ pr "\tc_%s := unsafe.Pointer(&%s[0])\n" n n;
+ pr "\tc_%s := C.size_t(len(%s))\n" len n;
| BytesOut (n, len) ->
- pr " c_%s := unsafe.Pointer (&%s[0])\n" n n;
- pr " c_%s := C.size_t (len (%s))\n" len n;
+ pr "\tc_%s := unsafe.Pointer(&%s[0])\n" n n;
+ pr "\tc_%s := C.size_t(len(%s))\n" len n;
| BytesPersistIn (n, len) ->
- pr " c_%s := %s.P\n" n n;
- pr " c_%s := C.size_t (%s.Size)\n" len n;
+ pr "\tc_%s := %s.P\n" n n;
+ pr "\tc_%s := C.size_t(%s.Size)\n" len n;
| BytesPersistOut (n, len) ->
- pr " c_%s := %s.P\n" n n;
- pr " c_%s := C.size_t (%s.Size)\n" len n;
+ pr "\tc_%s := %s.P\n" n n;
+ pr "\tc_%s := C.size_t(%s.Size)\n" len n;
| Closure { cbname } ->
- pr " var c_%s C.nbd_%s_callback\n" cbname cbname;
- pr " c_%s.callback =
(*[0]byte)(C._nbd_%s_callback_wrapper)\n"
+ pr "\tvar c_%s C.nbd_%s_callback\n" cbname cbname;
+ pr "\tc_%s.callback =
(*[0]byte)(C._nbd_%s_callback_wrapper)\n"
cbname cbname;
- pr " c_%s.free = (*[0]byte)(C._nbd_%s_callback_free)\n"
+ pr "\tc_%s.free = (*[0]byte)(C._nbd_%s_callback_free)\n"
cbname cbname;
- pr " %s_cbid := registerCallbackId(%s)\n" cbname cbname;
- pr " c_%s.user_data = C.alloc_cbid(C.long(%s_cbid))\n"
cbname cbname
+ pr "\t%s_cbid := registerCallbackId(%s)\n" cbname cbname;
+ pr "\tc_%s.user_data = C.alloc_cbid(C.long(%s_cbid))\n" cbname
cbname
| Enum (n, _) ->
- pr " c_%s := C.int (%s)\n" n n
+ pr "\tc_%s := C.int(%s)\n" n n
| Fd n ->
- pr " c_%s := C.int (%s)\n" n n
+ pr "\tc_%s := C.int(%s)\n" n n
| Flags (n, _) ->
- pr " c_%s := C.uint32_t (%s)\n" n n
+ pr "\tc_%s := C.uint32_t(%s)\n" n n
| Int n ->
- pr " c_%s := C.int (%s)\n" n n
+ pr "\tc_%s := C.int(%s)\n" n n
| Int64 n ->
- pr " c_%s := C.int64_t (%s)\n" n n
+ pr "\tc_%s := C.int64_t(%s)\n" n n
| Path n ->
- pr " c_%s := C.CString (%s)\n" n n;
- pr " defer C.free (unsafe.Pointer (c_%s))\n" n
+ pr "\tc_%s := C.CString(%s)\n" n n;
+ pr "\tdefer C.free(unsafe.Pointer(c_%s))\n" n
| SizeT n ->
- pr " c_%s := C.size_t (%s)\n" n n
+ pr "\tc_%s := C.size_t(%s)\n" n n
| SockAddrAndLen (n, len) ->
- pr " panic (\"SockAddrAndLen not supported\")\n";
- pr " var c_%s *C.struct_sockaddr\n" n;
- pr " var c_%s C.uint\n" len
+ pr "\tpanic(\"SockAddrAndLen not supported\")\n";
+ pr "\tvar c_%s *C.struct_sockaddr\n" n;
+ pr "\tvar c_%s C.uint\n" len
| String n ->
- pr " c_%s := C.CString (%s)\n" n n;
- pr " defer C.free (unsafe.Pointer (c_%s))\n" n
+ pr "\tc_%s := C.CString(%s)\n" n n;
+ pr "\tdefer C.free(unsafe.Pointer(c_%s))\n" n
| StringList n ->
- pr " c_%s := arg_string_list (%s)\n" n n;
- pr " defer free_string_list (c_%s)\n" n
+ pr "\tc_%s := arg_string_list(%s)\n" n n;
+ pr "\tdefer free_string_list(c_%s)\n" n
| UInt n ->
- pr " c_%s := C.uint (%s)\n" n n
+ pr "\tc_%s := C.uint(%s)\n" n n
| UInt32 n ->
- pr " c_%s := C.uint32_t (%s)\n" n n
+ pr "\tc_%s := C.uint32_t(%s)\n" n n
| UInt64 n ->
- pr " c_%s := C.uint64_t (%s)\n" n n
+ pr "\tc_%s := C.uint64_t(%s)\n" n n
| UIntPtr n ->
- pr " c_%s := C.uintptr_t (%s)\n" n n
+ pr "\tc_%s := C.uintptr_t(%s)\n" n n
) args;
if optargs <> [] then (
List.iter (
function
- | OClosure { cbname } -> pr " var c_%s
C.nbd_%s_callback\n"
+ | OClosure { cbname } -> pr "\tvar c_%s C.nbd_%s_callback\n"
cbname cbname
- | OFlags (n, _, _) -> pr " var c_%s C.uint32_t\n" n
+ | OFlags (n, _, _) -> pr "\tvar c_%s C.uint32_t\n" n
) optargs;
- pr " if optargs != nil {\n";
+ pr "\tif optargs != nil {\n";
List.iter (
fun optarg ->
- pr " if optargs.%sSet {\n" (go_name_of_optarg
optarg);
+ pr "\t\tif optargs.%sSet {\n" (go_name_of_optarg optarg);
(match optarg with
| OClosure { cbname } ->
- pr " c_%s.callback =
(*[0]byte)(C._nbd_%s_callback_wrapper)\n"
+ pr "\t\t\tc_%s.callback =
(*[0]byte)(C._nbd_%s_callback_wrapper)\n"
cbname cbname;
- pr " c_%s.free =
(*[0]byte)(C._nbd_%s_callback_free)\n"
+ pr "\t\t\tc_%s.free =
(*[0]byte)(C._nbd_%s_callback_free)\n"
cbname cbname;
- pr " %s_cbid :=
registerCallbackId(optargs.%s)\n"
+ pr "\t\t\t%s_cbid := registerCallbackId(optargs.%s)\n"
cbname (go_name_of_optarg optarg);
- pr " c_%s.user_data =
C.alloc_cbid(C.long(%s_cbid))\n"
+ pr "\t\t\tc_%s.user_data =
C.alloc_cbid(C.long(%s_cbid))\n"
cbname cbname
| OFlags (n, _, _) ->
- pr " c_%s = C.uint32_t (optargs.%s)\n"
+ pr "\t\t\tc_%s = C.uint32_t(optargs.%s)\n"
n (go_name_of_optarg optarg);
);
- pr " }\n";
+ pr "\t\t}\n";
) optargs;
- pr " }\n";
+ pr "\t}\n";
);
pr "\n";
- pr " ret := C._nbd_%s_wrapper (&c_err, h.h" name;
+ pr "\tret := C._nbd_%s_wrapper(&c_err, h.h" name;
List.iter (
function
| Bool n -> pr ", c_%s" n
@@ -347,57 +348,56 @@ let
* function has completed, in case all other references
* to the handle have disappeared and the finalizer would run.
*)
- pr " runtime.KeepAlive (h.h)\n";
+ pr "\truntime.KeepAlive(h.h)\n";
let errcode = go_ret_c_errcode ret in
(match errcode with
| None -> ()
| Some errcode ->
- pr " if ret == %s {\n" errcode;
- pr " err := get_error (\"%s\", c_err)\n" name;
- pr " C.free_error (&c_err)\n";
+ pr "\tif ret == %s {\n" errcode;
+ pr "\t\terr := get_error(\"%s\", c_err)\n" name;
+ pr "\t\tC.free_error(&c_err)\n";
(match go_ret_error ret with
- | None -> pr " return err\n"
- | Some v -> pr " return %s, err\n" v
+ | None -> pr "\t\treturn err\n"
+ | Some v -> pr "\t\treturn %s, err\n" v
);
- pr " }\n";
+ pr "\t}\n";
);
(match ret with
| RErr ->
- pr " return nil\n"
+ pr "\treturn nil\n"
| RBool ->
- pr " return int (ret) != 0, nil\n"
+ pr "\treturn int(ret) != 0, nil\n"
| RStaticString ->
- pr " /* ret is statically allocated, do not free it. */\n";
- pr " r := C.GoString (ret);\n";
- pr " return &r, nil\n"
+ pr "\t/* ret is statically allocated, do not free it. */\n";
+ pr "\tr := C.GoString(ret)\n";
+ pr "\treturn &r, nil\n"
| RFd ->
- pr " return int (ret), nil\n"
+ pr "\treturn int(ret), nil\n"
| RInt ->
- pr " return uint (ret), nil\n"
+ pr "\treturn uint(ret), nil\n"
| RInt64 ->
- pr " return uint64 (ret), nil\n"
+ pr "\treturn uint64(ret), nil\n"
| RCookie ->
- pr " return uint64 (ret), nil\n"
+ pr "\treturn uint64(ret), nil\n"
| RSizeT ->
- pr " return uint (ret), nil\n"
+ pr "\treturn uint(ret), nil\n"
| RString ->
- pr " r := C.GoString (ret)\n";
- pr " C.free (unsafe.Pointer (ret))\n";
- pr " return &r, nil\n"
+ pr "\tr := C.GoString(ret)\n";
+ pr "\tC.free(unsafe.Pointer(ret))\n";
+ pr "\treturn &r, nil\n"
| RUInt ->
- pr " return uint (ret), nil\n"
+ pr "\treturn uint(ret), nil\n"
| RUIntPtr ->
- pr " return uint (ret), nil\n"
+ pr "\treturn uint(ret), nil\n"
| RUInt64 ->
- pr " return uint64 (ret), nil\n"
+ pr "\treturn uint64(ret), nil\n"
| REnum { enum_prefix } ->
- pr " return %s (ret), nil\n" (camel_case enum_prefix)
+ pr "\treturn %s(ret), nil\n" (camel_case enum_prefix)
| RFlags { flag_prefix } ->
- pr " return %s (ret), nil\n" (camel_case flag_prefix)
+ pr "\treturn %s(ret), nil\n" (camel_case flag_prefix)
);
- pr "}\n";
- pr "\n"
+ pr "}\n"
let generate_golang_bindings_go () generate_header CStyle;
@@ -422,8 +422,8 @@ let
import \"C\"
import (
- \"runtime\"
- \"unsafe\"
+\t\"runtime\"
+\t\"unsafe\"
)
/* Enums. */
@@ -431,10 +431,11 @@ let
List.iter (
fun { enum_prefix; enums } ->
pr "type %s int\n" (camel_case enum_prefix);
+ pr "\n";
pr "const (\n";
List.iter (
fun (enum, v) ->
- pr " %s_%s = %s(%d)\n" enum_prefix enum (camel_case
enum_prefix) v
+ pr "\t%s_%s = %s(%d)\n" enum_prefix enum (camel_case
enum_prefix) v
) enums;
pr ")\n";
pr "\n"
@@ -448,13 +449,14 @@ let
let flag_type = camel_case flag_prefix in
let mask = ref 0 in
pr "type %s uint32\n" flag_type;
+ pr "\n";
pr "const (\n";
List.iter (
fun (flag, v) ->
- pr " %s_%s = %s(0x%02x)\n" flag_prefix flag flag_type v;
+ pr "\t%s_%s = %s(0x%02x)\n" flag_prefix flag flag_type v;
mask := !mask lor v
) flags;
- pr " %s_MASK = %s(0x%02x)\n" flag_prefix flag_type !mask;
+ pr "\t%s_MASK = %s(0x%02x)\n" flag_prefix flag_type !mask;
pr ")\n";
pr "\n"
) all_flags;
@@ -464,26 +466,26 @@ let
const (
";
List.iter (
- fun (n, v) -> pr " %s uint32 = %d\n" n v
+ fun (n, v) -> pr "\t%s uint32 = %d\n" n v
) constants;
List.iter (
fun (ns, ctxts) ->
let ns_upper = String.uppercase_ascii ns in
- pr " /* Meta-context namespace \"%s\" */\n" ns;
- pr " NAMESPACE_%s = \"%s:\"\n" ns_upper ns;
+ pr "\t/* Meta-context namespace \"%s\" */\n" ns;
+ pr "\tNAMESPACE_%s = \"%s:\"\n" ns_upper ns;
List.iter (
fun (ctxt, consts) ->
let ctxt_macro = String.uppercase_ascii (macro_name ctxt) in
- pr " CONTEXT_%s_%s = \"%s:%s\"\n" ns_upper
ctxt_macro ns ctxt;
+ pr "\tCONTEXT_%s_%s = \"%s:%s\"\n" ns_upper
ctxt_macro ns ctxt;
if consts <> [] then
- pr " /* Defined bits in \"%s:%s\" */\n" ns
ctxt;
+ pr "\t/* Defined bits in \"%s:%s\" */\n" ns
ctxt;
List.iter (fun (n, v) ->
- pr " %s uint32 = %d\n" n v
+ pr "\t%s uint32 = %d\n" n v
) consts
) ctxts;
) metadata_namespaces;
- pr ")\n\n";
+ pr ")\n";
(* Bindings. *)
List.iter print_binding handle_calls
@@ -510,21 +512,22 @@ let
/* Closures. */
func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
- ret := make([]uint32, int(count))
- addr := uintptr(unsafe.Pointer(entries))
- for i := 0; i < int(count); i++ {
- ptr := (*C.uint32_t)(unsafe.Pointer(addr))
- ret[i] = uint32(*ptr)
- addr += unsafe.Sizeof(*ptr)
- }
- return ret
+\tret := make([]uint32, int(count))
+\taddr := uintptr(unsafe.Pointer(entries))
+\tfor i := 0; i < int(count); i++ {
+\t\tptr := (*C.uint32_t)(unsafe.Pointer(addr))
+\t\tret[i] = uint32(*ptr)
+\t\taddr += unsafe.Sizeof(*ptr)
+\t}
+\treturn ret
}
";
List.iter (
fun { cbname; cbargs } ->
let uname = camel_case cbname in
- pr "type %sCallback func (" uname;
+ pr "\n";
+ pr "type %sCallback func(" uname;
let comma = ref false in
List.iter (
fun cbarg ->
@@ -551,7 +554,7 @@ let
pr ") int\n";
pr "\n";
pr "//export %s_callback\n" cbname;
- pr "func %s_callback (callbackid *C.long" cbname;
+ pr "func %s_callback(callbackid *C.long" cbname;
List.iter (
fun cbarg ->
pr ", ";
@@ -575,11 +578,11 @@ let
| CBArrayAndLen _ | CBMutable _ -> assert false
) cbargs;
pr ") C.int {\n";
- pr " callbackFunc := getCallbackId (int (*callbackid));\n";
- pr " callback, ok := callbackFunc.(%sCallback);\n" uname;
- pr " if !ok {\n";
- pr " panic (\"inappropriate callback
type\");\n";
- pr " }\n";
+ pr "\tcallbackFunc := getCallbackId(int(*callbackid))\n";
+ pr "\tcallback, ok := callbackFunc.(%sCallback)\n" uname;
+ pr "\tif !ok {\n";
+ pr "\t\tpanic(\"inappropriate callback type\")\n";
+ pr "\t}\n";
(* Deal with mutable int by creating a local variable
* and passing a pointer to it to the callback.
@@ -588,30 +591,30 @@ let
fun cbarg ->
match cbarg with
| CBMutable (Int n) ->
- pr " go_%s := int (*%s)\n" n n
+ pr "\tgo_%s := int(*%s)\n" n n
| _ -> ()
) cbargs;
- pr " ret := callback (";
+ pr "\tret := callback(";
let comma = ref false in
List.iter (
fun cbarg ->
if !comma then pr ", "; comma := true;
match cbarg with
| CBArrayAndLen (UInt32 n, count) ->
- pr "copy_uint32_array (%s, %s)" n count
+ pr "copy_uint32_array(%s, %s)" n count
| CBBytesIn (n, len) ->
- pr "C.GoBytes (%s, C.int (%s))" n len
+ pr "C.GoBytes(%s, C.int(%s))" n len
| CBInt n ->
- pr "int (%s)" n
+ pr "int(%s)" n
| CBUInt n ->
- pr "uint (%s)" n
+ pr "uint(%s)" n
| CBInt64 n ->
- pr "int64 (%s)" n
+ pr "int64(%s)" n
| CBString n ->
- pr "C.GoString (%s)" n
+ pr "C.GoString(%s)" n
| CBUInt64 n ->
- pr "uint64 (%s)" n
+ pr "uint64(%s)" n
| CBMutable (Int n) ->
pr "&go_%s" n
| CBArrayAndLen _ | CBMutable _ -> assert false
@@ -622,12 +625,11 @@ let
fun cbarg ->
match cbarg with
| CBMutable (Int n) ->
- pr " *%s = C.int (go_%s)\n" n n
+ pr "\t*%s = C.int(go_%s)\n" n n
| _ -> ()
) cbargs;
- pr " return C.int (ret);\n";
- pr "}\n";
- pr "\n"
+ pr "\treturn C.int(ret)\n";
+ pr "}\n"
) all_closures
let generate_golang_wrappers_go () --
2.41.0
Eric Blake
2023-Jul-26 17:50 UTC
[Libguestfs] [libnbd PATCH 7/7] golang: Enforce coding style during 'make check'
Now that I've finished tweaking the generator to output consistent Go
style, add a test that runs gofmt to flag places where we introduce
style regressions. As lining up columns in generated const() blocks
is trickier, for now I am making the test skip that by default (export
TEST_GOFMT_ALL=1 to see the difference).
A later patch may figure out how to do it in OCaml (two passes: one to
collect the maximum length of a name, the second to output columnar
data), or to include gofmt as part of the generation process (when
available), where a 'make dist' tarball will compile no matter what,
but only have correct formatting if the developer building the tarball
had gofmt installed.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
golang/Makefile.am | 2 +-
golang/codestyle-tests.sh | 45 +++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 1 deletion(-)
create mode 100755 golang/codestyle-tests.sh
diff --git a/golang/Makefile.am b/golang/Makefile.am
index fac65248..9201ed8e 100644
--- a/golang/Makefile.am
+++ b/golang/Makefile.am
@@ -98,7 +98,7 @@ TESTS_ENVIRONMENT = \
abs_top_srcdir=$(abs_top_srcdir) \
$(NULL)
LOG_COMPILER = $(top_builddir)/run
-TESTS = run-tests.sh
+TESTS = run-tests.sh codestyle-tests.sh
endif
diff --git a/golang/codestyle-tests.sh b/golang/codestyle-tests.sh
new file mode 100755
index 00000000..f4928fe5
--- /dev/null
+++ b/golang/codestyle-tests.sh
@@ -0,0 +1,45 @@
+#!/bin/bash -
+# nbd client library in userspace
+# Copyright Red Hat
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+. ../tests/functions.sh
+
+set -e
+set -x
+
+# Assume that 'gofmt' lives in the same place as 'go'
+GOFMT=${GOLANG}fmt
+
+requires $GOFMT --help
+
+rm -f codestyle-tests.out
+cleanup_fn rm -f codestyle-tests.out
+
+# Lining up generated = in const() in bindings.go is hard; to check
+# that file, export TEST_GOFMT_ALL=1 while running this test.
+if test x"$TEST_GOFMT_ALL" = x; then
+ exclude='-not -name bindings.go'
+else
+ exclude+fi
+
+$GOFMT -d $(find . -name "*.go" $exclude) > codestyle-tests.out
+if test -s codestyle-tests.out; then
+ echo 'FAIL: fix the following style errors' >&2
+ cat codestyle-tests.out >&2
+ exit 1
+fi
--
2.41.0
Laszlo Ersek
2023-Jul-27 11:23 UTC
[Libguestfs] [libnbd PATCH 4/7] Revert "generator/Go.ml: Simplify copy_uint32_array"
On 7/26/23 19:50, Eric Blake wrote:> This reverts commit 6725fa0e129f9a60d7b89707ef8604e0aeeeaf43, although > with a more modern style. > > Casting a C array to a Go slice just to benefit from potential > optimizations in Go's copy(), is rather complex to understand, > especially when we are still copying things (the main reason to treat > a C array as a Go slice is when avoiding a copy has a benefit). > Without a benchmark showing measurable difference in runtime speed, > and considering that network transit time probably dominates the time > spent on block status and its callback, it is not worth the > complexity. Furthermore, an upcoming patch wants to add a similar > helper function for converting between a list of C and Go structs, > where the copy() trick will not work; and having the two helpers look > alike is beneficial. > > Suggested-by: Laszlo Ersek <lersek at redhat.com>I've needed to dig a bit, but indeed, bullet (8) in: http://mid.mail-archive.com/0e4ff751-88d6-837b-15a5-6f6c370a2f09 at redhat.com https://listman.redhat.com/archives/libguestfs/2023-June/031736.html> CC: Nir Soffer <nsoffer at redhat.com> > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > generator/GoLang.ml | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > index 0aa83bdc..77dacadb 100644 > --- a/generator/GoLang.ml > +++ b/generator/GoLang.ml > @@ -509,12 +509,14 @@ let > > /* Closures. */ > > -func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 { > - ret := make([]uint32, int (count)) > - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices > - // TODO: Use unsafe.Slice() when we require Go 1.17. > - s := (*[1<<30]uint32)(unsafe.Pointer(entries))[:count:count] > - copy(ret, s) > +func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { > + ret := make([]uint32, int(count)) > + addr := uintptr(unsafe.Pointer(entries)) > + for i := 0; i < int(count); i++ { > + ptr := (*C.uint32_t)(unsafe.Pointer(addr)) > + ret[i] = uint32(*ptr) > + addr += unsafe.Sizeof(*ptr) > + } > return ret > } > ";This patch mixes four things: - whitespace changes (due to style modernization / gofmt, presumably), - reverting commit 6725fa0e129f, - changes proposed in my email, - functional changes on top of my email. The "func" line matches my proposal (OK), with additional whitespace updates (OK), but has nothing to do with reverting 6725fa0e129f, so calling the patch a "revert" is misleading. The initialization of "ret" undergoes a whitespace update (OK), but is neither a revert (6725fa0e129f did not change the initialization of "ret"), nor does it match my proposal. In my proposal, I had removed the "int" cast (or conversion) intentionally. Casting a C size_t to a Go int seems wrong. (IIRC I had verified the widths of the Go integer types from the Go documentation.) The initialization of "addr" matches my proposal, with some whitespace updates (OK), but is not a revert of 6725fa0e129f. The "for" statement is a revert, but does not match my proposal. My proposal made sure that the loop variable was a Go uint64, so that it could accommodate the C size_t. The only Go syntax I found suitable for that was to replace the ":=" embedded in the "for", with a separate definition/initialization of "i" (where I could spell out the uint64 type), and then to use the "for" variant that was effectively a "while" (using C terminology): var i uint64 = 0 for i < uint64(count) { ... i++ } Here we cast the C size_t to uint64, which is OK. A different approach to the same end, preserving the ":=" syntax in "for", could be: for i := uint64(0); i < uint64(count); i++ { ... } This keeps the "count" cast safe, and it also forces -- I think? -- "i" to have type "uint64", by casting "0" to "uint64" in the ":=" operation. Anyway, I was super careful about those nuances when I wrote my proposal, so I'd like to stick with them. I suggest that: - we not call this patch a "revert", just update the code incrementally; perhaps reference commit 6725fa0e129f in the commit message - we stick with the exact code I proposed (unless there are specific problems with it), applying whitespace updates on top that are now required by gofmt. Thanks, Laszlo