libflux: update API to use size_t where appropriate

[garlick]

Problem: some libflux interfaces use the C int type to represent buffer and I/O sizes, but size_t, as an unsigned type with more range, is more appropriate in public interfaces.

Convert the following public API interfaces to use size_t:

   flux_msg_get_payload()
   flux_msg_set_payload()
   flux_request_decode_raw()
   flux_request_encode_raw()
   flux_response_decode_raw()
   flux_response_encode_raw()
   flux_respond_raw()
   flux_event_encode_raw()
   flux_event_decode_raw()
   flux_event_publish_raw()
   flux_rpc_raw()
   flux_rpc_get_raw()
   flux_kvs_lookup_get_raw()
   flux_kvs_txn_put_raw()

Update tests and in-tree users, where appropriate.

[chu11]

haven't skimmed all of it yet, had initial concerns the change could bleed into the KVS related to #6266 but I think it's ok.

Second thought, we'll need changes to flux-sched too? I think flux-coral2 and flux-accounting will probably be ok.

[codecov]

Codecov Report

Attention: Patch coverage is 97.34513% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.61%. Comparing base (7cd6705) to head (038d1d7).

Files with missing lines	Patch %	Lines
src/broker/log.c	96.42%	1 Missing ⚠️
src/cmd/flux-event.c	92.30%	1 Missing ⚠️
src/cmd/flux-kvs.c	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6467      +/-   ##
==========================================
- Coverage   83.61%   83.61%   -0.01%     
==========================================
  Files         524      524              
  Lines       87623    87625       +2     
==========================================
  Hits        73268    73268              
- Misses      14355    14357       +2     

Files with missing lines	Coverage Δ
src/broker/shutdown.c	81.74% <100.00%> (-0.15%)	⬇️
src/broker/trace.c	77.27% <100.00%> (ø)
src/cmd/builtin/content.c	86.08% <100.00%> (ø)
src/cmd/builtin/dmesg.c	95.04% <100.00%> (+0.04%)	⬆️
src/cmd/builtin/dump.c	85.34% <100.00%> (ø)
src/common/libcontent/content.c	96.22% <100.00%> (ø)
src/common/libfilemap/filemap.c	83.06% <100.00%> (ø)
src/common/libfilemap/fileref.c	75.61% <100.00%> (-1.02%)	⬇️
src/common/libflux/event.c	77.66% <100.00%> (ø)
src/common/libflux/flog.c	85.08% <100.00%> (+0.87%)	⬆️
... and 20 more

... and 10 files with indirect coverage changes

[garlick]

This has been reworked a bit but the API facing changes are as originally proposed.

In the previous iteration I had introduced a signed arithmetic error that caused a -1 error return not to be detected in the content stack. So in this iteration, I scrutinized each change more carefully and was a little more conservative. There are probably more places that could internally be converted to use size_t but I didn't go all out in this PR.

[garlick]

This is ready for review in case that wasn't clear :-) No rush of course.

[grondo]

The change in the ABI appears to break both the flux-pmix and flux-sched builds, e.g.:

Were we going to first fix those projects first (and perhaps spot check some others that aren't testing in CI)?

  /usr/src/qmanager/modules/qmanager.cpp: In function ‘void status_request_cb(flux_t*, flux_msg_handler_t*, const flux_msg_t*, void*)’:
  /usr/src/qmanager/modules/qmanager.cpp:291:55: error: cannot convert ‘int*’ to ‘size_t*’ {aka ‘long unsigned int*’}
    291 |     if (flux_rpc_get_raw (f, (const void **)&payload, &len) < 0) {
        |                                                       ^~~~
        |                                                       |
        |                                                       int*
  In file included from /usr/include/flux/core/flux.h:24,
                   from /usr/include/flux/core.h:18,
                   from /usr/src/qmanager/modules/qmanager.cpp:15:
  /usr/include/flux/core/rpc.h:59:68: note:   initializing argument 3 of ‘int flux_rpc_get_raw(flux_future_t*, const void**, size_t*)’
     59 | int flux_rpc_get_raw (flux_future_t *f, const void **data, size_t *len);

[garlick]

Sorry, yes, I'll get PRs up for the failing projects and give an audit to the others.

[trws]

In the previous iteration I had introduced a signed arithmetic error that caused a -1 error return not to be detected in the content stack.

I'm only glancing at this, have an SI checkin in a few minutes, but came here after seeing the switching to size_t title to ask if we might be better off with ssize_t, and this quote especially makes me think the answer might be yes. I wont bore you all with the details but it's generally considered a mistake in the C++ committee that we use size_t for the sizes of many things rather than using something that can be the same type as difference_type, which must be signed, because it can accidentally hide things like that. It also has a tendency to make almost all loops that use it as a trip count slower (all unsigned int types do, because overflow blocks need to be generated where overflow on signed is UB).

There's so much size_t out there that it doesn't make a huge difference, but if we're making the switch from a signed type it might be worth keeping it signed and making it bigger unless we need deterministic rollover.

[garlick]

Fair point, but I think probably it makes sense to conform with similar usage in the C library and posix here. The unsigned value in these interfaces removes ambiguity about whether a value should be checked for negative for some kind of error/special meaning too.

[garlick]

PRs are up for flux-sched, flux-pmix, and dyad. I didn't spot any others.

[grondo]

Ok, we'll have to rebase this branch, then force a merge (which may require temporary removal of branch protections, I can't remember if the big button will work without that)

[garlick]

Rebased.

[grondo]

LGTM. Adding approval for completeness.

[grondo]

Ok, forcing a merge.

[garlick]

Thanks @grondo! I'll go kick CI in the three other PRs.

[garlick]

OK kicked sched and pmix.

The dyad one was already passing - maybe I patched dead code. No urgency over there I guess.