buffer: identify messages with length over the buffer limit #54
Conversation
Identify messages that are over the 4KiB buffer limit Address issue reported downstream by Debian[1] and upstream by kaliko (@mxjeff)[2] [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=953110 [2] MusicPlayerDaemon#51
| @@ -115,6 +116,13 @@ mpd_sync_send_command_v(struct mpd_async *async, const struct timeval *tv0, | |||
| success = mpd_async_send_command_v(async, command, copy); | |||
| va_end(copy); | |||
|
|
|||
| /* no characters were written to async buffer */ | |||
| if ((mpd_async_events(async) & MPD_ASYNC_EVENT_WRITE) == 0) { | |||
There was a problem hiding this comment.
This is a bad check for the error.
- the buffer could already have some data but not enough for the new command, thus making this a false positive
- the command could fit, but it failed because there was a previous error, thus overwriting the existing error condition
And it doesn't check the success flag.
There was a problem hiding this comment.
Hello Max Kellermann, thanks for the review.
-
Maybe i'm not understanding correctly, but if the buffer already have some data to write, then
mpd_async_events()will have theMPD_ASYNC_EVENT_WRITEflag set; that is why i did that test. In other words, that test is intended to see if more space will be made available in the buffer by callingmpd_sync_io().
I have updated the comment on line 119 to reflect this. -
This is already done at
mpd_async_set_error(); if an error is already set, it returns false and doesn't change it (mentioned in the declaration of this function in src/iasync.h). If you want another name for this function to reflect this, let me know.
The success flag was an oversight; i've corrected it with a new commit.
new commit is 8f46a3e
|
Fixed merge conflict and folded into one: c383bbb |
Identify messages that are over the 4KiB buffer limit
Address issue reported downstream by Debian[1] and upstream by kaliko
(@mxjeff)[2]
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=953110
[2] #51
Conservative solution for issue #51.
The output is thus:
--> mpc find '('$(for i in $(seq 1 4091); do echo -n "+"; done)')'MPD error: Not enough buffer space for message