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] add binarylimit command (still unused) #1791

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

grobian
Copy link
Contributor

@grobian grobian commented Aug 12, 2024

Maintain the binary limit, and use the default MPD uses too. This comes in handy when we are going to send binary responses such as in PR#1780

src/mpd.c Outdated
{
unsigned int size;

if (argc <= 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just logs an error message, no error is returned to the client. Shouldn't it be implemented like in e.g. mpd_command_consume? I also notice that the other handlers don't check argc. Instead min_argc is set in the handler struct (you are setting it to -1, but I think it should be 2).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right I copied probably not the best example function

@ejurgensen
Copy link
Member

As you write the limit isn't used, but are there any of the existing commands where it should be used? I looked through the list of supported commands, and I didn't notice any that looked like they would have binary responses. Is that also your view?

@grobian
Copy link
Contributor Author

grobian commented Aug 12, 2024

correct albumart is the first to use BINARY responose

because I cannot test it here, I will write a function that pushes out something in the chunks as necessary for reuse in the albumart code

argv[1]);
return ACK_ERROR_ARG;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that for values < 64 bytes mpd errors with "Value too small". See ClientCommands.cxx. Maybe do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sensical, else stuff gets very inefficient – or even worse – we end up in an endless loop (size=0)

Maintain the binary limit, and use the default MPD uses too.  This comes
in handy when we are going to send binary responses such as in PR owntone#1780

Signed-off-by: Fabian Groffen <grobian@gentoo.org>
Signed-off-by: Fabian Groffen <grobian@gentoo.org>
@ejurgensen ejurgensen merged commit 282e227 into owntone:master Aug 13, 2024
4 checks passed
@ejurgensen
Copy link
Member

Merged, thanks!

@grobian grobian deleted the mpd-0.22.4 branch August 13, 2024 17:11
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.

2 participants