[alsa-devel] [PATCH 1/2] New Control Plugin - arcam-av

[Takashi Iwai]

At Sun, 4 Jan 2009 12:37:11 +0000,
Peter Stokes wrote:
> 
> --- alsa-plugins-1.0.18-orig/arcam-av/arcam_av.c	1970-01-01 01:00:00.000000000 
> +0100
> +++ alsa-plugins-1.0.18/arcam-av/arcam_av.c	2009-01-02 22:30:55.000000000 
> +0000

Looks like your embedded patches are broken due to line-breaks by MUA.
Please fix it, or use attachments if it's difficult.

> +int arcam_av_connect(const char* port)
> +{
> +	int fd = open(port, O_RDWR | O_NOCTTY);
> +	if (fd < 0)
> +		return -errno;
> +
> +	struct termios portsettings;

Put this into the beginning to follow the standard C.

> +int arcam_av_send(int fd, arcam_av_cc_t command, unsigned char param1, 
> unsigned char param2)
> +{
> +	char buffer[7] = {'P', 'C', '_', command, param1, param2, 0x0D};
> +
> +	tcdrain(fd);
> +	ssize_t bytes = write(fd, buffer, 7);

Ditto.

> +	if (bytes == 7)
> +		return 0;
> +	else if (bytes >= 0)
> +		return -1;

No proper error code?

> +static int arcam_av_receive(int fd, arcam_av_cc_t* command, unsigned char* 
> param1, unsigned char* param2)
> +{
> +	static int index = 0;
> +	static arcam_av_cc_t received_command;
> +	static unsigned char received_param1;
> +	static unsigned char received_param2;

Are static variables safe to use here?
That is, is the plugin designed to access exclusively with a proper
protection?

> +
> +	do {
> +		static char buffer[8];

Ditto.

> +		ssize_t bytes = read(fd, buffer, sizeof buffer - index);
> +
> +		if (bytes <= 0)
> +			return -errno;
> +
> +		char* cursor = buffer;

Don't define a variable in the middle.

Well, I'll review the rest after my vacation is over...


thanks,

Takashi