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 <ler...@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.
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
