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

[mpd,db] MPD protocol fixes to handling of idle/noidle command and co… #1612

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

geneticdrift
Copy link

…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.

…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
Copy link
Member

Thanks for your contribution. The PR is too large, can you please split it? Start with the fix for the line thing. Also you need to adjust the code style to match the project. E.g. no mid function declarations.

@geneticdrift
Copy link
Author

My apologies. I understand why it would have been more convenient to have a step by step smaller commits. But please understand that it's a single ~5000 LOC c file and most of the changes are interdependent.
As I'm not an owntone user myself and did not intend to spend too much time on it, I'll leave it as an extended issue report and a proposed fix instead of a pull request in case a future maintainer find it useful.

@ejurgensen
Copy link
Member

Completely understandable. In that case thanks a million for taking the time to look into the code and fix these issues. I will see if I can take it from here and do the necessary fixups, so it can be merged.

@ejurgensen
Copy link
Member

I had a look at this to see if I could do the remaining work. One question regarding your changes: You removed mpd_command_noidle() and thus deleted this logic:

  if (ctx->events)
    mpd_notify_idle_client(ctx, ctx->events);

Can you explaing why?

@geneticdrift
Copy link
Author

Based on MPD source code, noidle doesn't notify pending events, it just leaves idle mode and sends OK.

https://github.com/MusicPlayerDaemon/MPD/blob/9c19368fc789fa7101110f73b6ffae8503ae326d/src/client/Process.cxx#L54C1-L67C1

@ejurgensen
Copy link
Member

Ok, I'm weary about removing it though. There must have been a reason @chme added this logic. Maybe this commit explains it, but I can't say I really understand it.

…state.

mpd_notify_idle_client returns 0 if it sent the idle response to the client.
@geneticdrift
Copy link
Author

It looks like MPD documentation and source code are in contradiction.

The idle command can be canceled by sending the command noidle (no other commands are allowed). MPD will then leave idle mode and print results immediately; might be empty at this time.

I don't think it's a problem because idle events are asynchronous anyway so clients can't rely on noidle returning changes.

But if you want to add this behavior see the added commit: 0e39b88.

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

Successfully merging this pull request may close these issues.

None yet

2 participants