At Wed, 31 May 2006 12:04:39 +0100,
James Courtier-Dutton wrote:
>
> Takashi Iwai wrote:
> > 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;
> > +}
> >
> >
> What do I use instead?
Use u32 array at the first place so that you need no cast.
If you do char -> u32 pointer conversion, you must guarantee the
alignment, at least. Otherwise the byte-wise conversion is needed
like you suggested.
> > +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?
> >
> No particular reason, just seemed like a ball park value that I should
> not have to change any time soon. I could probably replace it with some
> getuser32 calls to get the required length from the userspace app. Is
> getuser32() the correct call to use instead of copy_from_user(), if all
> one needs is a 4 byte integer from a particular memory location?
(You mean get_user() with an integer argument?)
Yes, we should retrieve the minimal header info (length and type), do
sanity checks, then allocate the value array according to the given
length.
> > Also, cases should be aligned to switch().
> >
> Ok.
> > But, above all, such a large switch() is ugly. Better to put the
> > container-type specific handling out of this function.
> >
> > 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.
> >
> That's true. I will look at putting the case action as a separate function.
>
> > +struct snd_ctl_misc {
> > + u32 version;
> > + u32 length;
> > + unsigned char message[];
> > +};
> >
> > The empty array isn't portable.
> >
> Ok, how do I do this then?
> The array is not actually empty, but it's size is dependent on the
> length field.
Yeah, I know. And that usage is non-portable.
The callback doesn't have to know about the version. So, simply
separate arguments like info2(ctl, type, length, value) should
suffice.
> > +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).
> >
> I don't understand. This is almost an exact copy of:
> typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol,
> struct snd_ctl_elem_info * uinfo);
> That is already in the code.
I meant you pass the value "1" to the function, and the callback again
checks "1". That's definitely to be avoided. Define a constant or
use enum instead of 1.
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
