libsubprocess: improve read API #5965
Conversation
|
We probably shouldn't mix the internal refactoring, external API change, and variable renames in one big commit. Also, are we sure it is necessary to change the signature of every read function? I was only advocating for changing |
I could splice out the variable renames, but the internal refactoring sort of comes with the change because ...
I did this because A) the internal code of all the functions shares the same internals B) as I was working on the UNBUF_LOCAL change, I realized it's awkward to return a pointer on success when there is no line to be read. You're just trusting the user to not read anything from the pointer. As an example, I really disliked a lot of these calls vs now it's just which i like better. 🤷 |
chu11
force-pushed
the
issue5953_libsubprocess_update_functions
branch
from
3061932
to
7be8321
Compare
|
re-pushed, spliting out the variable renames, it does clean up the big commit alot |
|
Well, you make a good point! |
| @@ -244,7 +244,7 @@ static void stdio_cb (flux_subprocess_t *p, const char *stream) | |||
| const char *line; | |||
There was a problem hiding this comment.
commit message and PR title: I'd probably call this "improve read API" since it's really about the API change not refactoring.
There was a problem hiding this comment.
good point, re-pushed with that tweaked commit message
chu11
changed the title
chu11
force-pushed
the
issue5953_libsubprocess_update_functions
branch
from
7be8321
to
f6d512b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5965 +/- ##
==========================================
- Coverage 83.33% 83.32% -0.02%
==========================================
Files 515 515
Lines 83398 83396 -2
==========================================
- Hits 69502 69487 -15
- Misses 13896 13909 +13
|
| @@ -244,7 +244,7 @@ static void stdio_cb (flux_subprocess_t *p, const char *stream) | |||
| const char *line; | |||
There was a problem hiding this comment.
Commit message nit: extra space after Solution:
chu11
force-pushed
the
issue5953_libsubprocess_update_functions
branch
from
f6d512b
to
7dafb65
Compare
Problem: An error message in subprocess_standard_output() indicated the wrong function. Correct the function name in the error message.
Problem: In the near future the API for several libsubprocess functions will change. As a result, some variable names that were previously used may not be the most useful/meaningful afterwards. In calls to flux_subprocess_read() and similar functions, use the variable "buf" instead of the generic "ptr" when referring the buffer of data returned. Use the variable "len" instead of "lenp" for the data length.
Problem: The flux_subprocess_read() family of functions return a pointer to a buffer on success and length of buffer in an optional parameter. This API style isn't the most intuitive as it is quite different than the read(2) system call. Most notably when EOF is reached on a read stream you have to check if the buffer pointer is non-NULL and the length returned from an optional parameter is 0. Solution: Update the read family of functions to return the length from the function and the buffer pointer in an optional parameter. Update all callers accordingly. Fixes flux-framework#5953
chu11
force-pushed
the
issue5953_libsubprocess_update_functions
branch
from
7dafb65
to
c436b85
Compare
|
thanks, rebased & re-pushed after fixing that commit message nit. will set MWP |
Problem: The flux_subprocess_read() family of functions return a pointer to a buffer on success and length of buffer in an
optional parameter.
This API style isn't the most intuitive as it is quite different than the read(2) system call. Most notably when EOF
is reached on a read stream you have to check if the buffer pointer is non-NULL and the length returned from an optional parameter is 0.
Solution: Update the read family of functions to return the length from the function and the buffer pointer in an optional parameter.
Update all callers accordingly.
side notes
notes, b/c so much code already used
ints andchar *s for these functions, I elected to keepintvsssize_tandchar *vsvoid *.Relatively simple refactoring ... sorry but the review will be a little tedious, lots of adjustments everywhere :-)