MPD 0.23 #1793
MPD 0.23 #1793
Conversation
grobian
force-pushed
the
mpd-0.23
branch
2 times, most recently
from
5451bb2
to
ab68118
Compare
|
Please add a description of the changes to the PR |
src/mpd.c
Outdated
| * | ||
| * @param argc Number of arguments in argv | ||
| * @param argv Pointer to the first filter parameter | ||
| * @param exact_match If true, creates filter for exact matches (e. g. find command) otherwise matches substrings (e. g. search command) | ||
| * @param qp Query parameters | ||
| */ | ||
| static int | ||
| parse_filter_window_params(int argc, char **argv, bool exact_match, struct query_params *qp) | ||
| parse_filter_window_params(int argc, char **argv, bool exact_match, struct query_params *qp, int *pos) |
There was a problem hiding this comment.
This function is becoming a bit too complex, can you break it up? Also add the extra arg to the comment above. It would also be great if you could add some examples of real-world client requests to the comment.
There was a problem hiding this comment.
Sorry, but it still looks quite complex to me. Can you share examples of arguments incl expected query_params outcome, so I can better understand what the parser needs to do?
There was a problem hiding this comment.
this function is removed in its entirety in the refactor, sorry
There was a problem hiding this comment.
you'll want to look at c54bb10#diff-725c859767cc777b1a9eafc699eea74762fd7120e4ca19385242210cf61bde48R948
There was a problem hiding this comment.
I did look at that, and I did see it's called mpd_parse_cmd_params(). That's the function that still looks quite complex. I must again ask you to share examples of input and output to understand what the parser needs to do.
There was a problem hiding this comment.
I'm affraid I need help as to what you find complex here. The new function is just a loop over arguments, strcmp to compare each to a certain function, and then a switch-statement to execute the matching function.
On which of these three do you need more information? The function is called by commands such as https://mpd.readthedocs.io/en/latest/protocol.html#command-addid and https://mpd.readthedocs.io/en/latest/protocol.html#command-playlistfind. As you can see, window, position and so on appear in those, and they are the bits being parsed here.
There was a problem hiding this comment.
I didn't ask for an explanation of the function, I can read it just fine. I asked for examples of arguments (input), because I cannot guess that by myself. Here you can see from the RSP protocol what I mean.
|
I've added a refactor of the parse_filter_window_params function, it is quite extensive, so I hope this was more or less what you had in mind. |
Signed-off-by: Fabian Groffen <grobian@gentoo.org>
MPD 0.23 feature allows specifying locations relative to the current item. Signed-off-by: Fabian Groffen <grobian@gentoo.org>
introduce new function mpd_parse_cmd_params that acts as an replacement for parse_filter_window_params and parse_group_params mpd_parse_cmd_params walks over the arguments and calls appropriate parsers for arguments such as filters, window and position arguments all callers for mpd_parse_cmd_params need to provide a params structure that is setup with params_allow to indicate which commands are valid for the current parsing step, which allows it to do the right thing per MPD command Signed-off-by: Fabian Groffen <grobian@gentoo.org>
Signed-off-by: Fabian Groffen <grobian@gentoo.org>
|
I rebased to master and added some explanations and examples of commands how they look and what would be parsed. (FYI, I'm traveling, and may be slow to respond.) |
|
I still don't see any examples, only syntax description from the mpd docs? They don't seem to be completely accurate, for instance "group" can apparently be repeated, but the syntax description doesn't show that. With actual examples I think you would have noticed a problem with filters: According to the docs, pre 0.21 they were "TYPE VALUE", but are now EXPRESSION, where EXPRESSION can for instance be "(Title contains 'name')". The problem is the filter parser doesn't work with that kind of input. It still expects just "TYPE VALUE". So the version upgrade to 0.21 in PR #1790 wasn't complete. When given the new kind of input the parser produces a qp->filter that's "(f.album_artist = '%')", so there also seems to be problem there. When given input it doesn't understand it shouldn't produce that kind of output, qp->filter should just be left unset. There are also some other lesser things I think should be done differently in this PR. All in all, it will require some work, and too much to go back and forth on. So I will look at it myself when I find time. I might set the version back to 0.20 until then. Btw, looking at this I also notice a stack buffer overflow because mpd_parse_args() has no bounds check of the input. That's not a bug in your work, it seems to have been there for quite a while. |
|
I guess I'm not used to your style of documentation, seems you want filled in values in the three examples I included? I thought the explanations would make it clear what the values would be. |
|
Yes, sorry about that, but I think properly supporting the mpd filter EXPRESSION's might require a lexer/parser implementation, which isn't straightforward.
Which specifications do you mean? According to the docs, 0.20 didn't use the EXPRESSION format, it only used TYPE VALUE, and that was supported. |
|
I don't think the format is recursive or requiring backtracking of any kind, and would disagree it requires extensive parsing to process. |
Implement the features required for MPD 0.23 support.
https://mpd.readthedocs.io/en/latest/protocol.html#since-0-23