Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add newline character strip flag to flux_buffer_read_line() #1624

Closed
chu11 opened this issue Aug 22, 2018 · 3 comments
Closed

add newline character strip flag to flux_buffer_read_line() #1624

chu11 opened this issue Aug 22, 2018 · 3 comments

Comments

@chu11
Copy link
Member

chu11 commented Aug 22, 2018

While working on #1331, it occurred to me that dumping data from a buffer to a log was common. By default, flux_buffer_read_line() returns a buffer with the newline character at the end, which should be stripped before dumping into a log.

It would be convenient if the library had an option to automatically strip it for you. I'm imagining just a boolean flag, so a flux_buffer_read_line (flux_buffer_t *b, int *len, bool newline) prototype.

The flag would have to be propogated into APIs that call this too of course.

@grondo
Copy link
Contributor

grondo commented Aug 22, 2018

Not that this isn't a good idea, but since e7fd48c flux_log() at least splits log messages at newlines and strips any trailing newlines for you.

Just my opinion, but I'm not a fan of the extra argument to flux_buffer_read_line, which would then have to be propagated throughout the API as you mention. I'd slightly prefer a flux_buffer_trim() which would nullify all trailing whitespace in a flux_buffer_t. Would something like that be acceptable?

@chu11
Copy link
Member Author

chu11 commented Aug 22, 2018

Ohh, I wasn't even aware flux_log() did that. I saw this newline stripping code in cron.

    char *p, *saveptr = NULL;                                                                                                          
    while ((p = strtok_r (s, "\n", &saveptr))) {                                                                                       
        flux_log (h, level, "cron-%ju[%s]: rank=%d: command=\"%s\": \"%s\"",                                                           
                e->id, e->name, e->rank, e->command, p);                                                                               
        s = NULL;                                                                                                                      
    }  

which I guess is unnecessary.

Not sure a flux_buffer_trim() would work, b/c what if there are multiple lines in the buffer? Perhaps some type of flag option for the whole buffer would be best.

But for the mean time, this isn't important since flux_log() takes care of these things.

@grondo
Copy link
Contributor

grondo commented Aug 22, 2018

Not sure a flux_buffer_trim() would work, b/c what if there are multiple lines in the buffer? Perhaps some type of flag option for the whole buffer would be best.

Oh yeah, sorry I wasn't thinking when I typed that. I was forgetting how the flux_buffer_read_line was being used -- and the returned buffer is const, so can't be easily modified in place by a simplistic trim function.

In the future we could add a flag to the buffer as a whole maybe, or a separate function flux_buffer_read_trimmed_line() -- I slightly prefer that to a flag on every call. However, this is just one person's opinion.

I saw this newline stripping code in cron.

Yeah, that predates the changes to flux_log() I think, and could be simplified now since flux_log also converts multiple lines to multiple log messages.

chu11 added a commit to chu11/flux-core that referenced this issue Aug 30, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
chu11 added a commit to chu11/flux-core that referenced this issue Aug 31, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
chu11 added a commit to chu11/flux-core that referenced this issue Aug 31, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
chu11 added a commit to chu11/flux-core that referenced this issue Aug 31, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
chu11 added a commit to chu11/flux-core that referenced this issue Aug 31, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
chu11 added a commit to chu11/flux-core that referenced this issue Aug 31, 2018
For convenience, support flux_buffer_peek_trimmed_line() and
flux_buffer_read_trimmed_line() variants, which will read lines
but strip off any trailing newline characters.

Fixes flux-framework#1624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants