MPD noidle and command lists (refactored version of PR #1612) #1794
Conversation
|
@grobian if you are up to testing these changes it would be great. I don't use the mpd part of OwnTone regularly so I can't properly test. |
| #include <sys/param.h> | ||
| #include <sys/queue.h> | ||
| #include <sys/types.h> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
doesn't this belong in its own commit as "cleanup"?
src/mpd.c
Outdated
| */ | ||
| #define CLIENT_MAX_COMMAND_LIST_DEFAULT (2048*1024) | ||
|
|
||
| static struct { |
There was a problem hiding this comment.
this is an anonymous struct for a single member, is it expected to contain more in the future? How about the current static vars such as allow_modifying_stored_playlists, shouldn't those be in this structure too (as a separate commit)?
Also, probably better to declare this structure named, and then define mpd_config as static variable.
(Config is very generic easy to clash, and IMO inconsistent with the rest of the file
src/mpd.c
Outdated
| { | ||
| int ret; | ||
|
|
||
| if (strchr(range, ':')) | ||
| static char separator = ':'; |
There was a problem hiding this comment.
yeah... why not #define MPD_SEPARATOR ':' instead
src/mpd.c
Outdated
| DPRINTF(E_LOG, L_MPD, "Error parsing range argument '%s' (return code = %d)\n", range, ret); | ||
| return -1; | ||
| } | ||
| *sep_pos++ = '\0'; |
There was a problem hiding this comment.
I've learned in another PR that this is preferred to be two operations instead of one
|
@ejurgensen to say something about this it needs to be more pinned down to commits that do something, at least for me. I guess now it's just running clients against it and YOLO. I've been playing with the thought to add some testsuite script, because while testing I noticed various things not working properly or not at all, and with something like https://mpd.readthedocs.io/en/latest/protocol.html#id34 in hand, it should be able to craft small sequence-based tests and verified outputs. Client compatibility I don't know how to address in a test suite, but having at least something here would be a start. |
|
Thanks for actually looking at the code, I didn't expect that. However, it seems you looked at the commits one at a time, so some of your comments are outdated (as Github also shows).
Indeed it is :-) I think a test suite would be fine, but then an actual C test framework needs to be chosen and added, and tbh I never bothered to do that (and in my defense, nor did the previous maintainers). |
|
No need to overdo it, just verifying the protocol is a good start. Owntone doesn't even work with mpc, that's why I'm trying to implement the features leading up to a recent protocol version. This commit conflicts with that effort via my PRs, so I think we're looking at what you're going to do next eagerly to see who needs to move in what direction. |
Seems to work ok on my computer with mpc 0.35. What problem are you seeing with mpc?
I'll try to merge your 0.23 changes first, and then fix conflicts with this branch |
|
on (a slightly tinkered of) the released version of owntone: |
|
Ok, it wasn't clear that you were referring to the release version |
ejurgensen
force-pushed
the
mpdprotocol3
branch
2 times, most recently
from
be550db to
f359927
Compare
…mmand list. Command handling: 1. Changed mpd_read_cb() to delegate to mpd_process_line() for each received command line. 2. mpd_process_line() handles idle state and command list state and delegates to mpd_process_command() to handle each command line. If the command was successful it sends OK to the client according the the command list state. Error responses are sent by mpd_process_command(). 3. mpd_process_command() parses the args and delegates to the individual command handler. mpd_input_filter: 1. Removed handling of command lists. They are handled by mpd_process_line(). 2. Return BEV_OK if there's at least one complete line of input even if there's more data in the input buffer. Idle/noidle: 1. Changed mpd_command_idle() to never write OK to the output buffer. Instead it is the responsibility of the caller to decide on the response. 2. Removed mpd_command_noidle() instead it is handled in mpd_process_line(). If the client is not in idle state noidle is ignored (no response sent) If the client is in idle state then it changes idle state to false and sends OK as the response to the idle command. Command lists: 1. Added command list state to the client context so commands in the list are buffered and only executed after receiving command_list_end. Connection state: 1. Added is_closing flag in the client context to ignore messages received after freeing the events buffer in intent to close the client connection. Command arguments parsing: 1. Updated COMMAND_ARGV_MAX to 70 to match current MPD. 2. Changed mpd_pars_range_arg to handle open-ended range. Command pause: 1. pause is ignored in stopped state instead returning error. Command move: 1. Changed mpd_command_move() to support moving a range. 2. Added db_queue_move_bypos_range() to support moving a range. Command password: 1. Password authentication flag set in handler mpd_command_password() instead of in command processor. Config: 1. Added config value: "max_command_list_size". The maximum allowed size of buffered commands in command list.
ejurgensen
force-pushed
the
mpdprotocol3
branch
from
494adc5 to
d40b8ab
Compare
Plus honor qp->order in queue_enum_start() if set by caller
ejurgensen
force-pushed
the
mpdprotocol3
branch
from
1390b58 to
1b28dfb
Compare
Some of the misc: * Initialize query_params from the start of each function * Reduce code duplication by consolidating the handler's integer conversion * Go back to classic int return types for handlers * Change list grouping to respond like mpd * Sanitize all user string output
ejurgensen
force-pushed
the
mpdprotocol3
branch
from
1b28dfb to
f37ca52
Compare
No description provided.