[alsa-lib] A suggestion on file cards.c

[alsa-lib] A suggestion on file cards.c

[alex dot baldacchino dot alsasub at gmail dot com]

[-- Attachment #1: Type: text/plain, Size: 5356 bytes --]

Hello,
first of all, let me point out documentation for snd_card_get_index is
incorrect: it is stated that zero is returned in case of success,
instead, a value greater than or equal to zero is the real success
result.

In second place, let me suggest a different implementation.

There's a number of functions (to get index, name, longname of a card;
other ones could be added eventually, as the interface could evolve)
resorting to a round of calls and pointer dereferencing to
snd_ctl_hw_open() and snd_ctl_card_info()/snd_ctl_hw_card_info() to
perform about the same operations as snd_card_load2() (eventually
combined with snd_card_load1() ), potentially in a more dangerous
fashion and possibly causing snd_card_load2() to be called anyway.
Thus, I'm wondering if such calls could be avoided with just a few
(not excessively invasive) changes to snd_card_load2() ( +
snd_card_load1(), if the case, as I think), and I've made an attempt
to implement those functions this way.

Let's consider a few things. Function snd_ctl_hw_open() is called with
the only purpose to allocate and populate an object of type snd_ctl_t,
whose pointer is then passed to snd_ctl_card_info(); the latter one
accesses that object fields to end up calling snd_ctl_hw_card_info(),
whose purpose is to call the same ioctl as snd_card_load2(), in order
to populate an object of type snd_ctl_card_info_t, therefore a call to
a (not too much) modified version of snd_card_load2 could be enough
for the same goals of successive calls to snd_ctl_hw_open() and
snd_ctl_card_info().

This would rise following constraints:

1) Creating an object of type snd_ctl_card_info_t accessible by the
caller after the function (snd_card_load2) returns. This is easy to
accomplish: the caller uses a local variable of that type, thus let's
just add an argument of type snd_ctl_card_info_t * and either use it
in place of the internal (automatic variable) one used by
snd_card_load2(), or choose whether to use the pointer argument (if
not null) or an automatic variable (I've chosen this path).

2) Accessing a device special file corresponding to an integer card
number. More reasoning needed here, since snd_ctl_hw_open() does it on
its own, whereas snd_card_load2 requires a char* argument. Translating
card number to device path can be done in three ways, essentially:

- building a proper path name the same way as snd_ctl_hw_open() does
it (that is, about the same way as snd_card_load1() does it - see
below) in each function calling snd_card_load2(); feasible, yet not
very good for maintenance;

- doing so inside snd_card_load2() if passed char pointer is null;
feasible, but it would still be duplicating (at least part of the)
code from snd_card_load1();

- calling snd_card_load2() via snd_card_load1() (as it's done right
now, but for one case - within snd_card_get_index - which would remain
about the same), by just furnishing snd_card_load1() with a
pointer-to-snd_ctl_card_info_t parameter to be passed straightforward
to snd_card_load2() (that could be null when specific card infos other
than card number aren't needed by the caller).

After a few considerations, the latter one seems feasible, and have
been my choice. Function snd_ctl_hw_open() builds the path name for a
control* device the same way it's done by snd_card_load1() (apparently
aload* devices aren't taken into account, but please, read further),
then snd_open_device is called, as it's done by snd_card_load2() BUT
with an important difference: the latter function opens devices
read-only, whereas snd_ctl_hw_open() can (and, as actually called by
snd_card_* stuff, it does) open devices read-write, which is both
needless and potentially dangerous. In case of failure opening chosen
device, snd_ctl_hw_open calls snd_card_load() and retries: this means
that snd_card_load1() and snd_card_load2() (via snd_card_load()) can
be called anyway, so that some operations are performed more than once
needlessly (and there's no need to call snd_card_load() from within
snd_card_get_index() anyway), but also that aload stuff, if compiled
as supported, is somehow taken into account silently even when it
would seem to be ignored (but perhaps shouldn't be?), as in
snd_card_get_name/longname. This should make snd_card_load1() suitable
to translate card number to device path and call snd_card_load2.

3) Printing out error messages. In some cases both snd_ctl_hw_open()
and snd_ctl_hw_card_info() can produce error messages, whereas
snd_card_load#() functions are mute in same scenarios. Whenever
they're called directly (in current implementation), such a behaviour
should remain as is, but when replacing snd_ctl_hw_* stuff same
errors/messages should be printed out. This is possible by duplicating
those (small) pieces of code from snd_ctl_hw_* functions and executing
them conditionally, availing of a further integer argument (used as a
boolean flag). To avoid complicating calls to snd_card_load1/2(), a
few macros can be defined to both mask out the "printing flag" and
give them an alias name appropriate for the context they're being
called from (which could add expressiveness to the implementation).

Anyway, take this as a simple suggestion, just give it a look.

===================================================
src/control/cards.c - attached
===================================================

[-- Attachment #2: cards.c --]
[-- Type: text/x-csrc, Size: 7267 bytes --]

/**
 * \file control/cards.c
 * \brief Basic Soundcard Operations
 * \author Jaroslav Kysela <perex@perex.cz>
 * \date 1998-2001
 */
/*
 *  Soundcard Operations - main file
 *  Copyright (c) 1998 by Jaroslav Kysela <perex@perex.cz>
 *
 *
 *   This library is free software; you can redistribute it and/or modify
 *   it under the terms of the GNU Lesser General Public License as
 *   published by the Free Software Foundation; either version 2.1 of
 *   the License, or (at your option) any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY; without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *   GNU Lesser General Public License for more details.
 *
 *   You should have received a copy of the GNU Lesser General Public
 *   License along with this library; if not, write to the Free Software
 *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
 *
 */

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <ctype.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include "control_local.h"

#ifndef DOC_HIDDEN

#define SND_FILE_CONTROL_FMT		ALSA_DEVICE_DIRECTORY "controlC%i"
#define SIZE_OF_SND_FILE_CONTROL_STR	( sizeof( SND_FILE_CONTROL_FMT ) + 10 )

#define SND_FILE_ALOAD_FMT		ALOAD_DEVICE_DIRECTORY "aloadC%i"
#define SIZE_OF_SND_FILE_ALOAD_STR	( sizeof( SND_FILE_ALOAD_FMT ) + 10 )

#endif /* DOC_HIDDEN */


static inline char * snd_card_print_path( char *buffer, const char *control_path_fmt, int card ){
	sprintf( buffer, control_path_fmt, card );
	return buffer;
}

#ifndef DOC_HIDDEN

/* defining context-dependent calls for snd_card_print_path */
#define _snd_card_build_control_path(buff, card)		snd_card_print_path( (buff), SND_FILE_CONTROL_FMT, (card) )
#define _snd_card_build_aload_path(buff, card)		snd_card_print_path( (buff), SND_FILE_ALOAD_FMT, (card) )

#define DONT_PRINT_ERRORS	0
#define PRINT_ERRORS	1

#endif /* DOC_HIDDEN */

static int snd_card_load2( const char *control, snd_ctl_card_info_t * out_info, int prn_err )
{
	int open_dev;
	snd_ctl_card_info_t *info_ref;

	open_dev = snd_open_device(control, O_RDONLY);

	if (open_dev >= 0) {
		if( out_info ) /* a reference to the object returned by next ioctl has been asked by the caller */
			info_ref = out_info;
		else /* next ioctl is only relevant for internal implementation */
			info_ref = (snd_ctl_card_info_t *) alloca( sizeof( snd_ctl_card_info_t ) );
		if (ioctl(open_dev, SNDRV_CTL_IOCTL_CARD_INFO, info_ref) < 0) {
			int err = -errno;
			close(open_dev);
			if( out_info && prn_err ) /* if the caller required info, it might want info on errors too - behaving same as snd_ctl_hw_card_info */
				SYSERR("SNDRV_CTL_IOCTL_CARD_INFO failed");
			return err;
		}
		close(open_dev);
		return info_ref->card;
	} else {
		return -errno;
	}
}


static int snd_card_load1( int card, snd_ctl_card_info_t *info_ref, int prn_err )
{
	int res;
	char control_path[ SIZE_OF_SND_FILE_CONTROL_STR ];

	if( prn_err ){ /* behaving same as snd_ctl_hw_open on request */
		if (CHECK_SANITY(card < 0 || card >= 32)) {
			SNDMSG("Invalid card index %d", card);
			return -EINVAL;
		}
	}

	res = snd_card_load2( _snd_card_build_control_path( control_path, card ), info_ref, prn_err );

#ifdef SUPPORT_ALOAD
	if (res < 0) {
		char aload_path[ SIZE_OF_SND_FILE_ALOAD_STR ];
		res = snd_card_load2( _snd_card_build_aload_path( aload_path, card ), info_ref, prn_err );
	}
#endif /* SUPPORT_ALOAD */

	return res;
}


#ifndef	DOC_HIDDEN

/* main goal is validating card number -> call snd_card_load1 with appropriate name */
#define snd_card_load_value( card, info )	snd_card_load1( (card), (info), DONT_PRINT_ERRORS )

/* main goal is getting card info -> call snd_card_load1 with appropriate name */
#define snd_card_load_info( card, info )	snd_card_load1( (card), (info), PRINT_ERRORS )

/* a string value must be passed instead of a card number -> call snd_card_load2 appropriately */
#define snd_card_load_info_from_string( str, info )	snd_card_load2( (str), (info), PRINT_ERRORS )

#endif /* DOC_HIDDEN */


/**
 * \brief Try to load the driver for a card.
 * \param card Card number.
 * \return 1 if driver is present, zero if driver is not present
 */
int snd_card_load(int card)
{
	return !!( snd_card_load_value( card, NULL ) >= 0 );
}


/**
 * \brief Try to determine the next card.
 * \param rcard pointer to card number
 * \result zero if success, otherwise a negative error code
 *
 * Tries to determine the next card from given card number.
 * If card number is -1, then the first available card is
 * returned. If the result card number is -1, no more cards
 * are available.
 */
int snd_card_next(int *rcard)
{
	int card;
	
	if (rcard == NULL)
		return -EINVAL;
	card = *rcard;
	card = card < 0 ? 0 : card + 1;
	for (; card < 32; card++) {
		if (snd_card_load(card)) {
			*rcard = card;
			return 0;
		}
	}
	*rcard = -1;
	return 0;
}

/**
 * \brief Convert card string to an integer value.
 * \param string String containing card identifier
 * \return an integer value greater than or equal to zero, corresponding to the string identifier, if success, otherwise a negative error code.
 *
 * The accepted format is an integer value in ASCII representation
 * or the card identifier (the id parameter for sound-card drivers).
 * The control device name like /dev/snd/controlC0 is accepted, too.
 */
int snd_card_get_index(const char *string)
{
	int card, err;
	snd_ctl_card_info_t info;

	if (!string || *string == '\0')
		return -EINVAL;

	/* integer value */
	if ((isdigit(*string) && *(string + 1) == 0) ||
	    (isdigit(*string) && isdigit(*(string + 1)) && *(string + 2) == 0)) {
		if (sscanf(string, "%i", &card) != 1)
			return -EINVAL;
		if (card < 0 || card > 31)
			return -EINVAL;
		err = snd_card_load_value( card, NULL );
		if (err >= 0) /* can this be different from card? */
			return card;
		return err;
	}

	/* device name */
	if (string[0] == '/')	
		return snd_card_load_info_from_string( string, &info );

	/* card identifier */
	for (card = 0; card < 32; card++) {
		if( snd_card_load_info( card, &info ) < 0 )
			continue;
		if (!strcmp((const char *)info.id, string))
			return card;
	}
	return -ENODEV;
}


/**
 * \brief Obtain the card name.
 * \param card Card number
 * \param name Result - card name corresponding to card number
 * \result zero if success, otherwise a negative error code
 */
int snd_card_get_name(int card, char **name)
{
	snd_ctl_card_info_t info;
	int err;
	
	if (name == NULL)
		return -EINVAL;

	if( ( err = snd_card_load_info( card, &info ) ) < 0 )
		return err;

	*name = strdup((const char *)info.name);
	if (*name == NULL)
		return -ENOMEM;

	return 0;
}


/**
 * \brief Obtain the card long name.
 * \param card Card number
 * \param name Result - card long name corresponding to card number
 * \result zero if success, otherwise a negative error code
 */
int snd_card_get_longname(int card, char **name)
{
	snd_ctl_card_info_t info;
	int err;
	
	if (name == NULL)
		return -EINVAL;

	if( ( err = snd_card_load_info( card, &info ) ) < 0 )
		return err;

	*name = strdup((const char *)info.longname);
	if (*name == NULL)
		return -ENOMEM;

	return 0;
}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]