At Tue, 30 May 2006 19:59:58 +0100,
James Courtier-Dutton wrote:
>
> So, from what I can see, there have been no real reasons why I should
> not commit my db gain code now.
No, please not yet. The code is still not good enough for commit.
For example, some things to be fixed regarding the code (and coding
style) are below:
+static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
+{
+ unsigned int pointer = misc->length;
+ u32 *store = (u32*) &misc->message[pointer];
This cast isn't always allowed.
+ *store = value;
+ misc->length += sizeof(u32);
+ return 0;
+}
+static int add_message_tlv_to_container(
+ struct snd_ctl_misc *misc, unsigned int length, unsigned char *message)
Unconventional indentation.
+ unsigned int pointer = misc->length;
+ unsigned int n;
+ for(n=0; n<length; n++) {
+ misc->message[pointer++] = message[n];
+ }
What's wrong with memcpy()?
Anyway, please fix spaces and remove braces.
+static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
+ struct snd_ctl_misc __user *_misc)
+{
+ struct snd_ctl_misc *misc;
+ struct snd_card *card = ctl->card;
+ struct snd_kcontrol *kctl;
+ struct snd_ctl_elem_info2 info2;
+ int result=0;
+ int numid;
+ unsigned int received_length;
+ unsigned int tlv_type;
+ unsigned int tlv_length;
+ unsigned int tlv_value;
+ unsigned int container_type;
+ unsigned int container_length;
+
+ misc = kmalloc(2048, GFP_KERNEL);
+ if (misc == NULL)
+ return -ENOMEM;
Why always 2048?
+ /* Only copy _misc: version and length from user-space */
+ if (copy_from_user(misc, _misc, 8)) {
+ result = -EFAULT;
+ goto exit_free;
+ }
+ /* Sanity check */
+ snd_printk("misc->version=%d\n",misc->version);
+ if (misc->version!=1) {
+ result = -EINVAL;
+ goto exit_free;
+ }
+ received_length = misc->length;
+ snd_printk("misc->length=%d\n",misc->length);
+ if (received_length>2040 || received_length<8) {
+ result = -EINVAL;
+ goto exit_free;
+ }
Fix spaces, remove debug prints.
+ misc->length=0;
+ message_to_uint32(misc, &container_type);
+ snd_printk("container_type=%d\n",container_type);
+ switch (container_type) {
+ case SND_MISC_DB_SCALE:
+ message_to_uint32(misc, &container_length);
+ /* FIXME: Do more sanity checks here */
+ message_to_uint32(misc, &tlv_type);
+ snd_printk("tlv_type=%d\n",tlv_type);
+ switch (tlv_type) {
+ case SND_MISC_ELEM_NUMID:
+ message_to_uint32(misc, &tlv_length);
+ snd_printk("tlv_length=%d\n",tlv_length);
+ if (tlv_length != 4) {
+ result = -EINVAL;
+ goto exit_free;
+ }
+ message_to_uint32(misc, &tlv_value);
+ snd_printk("tlv_value=%d\n",tlv_value);
+ break;
+ default:
+ result = -EINVAL;
+ goto exit_free;
+ break;
+ }
+ break;
+ default:
+ result = -EINVAL;
+ goto exit_free;
+ }
Fix spaces, remove debug prints.
Also, cases should be aligned to switch().
But, above all, such a large switch() is ugly. Better to put the
container-type specific handling out of this function.
+ numid=tlv_value;
+ //snd_printk("numid=%d\n", tlv_value);
+ down_read(&card->controls_rwsem);
+ kctl = snd_ctl_find_numid(card, numid);
+ if (kctl == NULL) {
+ snd_printk("stop 1\n");
+ result = -ENOENT;
+ goto exit_up;
+ }
+ if ( !(kctl->info2) ) {
+ snd_printk("stop 2\n");
+ result = -EINVAL;
+ goto exit_up;
+ }
+ if (kctl->info2(kctl, 1, &info2)) {
+ snd_printk("stop 3\n");
+ result = -EINVAL;
+ goto exit_up;
+ }
+ ;
+ snd_printk("ok 1\n");
+ misc->version=1;
+ misc->length=0;
+ container_open(misc, SND_MISC_DB_SCALE);
+ add_message_tlv_to_container(misc, info2.length, info2.message);
+ container_close(misc);
+ if (copy_to_user(_misc, misc, received_length))
+ result = -EFAULT;
+ snd_printk("ok 2\n");
Although this whole block basically belongs only to DB_SCALE case, the
current code looks as if this block is commonly handled regardless of
the container type.
+struct snd_ctl_misc {
+ u32 version;
+ u32 length;
+ unsigned char message[];
+};
The empty array isn't portable.
+typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);
The type should be non-numeric but constants (bitwise define or
enum).
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
