[libnbd PATCH 0/4] API: stats tracking
With this, I'll be able to clean up test 3/12 in the pending opt_set_meta_context() series to test whether filtering was done client- or server-side. Eric Blake (4): internal: Track traffic stats generator: Add RUInt64 for 64-bit counters without error api: Add new nbd_stats_*() statistics counters tests: Add language binding tests for stats reception docs/libnbd.pod | 20 ++ lib/internal.h | 6 + generator/API.ml | 83 +++++++- generator/API.mli | 3 +- generator/C.ml | 13 +- generator/GoLang.ml | 9 +- generator/OCaml.ml | 5 +- generator/Python.ml | 2 +- generator/states-issue-command.c | 3 +- generator/states-magic.c | 7 +- generator/states-newstyle-opt-export-name.c | 3 +- generator/states-newstyle-opt-go.c | 1 + generator/states-newstyle-opt-list.c | 3 +- generator/states-newstyle-opt-meta-context.c | 1 + generator/states-newstyle-opt-starttls.c | 3 +- .../states-newstyle-opt-structured-reply.c | 1 + generator/states-newstyle.c | 1 + generator/states-reply.c | 6 +- generator/states.c | 2 + lib/handle.c | 28 +++ python/t/620-stats.py | 77 ++++++++ ocaml/tests/Makefile.am | 1 + ocaml/tests/test_620_stats.ml | 77 ++++++++ examples/strict-structured-reads.c | 13 +- golang/Makefile.am | 3 +- golang/libnbd_620_stats.go | 181 ++++++++++++++++++ 26 files changed, 527 insertions(+), 25 deletions(-) create mode 100644 python/t/620-stats.py create mode 100644 ocaml/tests/test_620_stats.ml create mode 100644 golang/libnbd_620_stats.go -- 2.37.2
Increment statistics counters every time we send or receive bytes, as well as every time we send or receive a magic number delineating a portion of the data stream (the server's initial NBD_MAGIC, handshaking NBD_NEW_VERSION/NBD_OPT answered by one or more NBD_REP_MAGIC replies, and transmission NBD_REQUEST_MAGIC/NBD_CMD answered by NBD_SIMPLE_REPLY_MAGIC or one or more NBD_STRUCTURED_REPLY_MAGIC). A later patch will then expose these statistics to the client application. --- lib/internal.h | 6 ++++++ generator/states-issue-command.c | 3 ++- generator/states-magic.c | 7 +++++-- generator/states-newstyle-opt-export-name.c | 3 ++- generator/states-newstyle-opt-go.c | 1 + generator/states-newstyle-opt-list.c | 3 ++- generator/states-newstyle-opt-meta-context.c | 1 + generator/states-newstyle-opt-starttls.c | 3 ++- generator/states-newstyle-opt-structured-reply.c | 1 + generator/states-newstyle.c | 1 + generator/states-reply.c | 6 ++++-- generator/states.c | 2 ++ 12 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 8aaff151..1e991ef2 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -158,6 +158,12 @@ struct nbd_handle { int64_t unique; /* Used for generating cookie numbers. */ + /* Traffic statistics. */ + uint64_t bytes_sent; + uint64_t chunks_sent; + uint64_t bytes_received; + uint64_t chunks_received; + /* For debugging. */ bool debug; nbd_debug_callback debug_callback; diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index a8101144..df9295b5 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -47,6 +47,7 @@ ISSUE_COMMAND.START: h->request.handle = htobe64 (cmd->cookie); h->request.offset = htobe64 (cmd->offset); h->request.count = htobe32 ((uint32_t) cmd->count); + h->chunks_sent++; h->wbuf = &h->request; h->wlen = sizeof (h->request); if (cmd->type == NBD_CMD_WRITE || cmd->next) diff --git a/generator/states-magic.c b/generator/states-magic.c index d694a6ad..b9d4f9b2 100644 --- a/generator/states-magic.c +++ b/generator/states-magic.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -44,10 +44,13 @@ MAGIC.CHECK_MAGIC: version = be64toh (h->sbuf.new_handshake.version); if (version == NBD_NEW_VERSION) { assert (h->opt_current == 0); + h->chunks_received++; SET_NEXT_STATE (%.NEWSTYLE.START); } - else if (version == NBD_OLD_VERSION) + else if (version == NBD_OLD_VERSION) { + h->chunks_received++; SET_NEXT_STATE (%.OLDSTYLE.START); + } else { SET_NEXT_STATE (%.DEAD); set_error (0, "handshake: server is not either an oldstyle or fixed newstyle NBD server"); diff --git a/generator/states-newstyle-opt-export-name.c b/generator/states-newstyle-opt-export-name.c index 05975b61..398aa680 100644 --- a/generator/states-newstyle-opt-export-name.c +++ b/generator/states-newstyle-opt-export-name.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -23,6 +23,7 @@ NEWSTYLE.OPT_EXPORT_NAME.START: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_EXPORT_NAME); h->sbuf.option.optlen = htobe32 (strlen (h->export_name)); + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof h->sbuf.option; h->wflags = MSG_MORE; diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c index b7354aed..4c6e9900 100644 --- a/generator/states-newstyle-opt-go.c +++ b/generator/states-newstyle-opt-go.c @@ -40,6 +40,7 @@ NEWSTYLE.OPT_GO.START: h->sbuf.option.optlen = htobe32 (/* exportnamelen */ 4 + strlen (h->export_name) + sizeof nrinfos + 2 * nrinfos); + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof h->sbuf.option; h->wflags = MSG_MORE; diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c index a549bdc9..7bad8afe 100644 --- a/generator/states-newstyle-opt-list.c +++ b/generator/states-newstyle-opt-list.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -29,6 +29,7 @@ NEWSTYLE.OPT_LIST.START: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_LIST); h->sbuf.option.optlen = 0; + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof (h->sbuf.option); SET_NEXT_STATE (%SEND); diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 25c1c2fd..84dbcd69 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -75,6 +75,7 @@ NEWSTYLE.OPT_META_CONTEXT.START: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (opt); h->sbuf.option.optlen = htobe32 (len); + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof (h->sbuf.option); h->wflags = MSG_MORE; diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index 9eab023b..a9383ced 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -30,6 +30,7 @@ NEWSTYLE.OPT_STARTTLS.START: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_STARTTLS); h->sbuf.option.optlen = 0; + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof (h->sbuf.option); SET_NEXT_STATE (%SEND); diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c index 19c2ed7e..0ea0fd43 100644 --- a/generator/states-newstyle-opt-structured-reply.c +++ b/generator/states-newstyle-opt-structured-reply.c @@ -32,6 +32,7 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.START: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_STRUCTURED_REPLY); h->sbuf.option.optlen = htobe32 (0); + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof h->sbuf.option; SET_NEXT_STATE (%SEND); diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 41e7cc35..e84823e3 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -207,6 +207,7 @@ NEWSTYLE.PREPARE_OPT_ABORT: h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (NBD_OPT_ABORT); h->sbuf.option.optlen = htobe32 (0); + h->chunks_sent++; h->wbuf = &h->sbuf; h->wlen = sizeof h->sbuf.option; SET_NEXT_STATE (%SEND_OPT_ABORT); diff --git a/generator/states-reply.c b/generator/states-reply.c index 07363426..5a94ac1b 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -93,10 +93,10 @@ REPLY.START: return 0; } #ifdef DUMP_PACKETS - if (h->rbuf != NULL) - nbd_internal_hexdump (h->rbuf, r, stderr); + nbd_internal_hexdump (h->rbuf, r, stderr); #endif + h->bytes_received += r; h->rbuf += r; h->rlen -= r; SET_NEXT_STATE (%RECV_REPLY); @@ -117,9 +117,11 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY: magic = be32toh (h->sbuf.simple_reply.magic); if (magic == NBD_SIMPLE_REPLY_MAGIC) { + h->chunks_received++; SET_NEXT_STATE (%SIMPLE_REPLY.START); } else if (magic == NBD_STRUCTURED_REPLY_MAGIC) { + h->chunks_received++; SET_NEXT_STATE (%STRUCTURED_REPLY.START); } else { diff --git a/generator/states.c b/generator/states.c index d2aa51d3..547da753 100644 --- a/generator/states.c +++ b/generator/states.c @@ -89,6 +89,7 @@ recv_into_rbuf (struct nbd_handle *h) #ifdef DUMP_PACKETS nbd_internal_hexdump (rbuf, r, stderr); #endif + h->bytes_received += r; if (h->rbuf) h->rbuf += r; h->rlen -= r; @@ -112,6 +113,7 @@ send_from_wbuf (struct nbd_handle *h) /* sock->ops->send called set_error already. */ return -1; } + h->bytes_sent += r; h->wbuf += r; h->wlen -= r; if (h->wlen == 0) -- 2.37.2
On Fri, Sep 02, 2022 at 05:14:22PM -0500, Eric Blake wrote:
#ifdef DUMP_PACKETS - if (h->rbuf != NULL) - nbd_internal_hexdump (h->rbuf, r, stderr); + nbd_internal_hexdump (h->rbuf, r, stderr);
Should this be a separate patch? I guess the assertion here is that h->rbuf is set to an address earlier in the function so it can never be NULL. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Sat, Sep 03, 2022 at 05:25:11PM +0100, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 05:14:22PM -0500, Eric Blake wrote:
#ifdef DUMP_PACKETS - if (h->rbuf != NULL) - nbd_internal_hexdump (h->rbuf, r, stderr); + nbd_internal_hexdump (h->rbuf, r, stderr);
Should this be a separate patch? I guess the assertion here is that h->rbuf is set to an address earlier in the function so it can never be NULL.
Correct on both counts: I noticed it while touching the function; the 'if' is dead code becaues of earlier in the function, but a separate patch is worthwhile. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/03/22 20:26, Eric Blake wrote:
On Sat, Sep 03, 2022 at 05:25:11PM +0100, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 05:14:22PM -0500, Eric Blake wrote:
#ifdef DUMP_PACKETS - if (h->rbuf != NULL) - nbd_internal_hexdump (h->rbuf, r, stderr); + nbd_internal_hexdump (h->rbuf, r, stderr);
Should this be a separate patch? I guess the assertion here is that h->rbuf is set to an address earlier in the function so it can never be NULL.
Correct on both counts: I noticed it while touching the function; the 'if' is dead code becaues of earlier in the function, but a separate patch is worthwhile.
for both patches coming out of this: Acked-by: Laszlo Ersek <lersek@redhat.com>
The next patch will add some APIs that expose long-term statistics counters. A counter is always accessible (no need to return -1; if the handle is new the count is still 0, and even after the handle is in DEAD the counter is still useful). But it is also long-running; the existing RUInt might only be 32 bits, while it is easy to prove some counters (bytes transmitted, for example) will need 64-bits. So time to plumb in a new return type. Using signed int64 in OCaml bindings is okay (actually sending 2^63 bytes of data to overflow the counter takes a LOOONG time, so we don't worry if we can't get to 2^64). --- generator/API.ml | 5 ++++- generator/API.mli | 3 ++- generator/C.ml | 13 +++++++++---- generator/GoLang.ml | 9 ++++++--- generator/OCaml.ml | 5 +++-- generator/Python.ml | 2 +- 6 files changed, 25 insertions(+), 12 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 32a9ad79..e4e2ea0a 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -70,6 +70,7 @@ | RString | RUInt | RUIntPtr +| RUInt64 | REnum of enum | RFlags of flags and closure = { @@ -3368,10 +3369,12 @@ let () = failwithf "%s: if may_set_error is false, permitted_states must be empty (any permitted state)" name - (* may_set_error = true is incompatible with RUInt, REnum, and RFlags + (* may_set_error = true is incompatible with RUInt*, REnum, and RFlags * because these calls cannot return an error indication. *) | name, { ret = RUInt; may_set_error = true } + | name, { ret = RUIntPtr; may_set_error = true } + | name, { ret = RUInt64; may_set_error = true } | name, { ret = REnum _; may_set_error = true } | name, { ret = RFlags _; may_set_error = true } -> failwithf "%s: if ret is RUInt/REnum/RFlags, may_set_error must be false" name diff --git a/generator/API.mli b/generator/API.mli index d284637f..b0267705 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: the API - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -83,6 +83,7 @@ and caller frees, NULL for error *) | RUInt (** return a bitmask, no error possible *) | RUIntPtr (** uintptr_t in C, same as RUInt in non-C *) +| RUInt64 (** 64 bit int, no error possible *) | REnum of enum (** return an enum, no error possible *) | RFlags of flags (** return bitmask of flags, no error possible *) and closure = { diff --git a/generator/C.ml b/generator/C.ml index 9c67efaa..560f56db 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -68,7 +68,8 @@ let function | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1" | RStaticString | RString -> Some "NULL" - | RUInt | RUIntPtr | REnum _ | RFlags _ -> None (* errors not possible *) + | RUInt | RUIntPtr | RUInt64 | REnum _ + | RFlags _ -> None (* errors not possible *) let type_of_ret = function @@ -79,6 +80,7 @@ let | RString -> "char *" | RUInt -> "unsigned" | RUIntPtr -> "uintptr_t" + | RUInt64 -> "uint64_t" | RFlags _ -> "uint32_t" let rec name_of_arg = function @@ -730,24 +732,25 @@ let pr " nbd_internal_printable_string (ret);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | RUIntPtr | REnum _ | RFlags _ -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> () ); pr " debug (h, \"leave: ret=\" "; (match ret with | RBool | RErr | RFd | RInt | REnum _ -> pr "\"%%d\", ret" - | RInt64 | RCookie -> pr "\"%%\" PRIi64 \"\", ret" + | RInt64 | RCookie -> pr "\"%%\" PRIi64, ret" | RSizeT -> pr "\"%%zd\", ret" | RStaticString | RString -> pr "\"%%s\", ret_printable ? ret_printable : \"\"" | RUInt | RFlags _ -> pr "\"%%u\", ret" | RUIntPtr -> pr "\"%%\" PRIuPTR, ret" + | RUInt64 -> pr "\"%%\" PRIu64, ret" ); pr ");\n"; (match ret with | RStaticString | RString -> pr " free (ret_printable);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | REnum _ | RFlags _ | RUIntPtr -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> () ); pr " }\n"; pr " }\n" @@ -904,6 +907,8 @@ let pr "This call returns a bitmask.\n"; | RUIntPtr -> pr "This call returns a C<uintptr_t>.\n"; + | RUInt64 -> + pr "This call returns a counter.\n"; | REnum { enum_prefix } -> pr "This call returns one of the LIBNBD_%s_* values.\n" enum_prefix; | RFlags { flag_prefix } -> diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 73838199..eb7279a4 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -100,11 +100,12 @@ let | RCookie -> Some "uint64" | RSizeT -> Some "uint" | RString -> Some "*string" - (* RUInt | RUIntPtr returns (type, error) for consistency, but the + (* RUInt | RUIntPtr | RUInt64 returns (type, error) for consistency, but the * error is always nil unless h is closed *) | RUInt -> Some "uint" | RUIntPtr -> Some "uint" + | RUInt64 -> Some "uint64" | REnum { enum_prefix } -> Some (camel_case enum_prefix) | RFlags { flag_prefix } -> Some (camel_case flag_prefix) @@ -112,7 +113,7 @@ let | RErr -> None | RBool -> Some "false" | RStaticString | RString -> Some "nil" - | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr + | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> Some "0" let go_ret_c_errcode = function @@ -120,7 +121,7 @@ let | RStaticString -> Some "nil" | RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1" | RString -> Some "nil" - | RUInt | RUIntPtr | REnum _ | RFlags _ -> None + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> None (* We need a wrapper around every function (except Close) to * handle errors because cgo calls are sequence points and @@ -400,6 +401,8 @@ let pr " return uint (ret), nil\n" | RUIntPtr -> pr " return uint (ret), nil\n" + | RUInt64 -> + pr " return uint64 (ret), nil\n" | REnum { enum_prefix } -> pr " return %s (ret), nil\n" (camel_case enum_prefix) | RFlags { flag_prefix } -> diff --git a/generator/OCaml.ml b/generator/OCaml.ml index c708d454..f2ca0d18 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: generator - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -69,6 +69,7 @@ and | RSizeT -> "int" | RString -> "string" | RUInt | RUIntPtr -> "int" + | RUInt64 -> "int64" | REnum { enum_prefix } -> enum_prefix ^ ".t" | RFlags { flag_prefix } -> flag_prefix ^ ".t list" @@ -740,7 +741,7 @@ let | RFd | RInt | RSizeT | RUInt | RUIntPtr -> pr " rv = Val_int (r);\n" | REnum { enum_prefix } -> pr " rv = Val_%s (r);\n" enum_prefix | RFlags { flag_prefix } -> pr " rv = Val_%s (r);\n" flag_prefix - | RInt64 | RCookie -> pr " rv = caml_copy_int64 (r);\n" + | RInt64 | RCookie | RUInt64 -> pr " rv = caml_copy_int64 (r);\n" | RStaticString -> pr " rv = caml_copy_string (r);\n" | RString -> pr " rv = caml_copy_string (r);\n"; diff --git a/generator/Python.ml b/generator/Python.ml index cb89ccd7..449e1f9b 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -525,7 +525,7 @@ let | RErr -> pr " py_ret = Py_None;\n"; pr " Py_INCREF (py_ret);\n" - | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr -> + | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64 -> pr " py_ret = PyLong_FromLong (ret);\n" | RInt64 | RCookie -> pr " py_ret = PyLong_FromLongLong (ret);\n" -- 2.37.2
On 09/03/22 00:14, Eric Blake wrote:
The next patch will add some APIs that expose long-term statistics counters. A counter is always accessible (no need to return -1; if the handle is new the count is still 0, and even after the handle is in DEAD the counter is still useful). But it is also long-running; the existing RUInt might only be 32 bits, while it is easy to prove some counters (bytes transmitted, for example) will need 64-bits. So time to plumb in a new return type.
Using signed int64 in OCaml bindings is okay (actually sending 2^63 bytes of data to overflow the counter takes a LOOONG time, so we don't worry if we can't get to 2^64).
Should be OK for counters (the first use of the new data type in the generator), could be a problem for other purposes. See e.g. commit 0e714a6e06e6 ("ocaml: map C's uint32_t to OCaml's int64", 2022-01-17); mishandling the sign bit there caused infinite loops there. This is not an argument against the patch / new return data type in general, just a remark that in the future, once we try to use Uint64 for other things (not just byte / chunk counters), we could be surprised. Perhaps add a comment somewhere near the RUint64 data constructor in "generator/OCaml.ml"? ... Well, at the same time, we already silently map UInt64 (not RUInt64) to "int64", so there's already prior art for this. OK, feel free to ignore this remark.
--- generator/API.ml | 5 ++++- generator/API.mli | 3 ++- generator/C.ml | 13 +++++++++---- generator/GoLang.ml | 9 ++++++--- generator/OCaml.ml | 5 +++-- generator/Python.ml | 2 +- 6 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml index 32a9ad79..e4e2ea0a 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -70,6 +70,7 @@ | RString | RUInt | RUIntPtr +| RUInt64 | REnum of enum | RFlags of flags and closure = { @@ -3368,10 +3369,12 @@ let () = failwithf "%s: if may_set_error is false, permitted_states must be empty (any permitted state)" name
- (* may_set_error = true is incompatible with RUInt, REnum, and RFlags + (* may_set_error = true is incompatible with RUInt*, REnum, and RFlags * because these calls cannot return an error indication. *) | name, { ret = RUInt; may_set_error = true } + | name, { ret = RUIntPtr; may_set_error = true }
Silent fix for a different omission?
+ | name, { ret = RUInt64; may_set_error = true } | name, { ret = REnum _; may_set_error = true } | name, { ret = RFlags _; may_set_error = true } -> failwithf "%s: if ret is RUInt/REnum/RFlags, may_set_error must be false" name diff --git a/generator/API.mli b/generator/API.mli index d284637f..b0267705 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: the API - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -83,6 +83,7 @@ and caller frees, NULL for error *) | RUInt (** return a bitmask, no error possible *) | RUIntPtr (** uintptr_t in C, same as RUInt in non-C *) +| RUInt64 (** 64 bit int, no error possible *) | REnum of enum (** return an enum, no error possible *) | RFlags of flags (** return bitmask of flags, no error possible *) and closure = { diff --git a/generator/C.ml b/generator/C.ml index 9c67efaa..560f56db 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -68,7 +68,8 @@ let function | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1" | RStaticString | RString -> Some "NULL" - | RUInt | RUIntPtr | REnum _ | RFlags _ -> None (* errors not possible *) + | RUInt | RUIntPtr | RUInt64 | REnum _ + | RFlags _ -> None (* errors not possible *)
let type_of_ret = function @@ -79,6 +80,7 @@ let | RString -> "char *" | RUInt -> "unsigned" | RUIntPtr -> "uintptr_t" + | RUInt64 -> "uint64_t" | RFlags _ -> "uint32_t"
let rec name_of_arg = function @@ -730,24 +732,25 @@ let pr " nbd_internal_printable_string (ret);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | RUIntPtr | REnum _ | RFlags _ -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> () ); pr " debug (h, \"leave: ret=\" "; (match ret with | RBool | RErr | RFd | RInt | REnum _ -> pr "\"%%d\", ret" - | RInt64 | RCookie -> pr "\"%%\" PRIi64 \"\", ret" + | RInt64 | RCookie -> pr "\"%%\" PRIi64, ret"
another silent fix for an earlier problem?
| RSizeT -> pr "\"%%zd\", ret" | RStaticString | RString -> pr "\"%%s\", ret_printable ? ret_printable : \"\"" | RUInt | RFlags _ -> pr "\"%%u\", ret" | RUIntPtr -> pr "\"%%\" PRIuPTR, ret" + | RUInt64 -> pr "\"%%\" PRIu64, ret" ); pr ");\n"; (match ret with | RStaticString | RString -> pr " free (ret_printable);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | REnum _ | RFlags _ | RUIntPtr -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
looks like there were multiple RUintPtr omissions, those could go into a unified (but from this, separate) patch.
); pr " }\n"; pr " }\n" @@ -904,6 +907,8 @@ let pr "This call returns a bitmask.\n"; | RUIntPtr -> pr "This call returns a C<uintptr_t>.\n"; + | RUInt64 -> + pr "This call returns a counter.\n";
Hmmm. On one hand, I like that here we do tie the new type to counters. On the other hand, the explanation doesn't match the generic name "RUint64". Not sure what to suggest :/
| REnum { enum_prefix } -> pr "This call returns one of the LIBNBD_%s_* values.\n" enum_prefix; | RFlags { flag_prefix } -> diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 73838199..eb7279a4 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -100,11 +100,12 @@ let | RCookie -> Some "uint64" | RSizeT -> Some "uint" | RString -> Some "*string" - (* RUInt | RUIntPtr returns (type, error) for consistency, but the + (* RUInt | RUIntPtr | RUInt64 returns (type, error) for consistency, but the * error is always nil unless h is closed *) | RUInt -> Some "uint" | RUIntPtr -> Some "uint" + | RUInt64 -> Some "uint64" | REnum { enum_prefix } -> Some (camel_case enum_prefix) | RFlags { flag_prefix } -> Some (camel_case flag_prefix)
@@ -112,7 +113,7 @@ let | RErr -> None | RBool -> Some "false" | RStaticString | RString -> Some "nil" - | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr + | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> Some "0"
let go_ret_c_errcode = function @@ -120,7 +121,7 @@ let | RStaticString -> Some "nil" | RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1" | RString -> Some "nil" - | RUInt | RUIntPtr | REnum _ | RFlags _ -> None + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> None
(* We need a wrapper around every function (except Close) to * handle errors because cgo calls are sequence points and @@ -400,6 +401,8 @@ let pr " return uint (ret), nil\n" | RUIntPtr -> pr " return uint (ret), nil\n" + | RUInt64 -> + pr " return uint64 (ret), nil\n" | REnum { enum_prefix } -> pr " return %s (ret), nil\n" (camel_case enum_prefix) | RFlags { flag_prefix } -> diff --git a/generator/OCaml.ml b/generator/OCaml.ml index c708d454..f2ca0d18 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: generator - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -69,6 +69,7 @@ and | RSizeT -> "int" | RString -> "string" | RUInt | RUIntPtr -> "int" + | RUInt64 -> "int64" | REnum { enum_prefix } -> enum_prefix ^ ".t" | RFlags { flag_prefix } -> flag_prefix ^ ".t list"
@@ -740,7 +741,7 @@ let | RFd | RInt | RSizeT | RUInt | RUIntPtr -> pr " rv = Val_int (r);\n" | REnum { enum_prefix } -> pr " rv = Val_%s (r);\n" enum_prefix | RFlags { flag_prefix } -> pr " rv = Val_%s (r);\n" flag_prefix - | RInt64 | RCookie -> pr " rv = caml_copy_int64 (r);\n" + | RInt64 | RCookie | RUInt64 -> pr " rv = caml_copy_int64 (r);\n" | RStaticString -> pr " rv = caml_copy_string (r);\n" | RString -> pr " rv = caml_copy_string (r);\n"; diff --git a/generator/Python.ml b/generator/Python.ml index cb89ccd7..449e1f9b 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -525,7 +525,7 @@ let | RErr -> pr " py_ret = Py_None;\n"; pr " Py_INCREF (py_ret);\n" - | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr -> + | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64 -> pr " py_ret = PyLong_FromLong (ret);\n" | RInt64 | RCookie -> pr " py_ret = PyLong_FromLongLong (ret);\n"
This does not look right; I think RUInt64 belongs with RInt64 and RCookie (so that we invoke PyLong_FromLongLong()). Thanks Laszlo
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:
On 09/03/22 00:14, Eric Blake wrote:
The next patch will add some APIs that expose long-term statistics counters. A counter is always accessible (no need to return -1; if the handle is new the count is still 0, and even after the handle is in DEAD the counter is still useful). But it is also long-running; the existing RUInt might only be 32 bits, while it is easy to prove some counters (bytes transmitted, for example) will need 64-bits. So time to plumb in a new return type.
Using signed int64 in OCaml bindings is okay (actually sending 2^63 bytes of data to overflow the counter takes a LOOONG time, so we don't worry if we can't get to 2^64).
Should be OK for counters (the first use of the new data type in the generator), could be a problem for other purposes. See e.g. commit 0e714a6e06e6 ("ocaml: map C's uint32_t to OCaml's int64", 2022-01-17); mishandling the sign bit there caused infinite loops there.
This is not an argument against the patch / new return data type in general, just a remark that in the future, once we try to use Uint64 for other things (not just byte / chunk counters), we could be surprised.
Perhaps add a comment somewhere near the RUint64 data constructor in "generator/OCaml.ml"?
... Well, at the same time, we already silently map UInt64 (not RUInt64) to "int64", so there's already prior art for this. OK, feel free to ignore this remark.
And I did have a comment about RUInt64 being a counter in API.mli. ...
@@ -3368,10 +3369,12 @@ let () = failwithf "%s: if may_set_error is false, permitted_states must be empty (any permitted state)" name
- (* may_set_error = true is incompatible with RUInt, REnum, and RFlags + (* may_set_error = true is incompatible with RUInt*, REnum, and RFlags * because these calls cannot return an error indication. *) | name, { ret = RUInt; may_set_error = true } + | name, { ret = RUIntPtr; may_set_error = true }
Silent fix for a different omission?
Yes. I failed to call that out, but the commit is already in now. Missed in commit 85798518.
+++ b/generator/C.ml @@ -730,24 +732,25 @@ let pr " nbd_internal_printable_string (ret);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | RUIntPtr | REnum _ | RFlags _ -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> () ); pr " debug (h, \"leave: ret=\" "; (match ret with | RBool | RErr | RFd | RInt | REnum _ -> pr "\"%%d\", ret" - | RInt64 | RCookie -> pr "\"%%\" PRIi64 \"\", ret" + | RInt64 | RCookie -> pr "\"%%\" PRIi64, ret"
another silent fix for an earlier problem?
| RSizeT -> pr "\"%%zd\", ret" | RStaticString | RString -> pr "\"%%s\", ret_printable ? ret_printable : \"\"" | RUInt | RFlags _ -> pr "\"%%u\", ret" | RUIntPtr -> pr "\"%%\" PRIuPTR, ret" + | RUInt64 -> pr "\"%%\" PRIu64, ret"
Here, it was more of a consistency issue. Inserting a bare "" during string constant concatenation looks funny; we already omitted it in RUIntPtr (again, 85798518), and I liked that style better for RUInt64, so I copied it to RInt64 and RCookie. We could further compress things to have: pr " debug (h, \"leave: ret=\""; (match ret with | RBool -> pr "%%d\", ret" and so on, to get rid of yet another spurious pair of "" in the generated C code (since the return format is always exacty one argument, there's no need to have the generated api.c have "... ret=" "%<>" when "... ret=%<>" would do). I can touch that up in a followup, if it makes sense.
); pr ");\n"; (match ret with | RStaticString | RString -> pr " free (ret_printable);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | REnum _ | RFlags _ | RUIntPtr -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
looks like there were multiple RUintPtr omissions, those could go into a unified (but from this, separate) patch.
Actually, this line rearranged RUIntPtr to be earlier in the line (we don't have a strong precedent on whether branches to the match should be in alphabetical, declaration, or abitrary order).
); pr " }\n"; pr " }\n" @@ -904,6 +907,8 @@ let pr "This call returns a bitmask.\n"; | RUIntPtr -> pr "This call returns a C<uintptr_t>.\n"; + | RUInt64 -> + pr "This call returns a counter.\n";
Hmmm. On one hand, I like that here we do tie the new type to counters. On the other hand, the explanation doesn't match the generic name "RUint64". Not sure what to suggest :/
If we later find ourselves needing an actual 64-bit unsigned value, unrelated to counters, we can add RCounter at that point and repurpose RUInt64 to the new purpose. We've done that sort of enum splitting before; the enum names are less important that the underlying abstract type that then has to be ported to all the language bindings for the semantics it will be representing.
+++ b/generator/Python.ml @@ -525,7 +525,7 @@ let | RErr -> pr " py_ret = Py_None;\n"; pr " Py_INCREF (py_ret);\n" - | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr -> + | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64 -> pr " py_ret = PyLong_FromLong (ret);\n" | RInt64 | RCookie -> pr " py_ret = PyLong_FromLongLong (ret);\n"
This does not look right; I think RUInt64 belongs with RInt64 and RCookie (so that we invoke PyLong_FromLongLong()).
D'oh. Actual bug; now fixed as followup commit f79a1ac1 Thanks for reviewing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/06/22 11:08, Eric Blake wrote:
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:
We could further compress things to have:
pr " debug (h, \"leave: ret=\""; (match ret with | RBool -> pr "%%d\", ret"
and so on, to get rid of yet another spurious pair of "" in the generated C code (since the return format is always exacty one argument, there's no need to have the generated api.c have "... ret=" "%<>" when "... ret=%<>" would do). I can touch that up in a followup, if it makes sense.
It seems to simplify the generated code (which we sometimes quote in commit messages, when patching the generator), so if it's not a big burden, I'd be in favor of it.
); pr ");\n"; (match ret with | RStaticString | RString -> pr " free (ret_printable);\n" | RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT - | RUInt | REnum _ | RFlags _ | RUIntPtr -> () + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
looks like there were multiple RUintPtr omissions, those could go into a unified (but from this, separate) patch.
Actually, this line rearranged RUIntPtr to be earlier in the line (we don't have a strong precedent on whether branches to the match should be in alphabetical, declaration, or abitrary order).
Yup, sorry. The reordering, combined with the insertion, threw me off.
); pr " }\n"; pr " }\n" @@ -904,6 +907,8 @@ let pr "This call returns a bitmask.\n"; | RUIntPtr -> pr "This call returns a C<uintptr_t>.\n"; + | RUInt64 -> + pr "This call returns a counter.\n";
Hmmm. On one hand, I like that here we do tie the new type to counters. On the other hand, the explanation doesn't match the generic name "RUint64". Not sure what to suggest :/
If we later find ourselves needing an actual 64-bit unsigned value, unrelated to counters, we can add RCounter at that point and repurpose RUInt64 to the new purpose. We've done that sort of enum splitting before; the enum names are less important that the underlying abstract type that then has to be ported to all the language bindings for the semantics it will be representing.
OK.
+++ b/generator/Python.ml @@ -525,7 +525,7 @@ let | RErr -> pr " py_ret = Py_None;\n"; pr " Py_INCREF (py_ret);\n" - | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr -> + | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64 -> pr " py_ret = PyLong_FromLong (ret);\n" | RInt64 | RCookie -> pr " py_ret = PyLong_FromLongLong (ret);\n"
This does not look right; I think RUInt64 belongs with RInt64 and RCookie (so that we invoke PyLong_FromLongLong()).
D'oh. Actual bug; now fixed as followup commit f79a1ac1
Thanks for reviewing.
Cheers Laszlo
On Thu, Sep 08, 2022 at 02:53:21PM +0200, Laszlo Ersek wrote:
On 09/06/22 11:08, Eric Blake wrote:
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:
We could further compress things to have:
pr " debug (h, \"leave: ret=\""; (match ret with | RBool -> pr "%%d\", ret"
and so on, to get rid of yet another spurious pair of "" in the generated C code (since the return format is always exacty one argument, there's no need to have the generated api.c have "... ret=" "%<>" when "... ret=%<>" would do). I can touch that up in a followup, if it makes sense.
It seems to simplify the generated code (which we sometimes quote in commit messages, when patching the generator), so if it's not a big burden, I'd be in favor of it.
Done in commit 92e7a396. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/08/22 22:52, Eric Blake wrote:
On Thu, Sep 08, 2022 at 02:53:21PM +0200, Laszlo Ersek wrote:
On 09/06/22 11:08, Eric Blake wrote:
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:
We could further compress things to have:
pr " debug (h, \"leave: ret=\""; (match ret with | RBool -> pr "%%d\", ret"
and so on, to get rid of yet another spurious pair of "" in the generated C code (since the return format is always exacty one argument, there's no need to have the generated api.c have "... ret=" "%<>" when "... ret=%<>" would do). I can touch that up in a followup, if it makes sense.
It seems to simplify the generated code (which we sometimes quote in commit messages, when patching the generator), so if it's not a big burden, I'd be in favor of it.
Done in commit 92e7a396.
Thanks! Laszlo
On Tue, Sep 06, 2022 at 04:08:20AM -0500, Eric Blake wrote:
If we later find ourselves needing an actual 64-bit unsigned value, unrelated to counters, we can add RCounter at that point and repurpose RUInt64 to the new purpose. We've done that sort of enum splitting before; the enum names are less important that the underlying abstract type that then has to be ported to all the language bindings for the semantics it will be representing.
Right - the generator can change as much as we like. What matters is the C API/ABI. Less so the OCaml API: there's no guarantee for that, and we've changed that a bit over time, we just not to break it for no good reason. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
I was trying to write a test that determined whether a failure was due to a short-circuit test on the client side or an actual failure returned by the server. Such a test is a lot easier if we can watch status counters increase according to how much traffic goes over the wire. We may add more stats later (for example, how many bytes were written by nbd_[aio_]pread, or even how many times we had to grab the lock as part of an nbd_* command); or even to reset the counters for determining the protocol overhead around a particular set of I/O options, but this is a fun start. In this patch, update an example to serve as a compile time test of the new API; the next patch will add a runtime test of some simple uses while also checking the language bindings. --- docs/libnbd.pod | 20 ++++++++ generator/API.ml | 78 +++++++++++++++++++++++++++++- lib/handle.c | 28 +++++++++++ examples/strict-structured-reads.c | 13 ++++- 4 files changed, 135 insertions(+), 4 deletions(-) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 7a01179a..d256cd65 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -940,6 +940,26 @@ should return C<1> on all control paths, even when handling errors (note that with automatic retirement, assigning into C<error> is pointless as there is no later API to see that value). +=head1 STATISTICS COUNTERS + +Libnbd tracks several statistics counters, useful for tracking how +much traffic was sent over the connection. The counters track the +number of bytes sent and received, as well as the number of protocol +chunks (a group of bytes delineated by a magic number). + + printf ("bytes: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_bytes_sent (nbd), nbd_stats_bytes_received (nbd)); + printf ("chunks: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_chunks_sent (nbd), nbd_stats_chunks_received (nbd)); + +Note that if a command fails early at the client without needing to +consult the server, the counters will not be incremented; conversely, +the chunk counts can increase by more than one for a single API call +(for example, L<nbd_opt_go(3)> defaults to sending a two-option +sequence of C<NBD_OPT_SET_META_CONTEXT> and C<NBD_OPT_GO>, and the +server in turn replies with multiple C<NBD_REP_INFO> before concluding +with C<NBD_REP_ACK>). + =head1 SIGNALS Libnbd does not install signal handlers. It attempts to disable diff --git a/generator/API.ml b/generator/API.ml index e4e2ea0a..dbfde284 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -307,6 +307,70 @@ "clear_debug_callback", { callback was associated this does nothing."; }; + "stats_bytes_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes sent over socket so far"; + longdesc = "\ +Return the number of bytes that the client has sent to the server."; + see_also = [Link "stats_chunks_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"]; + }; + + "stats_chunks_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks sent over socket so far"; + longdesc = "\ +Return the number of chunks that the client has sent to the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a request is sent to the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one for APIs that wrap multiple +underlying requests (such as L<nbd_opt_go(3)>)."; + see_also = [Link "stats_bytes_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"; + Link "set_strict_mode"]; + }; + + "stats_bytes_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes received over socket so far"; + longdesc = "\ +Return the number of bytes that the client has received from the server."; + see_also = [Link "stats_chunks_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"]; + }; + + "stats_chunks_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks received over socket so far"; + longdesc = "\ +Return the number of chunks that the client has received from the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a reply is received from the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one when the server utilizes structured +replies (see L<nbd_get_structured_replies_negotiated(3)>)."; + see_also = [Link "stats_bytes_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"; + Link "get_structured_replies_negotiated"]; + }; + "set_handle_name", { default_call with args = [ String "handle_name" ]; ret = RErr; @@ -945,8 +1009,14 @@ "set_strict_mode", { such, when attempting to relax only one specific bit while keeping remaining checks at the client side, it is wiser to first call L<nbd_get_strict_mode(3)> and modify that value, rather than -blindly setting a constant value."; - see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; +blindly setting a constant value. + +When commands are not being attempted in parallel, it is possible +to observe whether a given command was filtered at the client +or triggered a server response by whether the statistics +counters increase (such as L<nbd_stats_bytes_sent(3)>)."; + see_also = [Link "get_strict_mode"; Link "set_handshake_flags"; + Link "stats_bytes_sent"; Link "stats_bytes_received"]; }; "get_strict_mode", { @@ -3274,6 +3344,10 @@ let first_version = (* Added in 1.15.x development cycle, will be stable and supported in 1.16. *) "poll2", (1, 16); "supports_vsock", (1, 16); + "stats_bytes_sent", (1, 16); + "stats_chunks_sent", (1, 16); + "stats_bytes_received", (1, 16); + "stats_chunks_received", (1, 16); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index 7fcdc8cd..d4426260 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -548,3 +548,31 @@ nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow) h->uri_allow_local_file = allow; return 0; } + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_sent (struct nbd_handle *h) +{ + return h->bytes_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_sent (struct nbd_handle *h) +{ + return h->chunks_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_received (struct nbd_handle *h) +{ + return h->bytes_received; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_received (struct nbd_handle *h) +{ + return h->chunks_received; +} diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index 6ea17006..b56a4825 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -4,6 +4,10 @@ * qemu-nbd -f $format -k $sock -r image * ./strict-structured-reads $sock * + * With nbdkit (but less useful until nbdkit learns to send holes): + * + * nbdkit -U- sparse-random 1G --run './strict-structured-reads "$unixsocket"' + * * This will perform read randomly over the image and check that all * structured replies comply with the NBD spec (chunks may be out of * order or interleaved, but no read succeeds unless chunks cover the @@ -257,8 +261,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - nbd_close (nbd); - + printf ("traffic:\n"); + printf (" bytes sent: %10" PRIu64 "\n", nbd_stats_bytes_sent (nbd)); + printf (" bytes received: %10" PRIu64 "\n", nbd_stats_bytes_received (nbd)); + printf (" chunks sent: %10" PRIu64 "\n", nbd_stats_chunks_sent (nbd)); + printf (" chunks received: %10" PRIu64 "\n", nbd_stats_chunks_received (nbd)); printf ("totals:\n"); printf (" data chunks: %10d\n", total_data_chunks); printf (" data bytes: %10" PRId64 "\n", total_data_bytes); @@ -270,5 +277,7 @@ main (int argc, char *argv[]) printf (" bytes read: %10" PRId64 "\n", total_bytes); printf (" compliant: %10d\n", total_success); + nbd_close (nbd); + exit (EXIT_SUCCESS); } -- 2.37.2
On Fri, Sep 02, 2022 at 05:14:24PM -0500, Eric Blake wrote:
I was trying to write a test that determined whether a failure was due to a short-circuit test on the client side or an actual failure returned by the server. Such a test is a lot easier if we can watch status counters increase according to how much traffic goes over the wire. We may add more stats later (for example, how many bytes were written by nbd_[aio_]pread, or even how many times we had to grab the lock as part of an nbd_* command); or even to reset the counters for determining the protocol overhead around a particular set of I/O options, but this is a fun start.
In this patch, update an example to serve as a compile time test of the new API; the next patch will add a runtime test of some simple uses while also checking the language bindings. --- docs/libnbd.pod | 20 ++++++++ generator/API.ml | 78 +++++++++++++++++++++++++++++- lib/handle.c | 28 +++++++++++ examples/strict-structured-reads.c | 13 ++++- 4 files changed, 135 insertions(+), 4 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 7a01179a..d256cd65 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -940,6 +940,26 @@ should return C<1> on all control paths, even when handling errors (note that with automatic retirement, assigning into C<error> is pointless as there is no later API to see that value).
+=head1 STATISTICS COUNTERS + +Libnbd tracks several statistics counters, useful for tracking how +much traffic was sent over the connection. The counters track the +number of bytes sent and received, as well as the number of protocol +chunks (a group of bytes delineated by a magic number). + + printf ("bytes: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_bytes_sent (nbd), nbd_stats_bytes_received (nbd)); + printf ("chunks: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_chunks_sent (nbd), nbd_stats_chunks_received (nbd));
You probably also want to note that if TLS is enabled these don't relate closely to either bytes or packets sent.
+Note that if a command fails early at the client without needing to +consult the server, the counters will not be incremented; conversely, +the chunk counts can increase by more than one for a single API call +(for example, L<nbd_opt_go(3)> defaults to sending a two-option +sequence of C<NBD_OPT_SET_META_CONTEXT> and C<NBD_OPT_GO>, and the +server in turn replies with multiple C<NBD_REP_INFO> before concluding +with C<NBD_REP_ACK>).
The trouble is that by writing this down, we need to keep supporting this. If you don't write it down, but it's implicit in our tests then we can change the API later if it turns out to be hard to support. So my preference would be to simply delete this paragraph and similar below ...
=head1 SIGNALS
Libnbd does not install signal handlers. It attempts to disable diff --git a/generator/API.ml b/generator/API.ml index e4e2ea0a..dbfde284 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -307,6 +307,70 @@ "clear_debug_callback", { callback was associated this does nothing."; };
+ "stats_bytes_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes sent over socket so far"; + longdesc = "\ +Return the number of bytes that the client has sent to the server."; + see_also = [Link "stats_chunks_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"]; + }; + + "stats_chunks_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks sent over socket so far"; + longdesc = "\ +Return the number of chunks that the client has sent to the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a request is sent to the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one for APIs that wrap multiple +underlying requests (such as L<nbd_opt_go(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"; + Link "set_strict_mode"]; + }; + + "stats_bytes_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes received over socket so far"; + longdesc = "\ +Return the number of bytes that the client has received from the server."; + see_also = [Link "stats_chunks_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"]; + }; + + "stats_chunks_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks received over socket so far"; + longdesc = "\ +Return the number of chunks that the client has received from the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a reply is received from the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one when the server utilizes structured +replies (see L<nbd_get_structured_replies_negotiated(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"; + Link "get_structured_replies_negotiated"]; + }; + "set_handle_name", { default_call with args = [ String "handle_name" ]; ret = RErr; @@ -945,8 +1009,14 @@ "set_strict_mode", { such, when attempting to relax only one specific bit while keeping remaining checks at the client side, it is wiser to first call L<nbd_get_strict_mode(3)> and modify that value, rather than -blindly setting a constant value."; - see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; +blindly setting a constant value. + +When commands are not being attempted in parallel, it is possible +to observe whether a given command was filtered at the client +or triggered a server response by whether the statistics +counters increase (such as L<nbd_stats_bytes_sent(3)>).";
^ This one.
+ see_also = [Link "get_strict_mode"; Link "set_handshake_flags"; + Link "stats_bytes_sent"; Link "stats_bytes_received"]; };
"get_strict_mode", { @@ -3274,6 +3344,10 @@ let first_version = (* Added in 1.15.x development cycle, will be stable and supported in 1.16. *) "poll2", (1, 16); "supports_vsock", (1, 16); + "stats_bytes_sent", (1, 16); + "stats_chunks_sent", (1, 16); + "stats_bytes_received", (1, 16); + "stats_chunks_received", (1, 16);
(* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index 7fcdc8cd..d4426260 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -548,3 +548,31 @@ nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow) h->uri_allow_local_file = allow; return 0; } + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_sent (struct nbd_handle *h) +{ + return h->bytes_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_sent (struct nbd_handle *h) +{ + return h->chunks_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_received (struct nbd_handle *h) +{ + return h->bytes_received; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_received (struct nbd_handle *h) +{ + return h->chunks_received; +} diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index 6ea17006..b56a4825 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -4,6 +4,10 @@ * qemu-nbd -f $format -k $sock -r image * ./strict-structured-reads $sock * + * With nbdkit (but less useful until nbdkit learns to send holes): + * + * nbdkit -U- sparse-random 1G --run './strict-structured-reads "$unixsocket"' + *
Is this hunk meant to be included?
* This will perform read randomly over the image and check that all * structured replies comply with the NBD spec (chunks may be out of * order or interleaved, but no read succeeds unless chunks cover the @@ -257,8 +261,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); }
- nbd_close (nbd); - + printf ("traffic:\n"); + printf (" bytes sent: %10" PRIu64 "\n", nbd_stats_bytes_sent (nbd)); + printf (" bytes received: %10" PRIu64 "\n", nbd_stats_bytes_received (nbd)); + printf (" chunks sent: %10" PRIu64 "\n", nbd_stats_chunks_sent (nbd)); + printf (" chunks received: %10" PRIu64 "\n", nbd_stats_chunks_received (nbd)); printf ("totals:\n"); printf (" data chunks: %10d\n", total_data_chunks); printf (" data bytes: %10" PRId64 "\n", total_data_bytes); @@ -270,5 +277,7 @@ main (int argc, char *argv[]) printf (" bytes read: %10" PRId64 "\n", total_bytes); printf (" compliant: %10d\n", total_success);
+ nbd_close (nbd); + exit (EXIT_SUCCESS); } --
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 09/03/22 18:30, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 05:14:24PM -0500, Eric Blake wrote:
I was trying to write a test that determined whether a failure was due to a short-circuit test on the client side or an actual failure returned by the server. Such a test is a lot easier if we can watch status counters increase according to how much traffic goes over the wire. We may add more stats later (for example, how many bytes were written by nbd_[aio_]pread, or even how many times we had to grab the lock as part of an nbd_* command); or even to reset the counters for determining the protocol overhead around a particular set of I/O options, but this is a fun start.
In this patch, update an example to serve as a compile time test of the new API; the next patch will add a runtime test of some simple uses while also checking the language bindings. --- docs/libnbd.pod | 20 ++++++++ generator/API.ml | 78 +++++++++++++++++++++++++++++- lib/handle.c | 28 +++++++++++ examples/strict-structured-reads.c | 13 ++++- 4 files changed, 135 insertions(+), 4 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 7a01179a..d256cd65 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -940,6 +940,26 @@ should return C<1> on all control paths, even when handling errors (note that with automatic retirement, assigning into C<error> is pointless as there is no later API to see that value).
+=head1 STATISTICS COUNTERS + +Libnbd tracks several statistics counters, useful for tracking how +much traffic was sent over the connection. The counters track the +number of bytes sent and received, as well as the number of protocol +chunks (a group of bytes delineated by a magic number). + + printf ("bytes: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_bytes_sent (nbd), nbd_stats_bytes_received (nbd)); + printf ("chunks: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_chunks_sent (nbd), nbd_stats_chunks_received (nbd));
You probably also want to note that if TLS is enabled these don't relate closely to either bytes or packets sent.
+Note that if a command fails early at the client without needing to +consult the server, the counters will not be incremented; conversely, +the chunk counts can increase by more than one for a single API call +(for example, L<nbd_opt_go(3)> defaults to sending a two-option +sequence of C<NBD_OPT_SET_META_CONTEXT> and C<NBD_OPT_GO>, and the +server in turn replies with multiple C<NBD_REP_INFO> before concluding +with C<NBD_REP_ACK>).
The trouble is that by writing this down, we need to keep supporting this. If you don't write it down, but it's implicit in our tests then we can change the API later if it turns out to be hard to support. So my preference would be to simply delete this paragraph and similar below ...
We ought to be able to document the (originally) intended purpose of a feature such that it does not come with "support strings attached". Commit messages are good, code comments are good too, yes, but what about explicit documentation? E.g. "p2v-hacking.pod" is a great thing. Anyway I don't insist; I just thought (without realizing the "support strings attached") that exactly those paragraphs were the nicest touch ;) So either way: Acked-by: Laszlo Ersek <lersek@redhat.com> Laszlo
=head1 SIGNALS
Libnbd does not install signal handlers. It attempts to disable diff --git a/generator/API.ml b/generator/API.ml index e4e2ea0a..dbfde284 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -307,6 +307,70 @@ "clear_debug_callback", { callback was associated this does nothing."; };
+ "stats_bytes_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes sent over socket so far"; + longdesc = "\ +Return the number of bytes that the client has sent to the server."; + see_also = [Link "stats_chunks_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"]; + }; + + "stats_chunks_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks sent over socket so far"; + longdesc = "\ +Return the number of chunks that the client has sent to the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a request is sent to the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one for APIs that wrap multiple +underlying requests (such as L<nbd_opt_go(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"; + Link "set_strict_mode"]; + }; + + "stats_bytes_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes received over socket so far"; + longdesc = "\ +Return the number of bytes that the client has received from the server."; + see_also = [Link "stats_chunks_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"]; + }; + + "stats_chunks_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks received over socket so far"; + longdesc = "\ +Return the number of chunks that the client has received from the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a reply is received from the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one when the server utilizes structured +replies (see L<nbd_get_structured_replies_negotiated(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"; + Link "get_structured_replies_negotiated"]; + }; + "set_handle_name", { default_call with args = [ String "handle_name" ]; ret = RErr; @@ -945,8 +1009,14 @@ "set_strict_mode", { such, when attempting to relax only one specific bit while keeping remaining checks at the client side, it is wiser to first call L<nbd_get_strict_mode(3)> and modify that value, rather than -blindly setting a constant value."; - see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; +blindly setting a constant value. + +When commands are not being attempted in parallel, it is possible +to observe whether a given command was filtered at the client +or triggered a server response by whether the statistics +counters increase (such as L<nbd_stats_bytes_sent(3)>).";
^ This one.
+ see_also = [Link "get_strict_mode"; Link "set_handshake_flags"; + Link "stats_bytes_sent"; Link "stats_bytes_received"]; };
"get_strict_mode", { @@ -3274,6 +3344,10 @@ let first_version = (* Added in 1.15.x development cycle, will be stable and supported in 1.16. *) "poll2", (1, 16); "supports_vsock", (1, 16); + "stats_bytes_sent", (1, 16); + "stats_chunks_sent", (1, 16); + "stats_bytes_received", (1, 16); + "stats_chunks_received", (1, 16);
(* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index 7fcdc8cd..d4426260 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -548,3 +548,31 @@ nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow) h->uri_allow_local_file = allow; return 0; } + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_sent (struct nbd_handle *h) +{ + return h->bytes_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_sent (struct nbd_handle *h) +{ + return h->chunks_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_received (struct nbd_handle *h) +{ + return h->bytes_received; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_received (struct nbd_handle *h) +{ + return h->chunks_received; +} diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index 6ea17006..b56a4825 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -4,6 +4,10 @@ * qemu-nbd -f $format -k $sock -r image * ./strict-structured-reads $sock * + * With nbdkit (but less useful until nbdkit learns to send holes): + * + * nbdkit -U- sparse-random 1G --run './strict-structured-reads "$unixsocket"' + *
Is this hunk meant to be included?
* This will perform read randomly over the image and check that all * structured replies comply with the NBD spec (chunks may be out of * order or interleaved, but no read succeeds unless chunks cover the @@ -257,8 +261,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); }
- nbd_close (nbd); - + printf ("traffic:\n"); + printf (" bytes sent: %10" PRIu64 "\n", nbd_stats_bytes_sent (nbd)); + printf (" bytes received: %10" PRIu64 "\n", nbd_stats_bytes_received (nbd)); + printf (" chunks sent: %10" PRIu64 "\n", nbd_stats_chunks_sent (nbd)); + printf (" chunks received: %10" PRIu64 "\n", nbd_stats_chunks_received (nbd)); printf ("totals:\n"); printf (" data chunks: %10d\n", total_data_chunks); printf (" data bytes: %10" PRId64 "\n", total_data_bytes); @@ -270,5 +277,7 @@ main (int argc, char *argv[]) printf (" bytes read: %10" PRId64 "\n", total_bytes); printf (" compliant: %10d\n", total_success);
+ nbd_close (nbd); + exit (EXIT_SUCCESS); } --
Rich.
On Mon, Sep 05, 2022 at 02:12:24PM +0200, Laszlo Ersek wrote:
On 09/03/22 18:30, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 05:14:24PM -0500, Eric Blake wrote:
I was trying to write a test that determined whether a failure was due to a short-circuit test on the client side or an actual failure returned by the server. Such a test is a lot easier if we can watch status counters increase according to how much traffic goes over the wire. We may add more stats later (for example, how many bytes were written by nbd_[aio_]pread, or even how many times we had to grab the lock as part of an nbd_* command); or even to reset the counters for determining the protocol overhead around a particular set of I/O options, but this is a fun start.
In this patch, update an example to serve as a compile time test of the new API; the next patch will add a runtime test of some simple uses while also checking the language bindings. --- docs/libnbd.pod | 20 ++++++++ generator/API.ml | 78 +++++++++++++++++++++++++++++- lib/handle.c | 28 +++++++++++ examples/strict-structured-reads.c | 13 ++++- 4 files changed, 135 insertions(+), 4 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 7a01179a..d256cd65 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -940,6 +940,26 @@ should return C<1> on all control paths, even when handling errors (note that with automatic retirement, assigning into C<error> is pointless as there is no later API to see that value).
+=head1 STATISTICS COUNTERS + +Libnbd tracks several statistics counters, useful for tracking how +much traffic was sent over the connection. The counters track the +number of bytes sent and received, as well as the number of protocol +chunks (a group of bytes delineated by a magic number). + + printf ("bytes: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_bytes_sent (nbd), nbd_stats_bytes_received (nbd)); + printf ("chunks: sent=%" PRIu64 " received=%" PRIu64, + nbd_stats_chunks_sent (nbd), nbd_stats_chunks_received (nbd));
You probably also want to note that if TLS is enabled these don't relate closely to either bytes or packets sent.
+Note that if a command fails early at the client without needing to +consult the server, the counters will not be incremented; conversely, +the chunk counts can increase by more than one for a single API call +(for example, L<nbd_opt_go(3)> defaults to sending a two-option +sequence of C<NBD_OPT_SET_META_CONTEXT> and C<NBD_OPT_GO>, and the +server in turn replies with multiple C<NBD_REP_INFO> before concluding +with C<NBD_REP_ACK>).
The trouble is that by writing this down, we need to keep supporting this. If you don't write it down, but it's implicit in our tests then we can change the API later if it turns out to be hard to support. So my preference would be to simply delete this paragraph and similar below ...
We ought to be able to document the (originally) intended purpose of a feature such that it does not come with "support strings attached". Commit messages are good, code comments are good too, yes, but what about explicit documentation? E.g. "p2v-hacking.pod" is a great thing.
Anyway I don't insist; I just thought (without realizing the "support strings attached") that exactly those paragraphs were the nicest touch ;)
In libnbd we've committed to supporting the ABI as long as the project is around, so that means the barrier to entry for new features is a bit higher. I'm wary in particular of features that appear to depend on internal implementations because that makes things very hard to change in future. Counting bytes seems to be OK though because NBD is always going to send bytes on a wire (and if it doesn't, we return 0). This could also be about where the documentation goes or whether it is defended with suitable disclaimers about "this may change / break in future". But as written above it looks like something supported. Rich.
So either way:
Acked-by: Laszlo Ersek <lersek@redhat.com>
Laszlo
=head1 SIGNALS
Libnbd does not install signal handlers. It attempts to disable diff --git a/generator/API.ml b/generator/API.ml index e4e2ea0a..dbfde284 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -307,6 +307,70 @@ "clear_debug_callback", { callback was associated this does nothing."; };
+ "stats_bytes_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes sent over socket so far"; + longdesc = "\ +Return the number of bytes that the client has sent to the server."; + see_also = [Link "stats_chunks_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"]; + }; + + "stats_chunks_sent", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks sent over socket so far"; + longdesc = "\ +Return the number of chunks that the client has sent to the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a request is sent to the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one for APIs that wrap multiple +underlying requests (such as L<nbd_opt_go(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_sent"; + Link "stats_bytes_received"; Link "stats_chunks_received"; + Link "set_strict_mode"]; + }; + + "stats_bytes_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of bytes received over socket so far"; + longdesc = "\ +Return the number of bytes that the client has received from the server."; + see_also = [Link "stats_chunks_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"]; + }; + + "stats_chunks_received", { + default_call with + args = []; ret = RUInt64; + may_set_error = false; + shortdesc = "statistics of chunks received over socket so far"; + longdesc = "\ +Return the number of chunks that the client has received from the +server, where a chunk is a group of bytes delineated by a magic +number that cannot be further subdivided without breaking the +protocol. + +This number increments when a reply is received from the server; +it is unchanged for an API that fails during initial client-side +sanity checking (see L<nbd_set_strict_mode(3)>), and can +increment by more than one when the server utilizes structured +replies (see L<nbd_get_structured_replies_negotiated(3)>).";
^ This one.
+ see_also = [Link "stats_bytes_received"; + Link "stats_bytes_sent"; Link "stats_chunks_sent"; + Link "get_structured_replies_negotiated"]; + }; + "set_handle_name", { default_call with args = [ String "handle_name" ]; ret = RErr; @@ -945,8 +1009,14 @@ "set_strict_mode", { such, when attempting to relax only one specific bit while keeping remaining checks at the client side, it is wiser to first call L<nbd_get_strict_mode(3)> and modify that value, rather than -blindly setting a constant value."; - see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; +blindly setting a constant value. + +When commands are not being attempted in parallel, it is possible +to observe whether a given command was filtered at the client +or triggered a server response by whether the statistics +counters increase (such as L<nbd_stats_bytes_sent(3)>).";
^ This one.
+ see_also = [Link "get_strict_mode"; Link "set_handshake_flags"; + Link "stats_bytes_sent"; Link "stats_bytes_received"]; };
"get_strict_mode", { @@ -3274,6 +3344,10 @@ let first_version = (* Added in 1.15.x development cycle, will be stable and supported in 1.16. *) "poll2", (1, 16); "supports_vsock", (1, 16); + "stats_bytes_sent", (1, 16); + "stats_chunks_sent", (1, 16); + "stats_bytes_received", (1, 16); + "stats_chunks_received", (1, 16);
(* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index 7fcdc8cd..d4426260 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -548,3 +548,31 @@ nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow) h->uri_allow_local_file = allow; return 0; } + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_sent (struct nbd_handle *h) +{ + return h->bytes_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_sent (struct nbd_handle *h) +{ + return h->chunks_sent; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_bytes_received (struct nbd_handle *h) +{ + return h->bytes_received; +} + +/* NB: may_set_error = false. */ +uint64_t +nbd_unlocked_stats_chunks_received (struct nbd_handle *h) +{ + return h->chunks_received; +} diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index 6ea17006..b56a4825 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -4,6 +4,10 @@ * qemu-nbd -f $format -k $sock -r image * ./strict-structured-reads $sock * + * With nbdkit (but less useful until nbdkit learns to send holes): + * + * nbdkit -U- sparse-random 1G --run './strict-structured-reads "$unixsocket"' + *
Is this hunk meant to be included?
* This will perform read randomly over the image and check that all * structured replies comply with the NBD spec (chunks may be out of * order or interleaved, but no read succeeds unless chunks cover the @@ -257,8 +261,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); }
- nbd_close (nbd); - + printf ("traffic:\n"); + printf (" bytes sent: %10" PRIu64 "\n", nbd_stats_bytes_sent (nbd)); + printf (" bytes received: %10" PRIu64 "\n", nbd_stats_bytes_received (nbd)); + printf (" chunks sent: %10" PRIu64 "\n", nbd_stats_chunks_sent (nbd)); + printf (" chunks received: %10" PRIu64 "\n", nbd_stats_chunks_received (nbd)); printf ("totals:\n"); printf (" data chunks: %10d\n", total_data_chunks); printf (" data bytes: %10" PRId64 "\n", total_data_bytes); @@ -270,5 +277,7 @@ main (int argc, char *argv[]) printf (" bytes read: %10" PRId64 "\n", total_bytes); printf (" compliant: %10d\n", total_success);
+ nbd_close (nbd); + exit (EXIT_SUCCESS); } --
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
This proves that the stat counters increment as desired, as well as proving that our RUInt32 generator type works. This commit is also a showcase of whether I can do 64-bit math in various languages (C's terseness in 'a == b + (c > d)' is annoying to replicate in languages that don't like playing fast and loose with types). :) --- python/t/620-stats.py | 77 +++++++++++++++ ocaml/tests/Makefile.am | 1 + ocaml/tests/test_620_stats.ml | 77 +++++++++++++++ golang/Makefile.am | 3 +- golang/libnbd_620_stats.go | 181 ++++++++++++++++++++++++++++++++++ 5 files changed, 338 insertions(+), 1 deletion(-) create mode 100644 python/t/620-stats.py create mode 100644 ocaml/tests/test_620_stats.ml create mode 100644 golang/libnbd_620_stats.go diff --git a/python/t/620-stats.py b/python/t/620-stats.py new file mode 100644 index 00000000..62f8443f --- /dev/null +++ b/python/t/620-stats.py @@ -0,0 +1,77 @@ +# libnbd Python bindings +# Copyright (C) 2010-2022 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import nbd + +h = nbd.NBD() + +# Pre-connection, stats start out at 0 +bs0 = h.stats_bytes_sent() +cs0 = h.stats_chunks_sent() +br0 = h.stats_bytes_received() +cr0 = h.stats_chunks_received() + +assert bs0 == 0 +assert cs0 == 0 +assert br0 == 0 +assert cr0 == 0 + +# Connection performs handshaking, which increments stats. +# The number of bytes/chunks here may grow over time as more features get +# automatically negotiated, so merely check that they are non-zero. +h.connect_command(["nbdkit", "-s", "--exit-with-parent", "null"]) + +bs1 = h.stats_bytes_sent() +cs1 = h.stats_chunks_sent() +br1 = h.stats_bytes_received() +cr1 = h.stats_chunks_received() + +assert cs1 > 0 +assert bs1 > cs1 +assert cr1 > 0 +assert br1 > cr1 + +# A flush command should be one chunk out, one chunk back (even if +# structured replies are in use) +h.flush() + +bs2 = h.stats_bytes_sent() +cs2 = h.stats_chunks_sent() +br2 = h.stats_bytes_received() +cr2 = h.stats_chunks_received() + +assert bs2 == bs1 + 28 +assert cs2 == cs1 + 1 +assert br2 == br1 + 16 # assumes nbdkit uses simple reply +assert cr2 == cr1 + 1 + +# Stats are still readable after the connection closes; we don't know if +# the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. +h.shutdown() + +bs3 = h.stats_bytes_sent() +cs3 = h.stats_chunks_sent() +br3 = h.stats_bytes_received() +cr3 = h.stats_chunks_received() + +assert bs3 > bs2 +assert cs3 == cs2 + 1 +assert br3 >= br2 +assert cr3 == cr2 + (br3 > br2) + +# Try to trigger garbage collection of h +h = None diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am index 22cefb4d..c4751ad3 100644 --- a/ocaml/tests/Makefile.am +++ b/ocaml/tests/Makefile.am @@ -42,6 +42,7 @@ ML_TESTS = \ test_590_aio_copy.ml \ test_600_debug_callback.ml \ test_610_exception.ml \ + test_620_stats.ml \ $(NULL) EXTRA_DIST = $(ML_TESTS) diff --git a/ocaml/tests/test_620_stats.ml b/ocaml/tests/test_620_stats.ml new file mode 100644 index 00000000..648096fd --- /dev/null +++ b/ocaml/tests/test_620_stats.ml @@ -0,0 +1,77 @@ +(* hey emacs, this is OCaml code: -*- tuareg -*- *) +(* libnbd OCaml test case + * Copyright (C) 2013-2022 Red Hat Inc. + * + * 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 + *) + +let () = + let nbd = NBD.create () in + + (* Pre-connection, stats start out at 0 *) + let bs0 = NBD.stats_bytes_sent nbd in + let cs0 = NBD.stats_chunks_sent nbd in + let br0 = NBD.stats_bytes_received nbd in + let cr0 = NBD.stats_chunks_received nbd in + assert (bs0 = 0L); + assert (cs0 = 0L); + assert (br0 = 0L); + assert (cr0 = 0L); + + (* Connection performs handshaking, which increments stats. + * The number of bytes/chunks here may grow over time as more features get + * automatically negotiated, so merely check that they are non-zero. + *) + NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "null"]; + + let bs1 = NBD.stats_bytes_sent nbd in + let cs1 = NBD.stats_chunks_sent nbd in + let br1 = NBD.stats_bytes_received nbd in + let cr1 = NBD.stats_chunks_received nbd in + assert (cs1 > 0L); + assert (bs1 > cs1); + assert (cr1 > 0L); + assert (br1 > cr1); + + (* A flush command should be one chunk out, one chunk back (even if + * structured replies are in use) + *) + NBD.flush nbd; + + let bs2 = NBD.stats_bytes_sent nbd in + let cs2 = NBD.stats_chunks_sent nbd in + let br2 = NBD.stats_bytes_received nbd in + let cr2 = NBD.stats_chunks_received nbd in + assert (bs2 = (Int64.add bs1 28L)); + assert (cs2 = (Int64.succ cs1)); + assert (br2 = (Int64.add br1 16L)); (* assumes nbdkit uses simple reply *) + assert (cr2 = (Int64.succ cr1)); + + (* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + *) + NBD.shutdown nbd; + + let bs3 = NBD.stats_bytes_sent nbd in + let cs3 = NBD.stats_chunks_sent nbd in + let br3 = NBD.stats_bytes_received nbd in + let cr3 = NBD.stats_chunks_received nbd in + let fudge = if cr2 = cr3 then 0L else 1L in + assert (bs3 > bs2); + assert (cs3 = (Int64.succ cs2)); + assert (br3 >= br2); + assert (cr3 = (Int64.add cr2 fudge)) + +let () = Gc.compact () diff --git a/golang/Makefile.am b/golang/Makefile.am index f170cbc4..765382d4 100644 --- a/golang/Makefile.am +++ b/golang/Makefile.am @@ -1,5 +1,5 @@ # nbd client library in userspace -# Copyright (C) 2013-2020 Red Hat Inc. +# Copyright (C) 2013-2022 Red Hat Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -45,6 +45,7 @@ source_files = \ libnbd_590_aio_copy_test.go \ libnbd_600_debug_callback_test.go \ libnbd_610_error_test.go \ + libnbd_620_stats.go \ $(NULL) generator_built = \ diff --git a/golang/libnbd_620_stats.go b/golang/libnbd_620_stats.go new file mode 100644 index 00000000..8d566198 --- /dev/null +++ b/golang/libnbd_620_stats.go @@ -0,0 +1,181 @@ +/* libnbd golang tests + * Copyright (C) 2013-2022 Red Hat Inc. + * + * 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 + */ + +package libnbd + +import "testing" + +func Test620Stats(t *testing.T) { + h, err := Create() + if err != nil { + t.Fatalf("could not create handle: %s", err) + } + defer h.Close() + + /* Pre-connection, stats start out at 0 */ + bs0, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs0, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br0, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr0, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if bs0 != 0 { + t.Fatalf("unexpected value for bs0") + } + if cs0 != 0 { + t.Fatalf("unexpected value for cs0") + } + if br0 != 0 { + t.Fatalf("unexpected value for br0") + } + if cr0 != 0 { + t.Fatalf("unexpected value for cr0") + } + + /* Connection performs handshaking, which increments stats. + * The number of bytes/chunks here may grow over time as more features + * get automatically negotiated, so merely check that they are non-zero. + */ + err = h.ConnectCommand([]string{ + "nbdkit", "-s", "--exit-with-parent", "null", + }) + if err != nil { + t.Fatalf("%s", err) + } + + bs1, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs1, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br1, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr1, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if cs1 == 0 { + t.Fatalf("unexpected value for cs1") + } + if bs1 <= cs1 { + t.Fatalf("unexpected value for bs1") + } + if cr1 == 0 { + t.Fatalf("unexpected value for cr1") + } + if br1 <= cr1 { + t.Fatalf("unexpected value for br1") + } + + /* A flush command should be one chunk out, one chunk back (even if + * structured replies are in use) + */ + err = h.Flush(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs2, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs2, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br2, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr2, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if bs2 != bs1 + 28 { + t.Fatalf("unexpected value for bs2") + } + if cs2 != cs1 + 1 { + t.Fatalf("unexpected value for cs2") + } + if br2 != br1 + 16 { /* assumes nbdkit uses simple reply */ + t.Fatalf("unexpected value for br2") + } + if cr2 != cr1 + 1 { + t.Fatalf("unexpected value for cr2") + } + + /* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + */ + err = h.Shutdown(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs3, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs3, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br3, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr3, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + slop := uint64(1) + if br2 == br3 { + slop = uint64(0) + } + + if bs3 <= bs2 { + t.Fatalf("unexpected value for bs3") + } + if cs3 != cs2 + 1 { + t.Fatalf("unexpected value for cs3") + } + if br3 < br2 { + t.Fatalf("unexpected value for br3") + } + if cr3 != cr2 + slop { + t.Fatalf("unexpected value for cr3") + } +} -- 2.37.2
So the patch series is OK, modulo my comments. But I think it'd be better not to bind our future selves to complicated guarantees about what are basically internal details of the library and protocol. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Sat, Sep 03, 2022 at 05:32:23PM +0100, Richard W.M. Jones wrote:
So the patch series is OK, modulo my comments.
I've made tweaks according to your requests.
But I think it'd be better not to bind our future selves to complicated guarantees about what are basically internal details of the library and protocol.
This, plus the 2 patches to see nbd_stats_ in LIBNBD_DEBUG=1 output, are now in as: 043d430c..0c0082c6 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/03/22 00:14, Eric Blake wrote:
This proves that the stat counters increment as desired, as well as proving that our RUInt32 generator type works.
How is this related to RUint32?
This commit is also a showcase of whether I can do 64-bit math in various languages (C's terseness in 'a == b + (c > d)' is annoying to replicate in languages that don't like playing fast and loose with types). :)
I don't understand; please elaborate. Thanks Laszlo
--- python/t/620-stats.py | 77 +++++++++++++++ ocaml/tests/Makefile.am | 1 + ocaml/tests/test_620_stats.ml | 77 +++++++++++++++ golang/Makefile.am | 3 +- golang/libnbd_620_stats.go | 181 ++++++++++++++++++++++++++++++++++ 5 files changed, 338 insertions(+), 1 deletion(-) create mode 100644 python/t/620-stats.py create mode 100644 ocaml/tests/test_620_stats.ml create mode 100644 golang/libnbd_620_stats.go
diff --git a/python/t/620-stats.py b/python/t/620-stats.py new file mode 100644 index 00000000..62f8443f --- /dev/null +++ b/python/t/620-stats.py @@ -0,0 +1,77 @@ +# libnbd Python bindings +# Copyright (C) 2010-2022 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import nbd + +h = nbd.NBD() + +# Pre-connection, stats start out at 0 +bs0 = h.stats_bytes_sent() +cs0 = h.stats_chunks_sent() +br0 = h.stats_bytes_received() +cr0 = h.stats_chunks_received() + +assert bs0 == 0 +assert cs0 == 0 +assert br0 == 0 +assert cr0 == 0 + +# Connection performs handshaking, which increments stats. +# The number of bytes/chunks here may grow over time as more features get +# automatically negotiated, so merely check that they are non-zero. +h.connect_command(["nbdkit", "-s", "--exit-with-parent", "null"]) + +bs1 = h.stats_bytes_sent() +cs1 = h.stats_chunks_sent() +br1 = h.stats_bytes_received() +cr1 = h.stats_chunks_received() + +assert cs1 > 0 +assert bs1 > cs1 +assert cr1 > 0 +assert br1 > cr1 + +# A flush command should be one chunk out, one chunk back (even if +# structured replies are in use) +h.flush() + +bs2 = h.stats_bytes_sent() +cs2 = h.stats_chunks_sent() +br2 = h.stats_bytes_received() +cr2 = h.stats_chunks_received() + +assert bs2 == bs1 + 28 +assert cs2 == cs1 + 1 +assert br2 == br1 + 16 # assumes nbdkit uses simple reply +assert cr2 == cr1 + 1 + +# Stats are still readable after the connection closes; we don't know if +# the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. +h.shutdown() + +bs3 = h.stats_bytes_sent() +cs3 = h.stats_chunks_sent() +br3 = h.stats_bytes_received() +cr3 = h.stats_chunks_received() + +assert bs3 > bs2 +assert cs3 == cs2 + 1 +assert br3 >= br2 +assert cr3 == cr2 + (br3 > br2) + +# Try to trigger garbage collection of h +h = None diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am index 22cefb4d..c4751ad3 100644 --- a/ocaml/tests/Makefile.am +++ b/ocaml/tests/Makefile.am @@ -42,6 +42,7 @@ ML_TESTS = \ test_590_aio_copy.ml \ test_600_debug_callback.ml \ test_610_exception.ml \ + test_620_stats.ml \ $(NULL)
EXTRA_DIST = $(ML_TESTS) diff --git a/ocaml/tests/test_620_stats.ml b/ocaml/tests/test_620_stats.ml new file mode 100644 index 00000000..648096fd --- /dev/null +++ b/ocaml/tests/test_620_stats.ml @@ -0,0 +1,77 @@ +(* hey emacs, this is OCaml code: -*- tuareg -*- *) +(* libnbd OCaml test case + * Copyright (C) 2013-2022 Red Hat Inc. + * + * 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 + *) + +let () = + let nbd = NBD.create () in + + (* Pre-connection, stats start out at 0 *) + let bs0 = NBD.stats_bytes_sent nbd in + let cs0 = NBD.stats_chunks_sent nbd in + let br0 = NBD.stats_bytes_received nbd in + let cr0 = NBD.stats_chunks_received nbd in + assert (bs0 = 0L); + assert (cs0 = 0L); + assert (br0 = 0L); + assert (cr0 = 0L); + + (* Connection performs handshaking, which increments stats. + * The number of bytes/chunks here may grow over time as more features get + * automatically negotiated, so merely check that they are non-zero. + *) + NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "null"]; + + let bs1 = NBD.stats_bytes_sent nbd in + let cs1 = NBD.stats_chunks_sent nbd in + let br1 = NBD.stats_bytes_received nbd in + let cr1 = NBD.stats_chunks_received nbd in + assert (cs1 > 0L); + assert (bs1 > cs1); + assert (cr1 > 0L); + assert (br1 > cr1); + + (* A flush command should be one chunk out, one chunk back (even if + * structured replies are in use) + *) + NBD.flush nbd; + + let bs2 = NBD.stats_bytes_sent nbd in + let cs2 = NBD.stats_chunks_sent nbd in + let br2 = NBD.stats_bytes_received nbd in + let cr2 = NBD.stats_chunks_received nbd in + assert (bs2 = (Int64.add bs1 28L)); + assert (cs2 = (Int64.succ cs1)); + assert (br2 = (Int64.add br1 16L)); (* assumes nbdkit uses simple reply *) + assert (cr2 = (Int64.succ cr1)); + + (* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + *) + NBD.shutdown nbd; + + let bs3 = NBD.stats_bytes_sent nbd in + let cs3 = NBD.stats_chunks_sent nbd in + let br3 = NBD.stats_bytes_received nbd in + let cr3 = NBD.stats_chunks_received nbd in + let fudge = if cr2 = cr3 then 0L else 1L in + assert (bs3 > bs2); + assert (cs3 = (Int64.succ cs2)); + assert (br3 >= br2); + assert (cr3 = (Int64.add cr2 fudge)) + +let () = Gc.compact () diff --git a/golang/Makefile.am b/golang/Makefile.am index f170cbc4..765382d4 100644 --- a/golang/Makefile.am +++ b/golang/Makefile.am @@ -1,5 +1,5 @@ # nbd client library in userspace -# Copyright (C) 2013-2020 Red Hat Inc. +# Copyright (C) 2013-2022 Red Hat Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -45,6 +45,7 @@ source_files = \ libnbd_590_aio_copy_test.go \ libnbd_600_debug_callback_test.go \ libnbd_610_error_test.go \ + libnbd_620_stats.go \ $(NULL)
generator_built = \ diff --git a/golang/libnbd_620_stats.go b/golang/libnbd_620_stats.go new file mode 100644 index 00000000..8d566198 --- /dev/null +++ b/golang/libnbd_620_stats.go @@ -0,0 +1,181 @@ +/* libnbd golang tests + * Copyright (C) 2013-2022 Red Hat Inc. + * + * 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 + */ + +package libnbd + +import "testing" + +func Test620Stats(t *testing.T) { + h, err := Create() + if err != nil { + t.Fatalf("could not create handle: %s", err) + } + defer h.Close() + + /* Pre-connection, stats start out at 0 */ + bs0, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs0, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br0, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr0, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if bs0 != 0 { + t.Fatalf("unexpected value for bs0") + } + if cs0 != 0 { + t.Fatalf("unexpected value for cs0") + } + if br0 != 0 { + t.Fatalf("unexpected value for br0") + } + if cr0 != 0 { + t.Fatalf("unexpected value for cr0") + } + + /* Connection performs handshaking, which increments stats. + * The number of bytes/chunks here may grow over time as more features + * get automatically negotiated, so merely check that they are non-zero. + */ + err = h.ConnectCommand([]string{ + "nbdkit", "-s", "--exit-with-parent", "null", + }) + if err != nil { + t.Fatalf("%s", err) + } + + bs1, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs1, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br1, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr1, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if cs1 == 0 { + t.Fatalf("unexpected value for cs1") + } + if bs1 <= cs1 { + t.Fatalf("unexpected value for bs1") + } + if cr1 == 0 { + t.Fatalf("unexpected value for cr1") + } + if br1 <= cr1 { + t.Fatalf("unexpected value for br1") + } + + /* A flush command should be one chunk out, one chunk back (even if + * structured replies are in use) + */ + err = h.Flush(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs2, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs2, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br2, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr2, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + + if bs2 != bs1 + 28 { + t.Fatalf("unexpected value for bs2") + } + if cs2 != cs1 + 1 { + t.Fatalf("unexpected value for cs2") + } + if br2 != br1 + 16 { /* assumes nbdkit uses simple reply */ + t.Fatalf("unexpected value for br2") + } + if cr2 != cr1 + 1 { + t.Fatalf("unexpected value for cr2") + } + + /* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + */ + err = h.Shutdown(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs3, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs3, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br3, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr3, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + slop := uint64(1) + if br2 == br3 { + slop = uint64(0) + } + + if bs3 <= bs2 { + t.Fatalf("unexpected value for bs3") + } + if cs3 != cs2 + 1 { + t.Fatalf("unexpected value for cs3") + } + if br3 < br2 { + t.Fatalf("unexpected value for br3") + } + if cr3 != cr2 + slop { + t.Fatalf("unexpected value for cr3") + } +}
On Mon, Sep 05, 2022 at 02:10:36PM +0200, Laszlo Ersek wrote:
On 09/03/22 00:14, Eric Blake wrote:
This proves that the stat counters increment as desired, as well as proving that our RUInt32 generator type works.
How is this related to RUint32?
Typo in the commit message; this should be RUInt64. Oh well, the commit is already in.
This commit is also a showcase of whether I can do 64-bit math in various languages (C's terseness in 'a == b + (c > d)' is annoying to replicate in languages that don't like playing fast and loose with types). :)
I don't understand; please elaborate.
Sure. In C, uint64_t == uint64_t + bool is legal; making for a nice terse expression.
+++ b/python/t/620-stats.py
+# Stats are still readable after the connection closes; we don't know if +# the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. +h.shutdown() + +bs3 = h.stats_bytes_sent() +cs3 = h.stats_chunks_sent() +br3 = h.stats_bytes_received() +cr3 = h.stats_chunks_received() + +assert bs3 > bs2 +assert cs3 == cs2 + 1 +assert br3 >= br2 +assert cr3 == cr2 + (br3 > br2)
And in python.
+++ b/ocaml/tests/test_620_stats.ml + (* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + *) + NBD.shutdown nbd; + + let bs3 = NBD.stats_bytes_sent nbd in + let cs3 = NBD.stats_chunks_sent nbd in + let br3 = NBD.stats_bytes_received nbd in + let cr3 = NBD.stats_chunks_received nbd in + let fudge = if cr2 = cr3 then 0L else 1L in + assert (bs3 > bs2); + assert (cs3 = (Int64.succ cs2)); + assert (br3 >= br2); + assert (cr3 = (Int64.add cr2 fudge))
Not so in OCaml, where + isn't even polymorphic to int64, so I have to break out manual calls to Int64.XXX methods. Here, I went with a helper variable 'fudge'.
+++ b/golang/libnbd_620_stats.go + /* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + */ + err = h.Shutdown(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs3, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs3, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br3, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr3, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + slop := uint64(1) + if br2 == br3 { + slop = uint64(0) + } + + if bs3 <= bs2 { + t.Fatalf("unexpected value for bs3") + } + if cs3 != cs2 + 1 { + t.Fatalf("unexpected value for cs3") + } + if br3 < br2 { + t.Fatalf("unexpected value for br3") + } + if cr3 != cr2 + slop { + t.Fatalf("unexpected value for cr3") + }
Nor in Go, where you can't even do uint64(bool), but HAVE to use an 'if' statement to populate 'slop' with the correct value. Hmm, maybe I should have used the same variable name between OCaml and Go, instead of 'slop'/'fudge'; oh well. (And Go is painfully verbose in the amount of boilerplate formatting it requires, compared to the other languages; although I will be the first to admit that I'm probably not an idiomatic Go coder) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Sep 06, 2022 at 04:19:40AM -0500, Eric Blake wrote:
+ assert (cr3 = (Int64.add cr2 fudge))
Not so in OCaml, where + isn't even polymorphic to int64, so I have to break out manual calls to Int64.XXX methods. Here, I went with a helper variable 'fudge'.
No implicit type conversions in OCaml! (as a deliberate choice) You don't need to the parens around Int64.add since function application always binds tightest. You could have written this if you'd wanted: let (+^) = Int64.add ... assert (cr3 = cr2 +^ fudge) Custom operators always have the same precedence as the normal operator with the same first character. Or in recent OCaml versions: assert Int64.(cr3 = cr2 + fudge) 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 09/06/22 11:19, Eric Blake wrote:
On Mon, Sep 05, 2022 at 02:10:36PM +0200, Laszlo Ersek wrote:
On 09/03/22 00:14, Eric Blake wrote:
This proves that the stat counters increment as desired, as well as proving that our RUInt32 generator type works.
How is this related to RUint32?
Typo in the commit message; this should be RUInt64. Oh well, the commit is already in.
This commit is also a showcase of whether I can do 64-bit math in various languages (C's terseness in 'a == b + (c > d)' is annoying to replicate in languages that don't like playing fast and loose with types). :)
I don't understand; please elaborate.
Sure. In C, uint64_t == uint64_t + bool is legal; making for a nice terse expression.
I interpreted your statement about 64-bit math and the formula a == b + (c > d) as if the formula were some means to implement *64-bit-safe* math (addition, overflow etc something like that). I understood how the expression worked at the lowest level, but didn't understand what it was good for -- what it expressed. The more concrete form is cr3 == cr2 + (br3 > br2) which seems to stand for: "chunks received #3" should equal "chunks received #2, plus one chunk, if we received additional bytes (in br3) since setting br2". Basically the section +bs2 = h.stats_bytes_sent() +cs2 = h.stats_chunks_sent() +br2 = h.stats_bytes_received() +cr2 = h.stats_chunks_received() + +[...] + +# Stats are still readable after the connection closes; we don't know if +# the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. +h.shutdown() + +bs3 = h.stats_bytes_sent() +cs3 = h.stats_chunks_sent() +br3 = h.stats_bytes_received() +cr3 = h.stats_chunks_received() + +assert bs3 > bs2 +assert cs3 == cs2 + 1 +assert br3 >= br2 +assert cr3 == cr2 + (br3 > br2) was a bit too hard for me to parse. I didn't notice the role of h.shutdown() before. Acked-by: Laszlo Ersek <lersek@redhat.com>
+++ b/python/t/620-stats.py
+# Stats are still readable after the connection closes; we don't know if +# the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. +h.shutdown() + +bs3 = h.stats_bytes_sent() +cs3 = h.stats_chunks_sent() +br3 = h.stats_bytes_received() +cr3 = h.stats_chunks_received() + +assert bs3 > bs2 +assert cs3 == cs2 + 1 +assert br3 >= br2 +assert cr3 == cr2 + (br3 > br2)
And in python.
+++ b/ocaml/tests/test_620_stats.ml + (* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + *) + NBD.shutdown nbd; + + let bs3 = NBD.stats_bytes_sent nbd in + let cs3 = NBD.stats_chunks_sent nbd in + let br3 = NBD.stats_bytes_received nbd in + let cr3 = NBD.stats_chunks_received nbd in + let fudge = if cr2 = cr3 then 0L else 1L in + assert (bs3 > bs2); + assert (cs3 = (Int64.succ cs2)); + assert (br3 >= br2); + assert (cr3 = (Int64.add cr2 fudge))
Not so in OCaml, where + isn't even polymorphic to int64, so I have to break out manual calls to Int64.XXX methods. Here, I went with a helper variable 'fudge'.
+++ b/golang/libnbd_620_stats.go + /* Stats are still readable after the connection closes; we don't know if + * the server sent reply bytes to our NBD_CMD_DISC, so don't insist on it. + */ + err = h.Shutdown(nil) + if err != nil { + t.Fatalf("%s", err) + } + + bs3, err := h.StatsBytesSent() + if err != nil { + t.Fatalf("%s", err) + } + cs3, err := h.StatsChunksSent() + if err != nil { + t.Fatalf("%s", err) + } + br3, err := h.StatsBytesReceived() + if err != nil { + t.Fatalf("%s", err) + } + cr3, err := h.StatsChunksReceived() + if err != nil { + t.Fatalf("%s", err) + } + slop := uint64(1) + if br2 == br3 { + slop = uint64(0) + } + + if bs3 <= bs2 { + t.Fatalf("unexpected value for bs3") + } + if cs3 != cs2 + 1 { + t.Fatalf("unexpected value for cs3") + } + if br3 < br2 { + t.Fatalf("unexpected value for br3") + } + if cr3 != cr2 + slop { + t.Fatalf("unexpected value for cr3") + }
Nor in Go, where you can't even do uint64(bool), but HAVE to use an 'if' statement to populate 'slop' with the correct value. Hmm, maybe I should have used the same variable name between OCaml and Go, instead of 'slop'/'fudge'; oh well. (And Go is painfully verbose in the amount of boilerplate formatting it requires, compared to the other languages; although I will be the first to admit that I'm probably not an idiomatic Go coder)
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones