Qemu has a couple of documented meta-context definitions[1]; we might as well expose constants for these namespaces. "qemu:dirty-bitmap:NAME" is a set of namespaces for any arbitrary dirty bitmap name; we can't define constants for every bitspace name, but it is possible to do NBD_OPT_LIST_META_CONTEXT on "qemu:dirty-bitmap:" to see which dirty bitmaps are available. When a dirty bitmap is negotiated, only one bit is defined (an extent is dirty or not). The presence of '-' and ':' in the context name beyond the namespace requires a new helper function. "qemu:allocation-depth" returns an integer rather than a bitmap (0 for unmapped, 1 for current image, 2 and beyond for number of files in the backing chain before the data was supplied), so we can't really define any constants for interpreting its values. [1] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/interop/nbd.txt For libnbd.h, the generated diff is: | --- include/libnbd.h.bak 2023-07-26 11:01:45.401328604 -0500 | +++ include/libnbd.h 2023-07-26 11:59:38.289021067 -0500 | @@ -1083,6 +1083,16 @@ | #define LIBNBD_STATE_HOLE 1 | #define LIBNBD_STATE_ZERO 2 | | +/* "qemu" namespace */ | +#define LIBNBD_NAMESPACE_QEMU "qemu:" | + | +/* "qemu" namespace contexts */ | +#define LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP "qemu:dirty-bitmap:" | +#define LIBNBD_CONTEXT_QEMU_ALLOCATION_DEPTH "qemu:allocation-depth" | + | +/* "qemu:dirty-bitmap:" context related constants */ | +#define LIBNBD_STATE_DIRTY 1 | + | #ifdef __cplusplus | } | #endif Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/utils.mli | 1 + generator/API.ml | 9 ++++++--- generator/C.ml | 22 +++++++++++++++------- generator/GoLang.ml | 3 ++- generator/OCaml.ml | 4 ++-- generator/Python.ml | 3 ++- generator/utils.ml | 5 +++++ interop/dirty-bitmap.c | 6 ++++-- 8 files changed, 37 insertions(+), 16 deletions(-) diff --git a/generator/utils.mli b/generator/utils.mli index 773e705f..c4d47a34 100644 --- a/generator/utils.mli +++ b/generator/utils.mli @@ -46,6 +46,7 @@ val val cspan : string -> string -> int val quote : string -> string val spaces : int -> string +val macro_name : string -> string val files_equal : string -> string -> bool val generate_header : diff --git a/generator/API.ml b/generator/API.ml index 02c1260d..72c81657 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -3919,9 +3919,12 @@ let constants = let metadata_namespaces = [ "base", [ "allocation", [ - "STATE_HOLE", 1 lsl 0; - "STATE_ZERO", 1 lsl 1; - ] ]; + "STATE_HOLE", 1 lsl 0; + "STATE_ZERO", 1 lsl 1; + ] ]; + "qemu", [ "dirty-bitmap:", [ "STATE_DIRTY", 1 lsl 0; ]; + "allocation-depth", []; + ]; ] let pod_of_link = function diff --git a/generator/C.ml b/generator/C.ml index f772117c..a2670f70 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -373,13 +373,18 @@ let pr "#define LIBNBD_HAVE_NBD_%s 1\n" name_upper; pr "\n" -let print_ns_ctxt ns ns_upper ctxt consts = - let ctxt_upper = String.uppercase_ascii ctxt in +let print_ns_ctxt ns ns_upper ctxt = + let ctxt_macro = macro_name ctxt in + let ctxt_upper = String.uppercase_ascii ctxt_macro in pr "#define LIBNBD_CONTEXT_%s_%s \"%s:%s\"\n" - ns_upper ctxt_upper ns ctxt; - pr "\n"; - pr "/* \"%s:%s\" context related constants */\n" ns ctxt; - List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts + ns_upper ctxt_upper ns ctxt + +let print_ns_ctxt_bits ns ctxt consts = + if consts <> [] then ( + pr "\n"; + pr "/* \"%s:%s\" context related constants */\n" ns ctxt; + List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts + ) let print_ns ns ctxts = let ns_upper = String.uppercase_ascii ns in @@ -388,7 +393,10 @@ let pr "\n"; pr "/* \"%s\" namespace contexts */\n" ns; List.iter ( - fun (ctxt, consts) -> print_ns_ctxt ns ns_upper ctxt consts + fun (ctxt, _) -> print_ns_ctxt ns ns_upper ctxt + ) ctxts; + List.iter ( + fun (ctxt, consts) -> print_ns_ctxt_bits ns ctxt consts ) ctxts; pr "\n" diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 50ed7226..7a7e7f4b 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -472,7 +472,8 @@ let pr " namespace_%s = \"%s:\"\n" ns ns; List.iter ( fun (ctxt, consts) -> - pr " context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; + let ctxt_macro = macro_name ctxt in + pr " context_%s_%s = \"%s:%s\"\n" ns ctxt_macro ns ctxt; List.iter (fun (n, v) -> pr " %s uint32 = %d\n" n v ) consts diff --git a/generator/OCaml.ml b/generator/OCaml.ml index edb81f25..efc1af22 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -183,7 +183,7 @@ type pr "val namespace_%s : string\n" ns; List.iter ( fun (ctxt, consts) -> - pr "val context_%s_%s : string\n" ns ctxt; + pr "val context_%s_%s : string\n" ns (macro_name ctxt); List.iter (fun (n, _) -> pr "val %s : int32\n" (String.lowercase_ascii n) ) consts @@ -315,7 +315,7 @@ let pr "let namespace_%s = \"%s:\"\n" ns ns; List.iter ( fun (ctxt, consts) -> - pr "let context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; + pr "let context_%s_%s = \"%s:%s\"\n" ns (macro_name ctxt) ns ctxt; List.iter (fun (n, i) -> pr "let %s = %d_l\n" (String.lowercase_ascii n) i ) consts diff --git a/generator/Python.ml b/generator/Python.ml index c81878de..beaeaa4c 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -745,7 +745,8 @@ let pr "NAMESPACE_%s = \"%s:\"\n" ns_upper ns; List.iter ( fun (ctxt, consts) -> - let ctxt_upper = String.uppercase_ascii ctxt in + let ctxt_macro = macro_name ctxt in + let ctxt_upper = String.uppercase_ascii ctxt_macro in pr "%s = \"%s:%s\"\n" (sprintf "CONTEXT_%s_%s" ns_upper ctxt_upper) ns ctxt; List.iter (fun (n, i) -> pr "%s = %d\n" n i) consts diff --git a/generator/utils.ml b/generator/utils.ml index e0c67d20..61cce877 100644 --- a/generator/utils.ml +++ b/generator/utils.ml @@ -151,6 +151,11 @@ let let spaces n = String.make n ' ' +(* Convert s to macro name by changing '-' to '_' and eliding ':'. *) +let macro_name s = + let underscore = Str.global_replace (Str.regexp_string "-") "_" s in + Str.global_replace (Str.regexp ":") "" underscore + (* Save the current output channel and replace it with a temporary buffer while * running ‘code’. Return the buffer. *) diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 05f6e9db..16faaed9 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -127,8 +127,10 @@ main (int argc, char *argv[]) nbd_extent_callback extent_callback = { .callback = cb, .user_data = &data }; char c; - if (argc < 3) { - fprintf (stderr, "%s bitmap qemu-nbd [args ...]\n", argv[0]); + if (argc < 3 || strncmp (argv[1], LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP, + strlen (LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP)) != 0) { + fprintf (stderr, "%s qemu:dirty-bitmap:name qemu-nbd [args ...]\n", + argv[0]); exit (EXIT_FAILURE); } bitmap = argv[1]; -- 2.41.0

On Thu, Jul 27, 2023 at 01:41:03PM +0200, Laszlo Ersek wrote: I'm of the opinion that if any project declares their own namespace of extensions, and then documents that declaration in the upstream NBD spec (which qemu has done: [1]), then libnbd might as well try and make it easier to probe for the existence of that extension. [1] https://github.com/NetworkBlockDevice/nbd/blob/58b356bd19e63/doc/proto.md... That provides interoperability tests of what libnbd does when talking to various servers (some of the tests are repeated across nbdkit, qemu, and nbd-server; some are specific to features of a given server). Altering the test to actually use the constants defined by this patch ensures that we have API stability for those constants. Yes, we could indeed add at least 0 and 1 as constants for qemu:allocation-depth, if we wanted. I'm not sure how to go about documenting them, though; the point is that base:allocation uses flags as a bitmap, but qemu:allocation-depth uses flags as an integer. Correct. This patch is independent of the Go changes, but noticed because the way we were outputting a list of meta-contexts when the list was only 1 element long was odd. Making the list more than one element long made it easier to test that all the loops are working correctly (or, as shown by this patch, that reordering the loops makes output more sensible). I'll wait to see what Rich suggests. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

Go insists that a module's members are not public unless they start with a capital letter. Our attempt at defining constants 'namespace_base' and 'context_base_allocation' were thus only visible to our in-package tests, and not for external clients. Although this patch does not completely address gofmt complaints, adding some strategic grouping comments inside the const() block has an additional benefit of reducing the amount of whitespace that gofmt wants to inject into unrelated lines. The change to generated golang/bindigs.go is: | --- golang/bindings.go.bak 2023-07-26 12:06:47.618324437 -0500 | +++ golang/bindings.go 2023-07-26 12:06:55.521385295 -0500 | @@ -111,14 +111,18 @@ | READ_DATA uint32 = 1 | READ_HOLE uint32 = 2 | READ_ERROR uint32 = 3 | - namespace_base = "base:" | - context_base_allocation = "base:allocation" | + /* Meta-context namespace "base" */ | + NAMESPACE_BASE = "base:" | + CONTEXT_BASE_ALLOCATION = "base:allocation" | + /* Defined bits in "base:allocation" */ | STATE_HOLE uint32 = 1 | STATE_ZERO uint32 = 2 | - namespace_qemu = "qemu:" | - context_qemu_dirty_bitmap = "qemu:dirty-bitmap:" | + /* Meta-context namespace "qemu" */ | + NAMESPACE_QEMU = "qemu:" | + CONTEXT_QEMU_DIRTY_BITMAP = "qemu:dirty-bitmap:" | + /* Defined bits in "qemu:dirty-bitmap:" */ | STATE_DIRTY uint32 = 1 | - context_qemu_allocation_depth = "qemu:allocation-depth" | + CONTEXT_QEMU_ALLOCATION_DEPTH = "qemu:allocation-depth" | ) | | /* SetDebug: set or clear the debug flag */ Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- RFC: Do we want MixedCase rather than ALL_CAPS names? If so, this patch needs to be rethought a bit, as it would impact API for existing public constants as well as the ones changed here. But we don't promise API stability for Go. --- generator/GoLang.ml | 10 +++++--- golang/libnbd_230_opt_info_test.go | 24 +++++++++---------- golang/libnbd_240_opt_list_meta_test.go | 14 +++++------ .../libnbd_245_opt_list_meta_queries_test.go | 4 ++-- golang/libnbd_250_opt_set_meta_test.go | 20 ++++++++-------- .../libnbd_255_opt_set_meta_queries_test.go | 12 +++++----- golang/libnbd_460_block_status_test.go | 4 ++-- 7 files changed, 46 insertions(+), 42 deletions(-) diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 7a7e7f4b..82d73ed6 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -469,11 +469,15 @@ let ) constants; List.iter ( fun (ns, ctxts) -> - pr " namespace_%s = \"%s:\"\n" ns ns; + let ns_upper = String.uppercase_ascii ns in + pr " /* Meta-context namespace \"%s\" */\n" ns; + pr " NAMESPACE_%s = \"%s:\"\n" ns_upper ns; List.iter ( fun (ctxt, consts) -> - let ctxt_macro = macro_name ctxt in - pr " context_%s_%s = \"%s:%s\"\n" ns ctxt_macro ns ctxt; + let ctxt_macro = String.uppercase_ascii (macro_name ctxt) in + pr " CONTEXT_%s_%s = \"%s:%s\"\n" ns_upper ctxt_macro ns ctxt; + if consts <> [] then + pr " /* Defined bits in \"%s:%s\" */\n" ns ctxt; List.iter (fun (n, v) -> pr " %s uint32 = %d\n" n v ) consts diff --git a/golang/libnbd_230_opt_info_test.go b/golang/libnbd_230_opt_info_test.go index cd00be9e..cc7c2d91 100644 --- a/golang/libnbd_230_opt_info_test.go +++ b/golang/libnbd_230_opt_info_test.go @@ -45,7 +45,7 @@ func Test230OptInfo(t *testing.T) { t.Fatalf("could not connect: %s", err) } - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not add meta context: %s", err) } @@ -59,7 +59,7 @@ func Test230OptInfo(t *testing.T) { if err == nil { t.Fatalf("expected error") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -83,7 +83,7 @@ func Test230OptInfo(t *testing.T) { if !ro { t.Fatalf("unexpected readonly") } - meta, err := h.CanMetaContext(context_base_allocation) + meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta failed unexpectedly: %s", err) } @@ -104,7 +104,7 @@ func Test230OptInfo(t *testing.T) { if err == nil { t.Fatalf("expected error") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -151,7 +151,7 @@ func Test230OptInfo(t *testing.T) { if ro { t.Fatalf("unexpected readonly") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -177,7 +177,7 @@ func Test230OptInfo(t *testing.T) { if err == nil { t.Fatalf("expected error") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -205,7 +205,7 @@ func Test230OptInfo(t *testing.T) { if !ro { t.Fatalf("unexpected readonly") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta failed unexpectedly: %s", err) } @@ -236,7 +236,7 @@ func Test230OptInfo(t *testing.T) { if size != 4 { t.Fatalf("unexpected size") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta failed unexpectedly: %s", err) } @@ -270,7 +270,7 @@ func Test230OptInfo(t *testing.T) { t.Fatalf("could not add meta context: %s", err) } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -278,7 +278,7 @@ func Test230OptInfo(t *testing.T) { if err != nil { t.Fatalf("opt_info failed unexpectedly: %s", err) } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta failed unexpectedly: %s", err) } @@ -290,7 +290,7 @@ func Test230OptInfo(t *testing.T) { t.Fatalf("set request meta context failed unexpectedly: %s", err) } /* Adding to the request list now won't matter */ - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not add meta context: %s", err) } @@ -298,7 +298,7 @@ func Test230OptInfo(t *testing.T) { if err != nil { t.Fatalf("opt_go failed unexpectedly: %s", err) } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta failed unexpectedly: %s", err) } diff --git a/golang/libnbd_240_opt_list_meta_test.go b/golang/libnbd_240_opt_list_meta_test.go index 0235fe3f..011b5704 100644 --- a/golang/libnbd_240_opt_list_meta_test.go +++ b/golang/libnbd_240_opt_list_meta_test.go @@ -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 @@ -92,7 +92,7 @@ func Test240OptListMeta(t *testing.T) { /* Third pass: specific query should have one match. */ list_count = 0 list_seen = false - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not request add_meta_context: %s", err) } @@ -107,7 +107,7 @@ func Test240OptListMeta(t *testing.T) { if err != nil { t.Fatalf("could not request get_meta_context: %s", err) } - if *tmp != context_base_allocation { + if *tmp != CONTEXT_BASE_ALLOCATION { t.Fatalf("wrong result of get_meta_context: %s", *tmp) } r, err = h.OptListMetaContext(func(name string) int { @@ -129,7 +129,7 @@ func Test240OptListMeta(t *testing.T) { if err == nil { t.Fatalf("expected error") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -144,7 +144,7 @@ func Test240OptListMeta(t *testing.T) { if size != 1048576 { t.Fatalf("get_size gave wrong size") } - meta, err := h.CanMetaContext(context_base_allocation) + meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta_context failed unexpectedly: %s", err) } @@ -175,7 +175,7 @@ func Test240OptListMeta(t *testing.T) { if size != 1048576 { t.Fatalf("get_size gave wrong size") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("can_meta_context failed unexpectedly: %s", err) } @@ -187,7 +187,7 @@ func Test240OptListMeta(t *testing.T) { /* Final pass: "base:" query should get at least "base:allocation" */ list_count = 0 list_seen = false - err = h.AddMetaContext("base:") + err = h.AddMetaContext(NAMESPACE_BASE) if err != nil { t.Fatalf("could not request add_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 fc1ab60c..3ae2d854 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 @@ -97,7 +97,7 @@ 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) }) diff --git a/golang/libnbd_250_opt_set_meta_test.go b/golang/libnbd_250_opt_set_meta_test.go index 6d27fa22..4263ede5 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 @@ -67,18 +67,18 @@ func Test250OptSetMeta(t *testing.T) { if sr { t.Fatalf("unexpected structured replies state") } - meta, err := h.CanMetaContext(context_base_allocation) + meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } if meta { t.Fatalf("unexpected can meta context state") } - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not request add_meta_context: %s", err) } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -109,7 +109,7 @@ func Test250OptSetMeta(t *testing.T) { if !sr { t.Fatalf("unexpected structured replies state") } - _, err = h.CanMetaContext(context_base_allocation) + _, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err == nil { t.Fatalf("expected error") } @@ -151,7 +151,7 @@ func Test250OptSetMeta(t *testing.T) { if r != set_count || r != 0 || set_seen { t.Fatalf("unexpected set_count after opt_set_meta_context") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } @@ -166,7 +166,7 @@ func Test250OptSetMeta(t *testing.T) { if err != nil { t.Fatalf("could not request add_meta_context: %s", err) } - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not request add_meta_context: %s", err) } @@ -183,7 +183,7 @@ func Test250OptSetMeta(t *testing.T) { if r != 1 || r != set_count || !set_seen { t.Fatalf("unexpected set_count after opt_set_meta_context") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } @@ -204,7 +204,7 @@ func Test250OptSetMeta(t *testing.T) { if err != nil { t.Fatalf("could not request opt_go: %s", err) } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } @@ -224,7 +224,7 @@ func Test250OptSetMeta(t *testing.T) { if set_count != 0 || set_seen { t.Fatalf("unexpected set_count after opt_set_meta_context") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } diff --git a/golang/libnbd_255_opt_set_meta_queries_test.go b/golang/libnbd_255_opt_set_meta_queries_test.go index cca25ae7..232560bc 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 @@ -74,7 +74,7 @@ func(name string) int { */ setq_count = 0 setq_seen = false - err = h.AddMetaContext(context_base_allocation) + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not request add_meta_context: %s", err) } @@ -87,7 +87,7 @@ func(name string) int { if r != setq_count || r != 0 || setq_seen { t.Fatalf("unexpected set_count after opt_set_meta_context_queries") } - meta, err := h.CanMetaContext(context_base_allocation) + meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } @@ -99,7 +99,7 @@ func(name string) int { setq_count = 0 setq_seen = false r, err = h.OptSetMetaContextQueries([]string{ - "x-nosuch:context", context_base_allocation}, + "x-nosuch:context", CONTEXT_BASE_ALLOCATION}, func(name string) int { return setmetaqf(42, name) }) @@ -109,7 +109,7 @@ func(name string) int { if r != 1 || r != setq_count || !setq_seen { t.Fatalf("unexpected set_count after opt_set_meta_context_queries") } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } @@ -126,7 +126,7 @@ func(name string) int { if err != nil { t.Fatalf("could not request opt_go: %s", err) } - meta, err = h.CanMetaContext(context_base_allocation) + meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("could not check can meta context: %s", err) } diff --git a/golang/libnbd_460_block_status_test.go b/golang/libnbd_460_block_status_test.go index dca0d1b8..d2d11e9c 100644 --- a/golang/libnbd_460_block_status_test.go +++ b/golang/libnbd_460_block_status_test.go @@ -31,7 +31,7 @@ func mcf(metacontext string, offset uint64, e []uint32, error *int) int { if *error != 0 { panic("expected *error == 0") } - if metacontext == "base:allocation" { + if metacontext == CONTEXT_BASE_ALLOCATION { entries = e } return 0 @@ -69,7 +69,7 @@ func Test460BlockStatus(t *testing.T) { } defer h.Close() - err = h.AddMetaContext("base:allocation") + err = h.AddMetaContext(CONTEXT_BASE_ALLOCATION) if err != nil { t.Fatalf("%s", err) } -- 2.41.0

On Wed, Jul 26, 2023 at 12:29:45PM -0500, Eric Blake wrote: I don't greatly care either way, unless someone who uses golang expresses an opinion. Acked-by: Richard W.M. Jones <rjones(a)redhat.com&gt; (upstream in commit 14612ca0) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On 7/26/23 19:29, Eric Blake wrote: Nice! Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;

Qemu has a couple of documented meta-context definitions[1]; we might as well expose constants for these namespaces. "qemu:dirty-bitmap:NAME" is a set of namespaces for any arbitrary dirty bitmap name; we can't define constants for every bitspace name, but it is possible to do NBD_OPT_LIST_META_CONTEXT on "qemu:dirty-bitmap:" to see which dirty bitmaps are available. When a dirty bitmap is negotiated, only one bit is defined (an extent is dirty or not). The presence of '-' and ':' in the context name beyond the namespace requires a new helper function. "qemu:allocation-depth" returns an integer rather than a bitmap (0 for unmapped, 1 for current image, 2 and beyond for number of files in the backing chain before the data was supplied), so we can't really define any constants for interpreting its values. [1] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/interop/nbd.txt For libnbd.h, the generated diff is: | --- include/libnbd.h.bak 2023-07-26 11:01:45.401328604 -0500 | +++ include/libnbd.h 2023-07-26 11:59:38.289021067 -0500 | @@ -1083,6 +1083,16 @@ | #define LIBNBD_STATE_HOLE 1 | #define LIBNBD_STATE_ZERO 2 | | +/* "qemu" namespace */ | +#define LIBNBD_NAMESPACE_QEMU "qemu:" | + | +/* "qemu" namespace contexts */ | +#define LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP "qemu:dirty-bitmap:" | +#define LIBNBD_CONTEXT_QEMU_ALLOCATION_DEPTH "qemu:allocation-depth" | + | +/* "qemu:dirty-bitmap:" context related constants */ | +#define LIBNBD_STATE_DIRTY 1 | + | #ifdef __cplusplus | } | #endif Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/utils.mli | 1 + generator/API.ml | 9 ++++++--- generator/C.ml | 22 +++++++++++++++------- generator/GoLang.ml | 3 ++- generator/OCaml.ml | 4 ++-- generator/Python.ml | 3 ++- generator/utils.ml | 5 +++++ interop/dirty-bitmap.c | 6 ++++-- 8 files changed, 37 insertions(+), 16 deletions(-) diff --git a/generator/utils.mli b/generator/utils.mli index 773e705f..c4d47a34 100644 --- a/generator/utils.mli +++ b/generator/utils.mli @@ -46,6 +46,7 @@ val val cspan : string -> string -> int val quote : string -> string val spaces : int -> string +val macro_name : string -> string val files_equal : string -> string -> bool val generate_header : diff --git a/generator/API.ml b/generator/API.ml index 02c1260d..72c81657 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -3919,9 +3919,12 @@ let constants = let metadata_namespaces = [ "base", [ "allocation", [ - "STATE_HOLE", 1 lsl 0; - "STATE_ZERO", 1 lsl 1; - ] ]; + "STATE_HOLE", 1 lsl 0; + "STATE_ZERO", 1 lsl 1; + ] ]; + "qemu", [ "dirty-bitmap:", [ "STATE_DIRTY", 1 lsl 0; ]; + "allocation-depth", []; + ]; ] let pod_of_link = function diff --git a/generator/C.ml b/generator/C.ml index f772117c..a2670f70 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -373,13 +373,18 @@ let pr "#define LIBNBD_HAVE_NBD_%s 1\n" name_upper; pr "\n" -let print_ns_ctxt ns ns_upper ctxt consts = - let ctxt_upper = String.uppercase_ascii ctxt in +let print_ns_ctxt ns ns_upper ctxt = + let ctxt_macro = macro_name ctxt in + let ctxt_upper = String.uppercase_ascii ctxt_macro in pr "#define LIBNBD_CONTEXT_%s_%s \"%s:%s\"\n" - ns_upper ctxt_upper ns ctxt; - pr "\n"; - pr "/* \"%s:%s\" context related constants */\n" ns ctxt; - List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts + ns_upper ctxt_upper ns ctxt + +let print_ns_ctxt_bits ns ctxt consts = + if consts <> [] then ( + pr "\n"; + pr "/* \"%s:%s\" context related constants */\n" ns ctxt; + List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) consts + ) let print_ns ns ctxts = let ns_upper = String.uppercase_ascii ns in @@ -388,7 +393,10 @@ let pr "\n"; pr "/* \"%s\" namespace contexts */\n" ns; List.iter ( - fun (ctxt, consts) -> print_ns_ctxt ns ns_upper ctxt consts + fun (ctxt, _) -> print_ns_ctxt ns ns_upper ctxt + ) ctxts; + List.iter ( + fun (ctxt, consts) -> print_ns_ctxt_bits ns ctxt consts ) ctxts; pr "\n" diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 50ed7226..7a7e7f4b 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -472,7 +472,8 @@ let pr " namespace_%s = \"%s:\"\n" ns ns; List.iter ( fun (ctxt, consts) -> - pr " context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; + let ctxt_macro = macro_name ctxt in + pr " context_%s_%s = \"%s:%s\"\n" ns ctxt_macro ns ctxt; List.iter (fun (n, v) -> pr " %s uint32 = %d\n" n v ) consts diff --git a/generator/OCaml.ml b/generator/OCaml.ml index edb81f25..efc1af22 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -183,7 +183,7 @@ type pr "val namespace_%s : string\n" ns; List.iter ( fun (ctxt, consts) -> - pr "val context_%s_%s : string\n" ns ctxt; + pr "val context_%s_%s : string\n" ns (macro_name ctxt); List.iter (fun (n, _) -> pr "val %s : int32\n" (String.lowercase_ascii n) ) consts @@ -315,7 +315,7 @@ let pr "let namespace_%s = \"%s:\"\n" ns ns; List.iter ( fun (ctxt, consts) -> - pr "let context_%s_%s = \"%s:%s\"\n" ns ctxt ns ctxt; + pr "let context_%s_%s = \"%s:%s\"\n" ns (macro_name ctxt) ns ctxt; List.iter (fun (n, i) -> pr "let %s = %d_l\n" (String.lowercase_ascii n) i ) consts diff --git a/generator/Python.ml b/generator/Python.ml index c81878de..beaeaa4c 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -745,7 +745,8 @@ let pr "NAMESPACE_%s = \"%s:\"\n" ns_upper ns; List.iter ( fun (ctxt, consts) -> - let ctxt_upper = String.uppercase_ascii ctxt in + let ctxt_macro = macro_name ctxt in + let ctxt_upper = String.uppercase_ascii ctxt_macro in pr "%s = \"%s:%s\"\n" (sprintf "CONTEXT_%s_%s" ns_upper ctxt_upper) ns ctxt; List.iter (fun (n, i) -> pr "%s = %d\n" n i) consts diff --git a/generator/utils.ml b/generator/utils.ml index e0c67d20..61cce877 100644 --- a/generator/utils.ml +++ b/generator/utils.ml @@ -151,6 +151,11 @@ let let spaces n = String.make n ' ' +(* Convert s to macro name by changing '-' to '_' and eliding ':'. *) +let macro_name s = + let underscore = Str.global_replace (Str.regexp_string "-") "_" s in + Str.global_replace (Str.regexp ":") "" underscore + (* Save the current output channel and replace it with a temporary buffer while * running ‘code’. Return the buffer. *) diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 05f6e9db..16faaed9 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -127,8 +127,10 @@ main (int argc, char *argv[]) nbd_extent_callback extent_callback = { .callback = cb, .user_data = &data }; char c; - if (argc < 3) { - fprintf (stderr, "%s bitmap qemu-nbd [args ...]\n", argv[0]); + if (argc < 3 || strncmp (argv[1], LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP, + strlen (LIBNBD_CONTEXT_QEMU_DIRTY_BITMAP)) != 0) { + fprintf (stderr, "%s qemu:dirty-bitmap:name qemu-nbd [args ...]\n", + argv[0]); exit (EXIT_FAILURE); } bitmap = argv[1]; base-commit: 5c2fc3cc7e14146d000b65b191e70d9a0585a395 -- 2.41.0

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(a)redhat.com&gt; --- 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/ge... - */ + * 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/ge... + */ } 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

On Thu, Jul 27, 2023 at 01:52:00PM +0200, Laszlo Ersek wrote: That's gofmt for you. I was surprised by it too. Equally surprising, gofmt (at least from golang-bin-1.20.6-1.fc38) does NOT remove the spaces from that format, nor does it complain that the () are spurious. Of course, future gofmt may change style again, but now we have a regression test in place to catch that. And we may be faced with decisions down the road on how to pacify cases where gofmt itself changes styles from one release of the language to the next (if that happens, that would be a strong argument that we should remove gofmt from a gating 'make check' test, and instead incorporate it as an optional 'make dist' step - if we reach a point where there is no longer "the" canonical formatting, we shouls still favor tarballs created with "a" canoncial format). For now, I'm happy to squash in: diff --git i/golang/libnbd_620_stats_test.go w/golang/libnbd_620_stats_test.go index 63667160..6cc3cdc1 100644 --- i/golang/libnbd_620_stats_test.go +++ w/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") } } as well as amending the commit message to mention the manual touchup needed to work around the (in our opinion, misguided) advice from Go. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

On Thu, Jul 27, 2023 at 09:50:01AM -0500, Eric Blake wrote: snip IMHO that is defeating the point of using 'go fmt'. The value of 'go fmt' is that developers & projects no longer need to endlessly debate style. There is a consistent format across every project in the world that is written in Go. The value of this consistency outweighs the negatives of the few cases where formatting isn't the preference of individual developers. People will usually have their editors set to auto run 'go fmt' on any '.go' file, and telling them to manually alter certain style rules the project didn't like is an unecessary burden. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Wed, Jul 26, 2023 at 12:50:03PM -0500, Eric Blake wrote: Touching up manually isn't a good idea if you want to enforce 'go fmt' compliance, as you'll want contributors to be able to set their editors to automatically run 'go fmt' upon save and submit as is. libvirt-ci provides a container image for running & reporting go fmt violations. Since you're using lcitool manifest, just make this change $ git diff ci/manifest.yml diff --git a/ci/manifest.yml b/ci/manifest.yml index f7b1daf..a5f3e66 100644 --- a/ci/manifest.yml +++ b/ci/manifest.yml @@ -6,6 +6,7 @@ gitlab: project: libnbd jobs: check-dco: false + go-fmt: true targets: alpine-315: x86_64 and re-generate, and it'll enforce style on *.go tree-wide. libvirt-ci manifest also supports 'cargo-fmt' for rust, 'black' for python, and 'clang-format' for C too, enabled in the same way. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Thu, Jul 27, 2023 at 11:07:07AM -0500, Eric Blake wrote: Yep, only committed files. You can do both - in libvirt we have meson test check style, but in CI we have style checks split out into a separate job, as it makes it nicer when browsing CI failures to separate reporting of functional failures from style violations. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

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(a)redhat.com&gt; --- 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

On Thu, Jul 27, 2023 at 01:54:25PM +0200, Laszlo Ersek wrote: Some automation involved (emacs macros are nice), but yeah, it's grunt-work. Agreed. Maybe a better approach would be figuring out an optional argument to pr to insist on TAB instead of space indent? Or, as mentioned elsewhere, not bothering with TAB at all in OCaml but instead figuring out how to run gofmt itself as part of the generation pipeline. Yep, I've already encountered a number of conflicts as I shuffle around patches, trying to pick out which changes are more than just whitespace (such as the RBool change). Not terribly difficult, but back in the grunt-work category. I tried: + pr "\t" ^ "/* %s field is ignored unless %sSet == true. */\n" fname fname; File "GoLang.ml", line 186, characters 8-15: 186 | pr "\t" ^ "/* %s field is ignored unless %sSet == true. */\n" ^^^^^^^ Error: This expression has type unit but an expression was expected of type string and: File "GoLang.ml", line 186, characters 11-71: 186 | pr ("\t" ^ "/* %s field is ignored unless %sSet == true. */\n") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error: This expression has type string but an expression was expected of type ('weak2 -> 'weak3 -> 'weak4, unit, string, unit) format4 = ('weak2 -> 'weak3 -> 'weak4, unit, string, string, string, unit) CamlinternalFormatBasics.format6 so those are non-starters. We can get more verbose with: + pr "\t"; + pr "/* %s field is ignored unless %sSet == true. */\n" fname fname; but that may not scale as nicely. Go's use of TAB does not specify what tabstop it prefers; our .editorconfig suggests using a tabstop of 4 (instead of the usual 8) when reading a .go file for less visual distraction, and which matches with GoLang.ml currently using 4-space indentation. Rich, do you have any ideas on any better approach to take here? I also debated about splitting this patch into two (yes, more gruntwork): one to address space before '(', and the other to address leading indentation. Lines like this would be touched by both patches if I split. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

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(a)redhat.com&gt; --- 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

On Thu, Jul 27, 2023 at 01:23:24PM +0200, Laszlo Ersek wrote: Fair enough. It is undoing the effects of the earlier patch, but not in the same way as a straight revert (in part because enough else has changed in the meantime that a straight revert is no longer possible). Plus the decision on where to stage this in the series (at one point, I had it after 6/7 - and you already noted there how much rebase churn gets caused as we mix this series around). I completely missed that point. You do make a valid point that a C size_t might be larger than a Go int (does Go even try to run on 32-bit platforms?) - but we DO have the additional knowledge that because our block status reply results cannot exceed 64M over the wire, any count passed to the callback function will fit in 32 bits regardless of the width of C's size_t. Maybe an assertion that count does not lose precision when cast to Go int is sufficient? I don't know enough Go syntax to quickly state if there is some other way to coerce a := expression into having a guaranteed 64-bit native type. Okay, I'll need to rethink how to do this patch (perhaps by rearranging it to occur after 7/7, by folding it into the rest of the 64-bit series where I add the nbd_extent list conversion code). -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org