[alsa-devel] [PATCH] Provide card number / PID via sequencer client info

[Takashi Iwai]

On Mon, 15 Feb 2016 19:32:43 +0100,
Martin Koegler wrote:
> 
> On Mon, Feb 15, 2016 at 11:30:15AM +0100, Takashi Iwai wrote:
> > The idea looks good.  One remaining question is whether only providing
> > the card number or pid is sufficient for all cases.
> 
> I only care about the kernel client. 

Well, the question is whether only card number is enough.  What if
cards provide multiple rawmidi devices?

> > > @@ -357,7 +357,8 @@ struct snd_seq_client_info {
> > >  	unsigned char event_filter[32];	/* event filter bitmap */
> > >  	int num_ports;			/* RO: number of ports */
> > >  	int event_lost;			/* number of lost events */
> > > -	char reserved[64];		/* for future use */
> > > +	int owner;			/* RO: card number[kernel] / PID[user] */
> > > +	char reserved[64 - sizeof(int)]; /* for future use */
> > 
> > The sizeof(int) is always 4.  So let's make it explicit.
> > 
> > But, please make sure that this change won't lead to any
> > incompatibility.  Some architectures might align with 64bit long,
> > although the 32bit int and the rest char[] should be fine on all known
> > architectures, AFAIK.
> 
> Sorry, I don't have access to the various non x86-architectures, so I can't test.

Usually it's tested by cross-compiling, so everyone can do it.

> 
> I will change to 
> int card; 
> int pid; 
> char reserve[56];

Yes, this would be better.


> More safety would just provide a much more complex (ugly?) structure:
> 
> struct _int_snd_seq_client_info_old {
>         int client;                     /* client number to inquire */
>         snd_seq_client_type_t type;     /* client type */
>         char name[64];                  /* client name */
>         unsigned int filter;            /* filter flags */
>         unsigned char multicast_filter[8]; /* multicast filter bitmap */
>         unsigned char event_filter[32]; /* event filter bitmap */
>         int num_ports;                  /* RO: number of ports */
>         int event_lost;                 /* number of lost events */
>         char reserved[64]; /* for future use */
> };
> 
> struct _int_snd_seq_client_info_new {
>         int client;                     /* client number to inquire */
>         snd_seq_client_type_t type;     /* client type */
>         char name[64];                  /* client name */
>         unsigned int filter;            /* filter flags */
>         unsigned char multicast_filter[8]; /* multicast filter bitmap */
>         unsigned char event_filter[32]; /* event filter bitmap */
>         int num_ports;                  /* RO: number of ports */
>         int event_lost;                 /* number of lost events */
>         int card;
>         int pid;
>         char reserved[64]; /* for future use */
> };
> 
> struct snd_seq_client_info {
>         int client;                     /* client number to inquire */
>         snd_seq_client_type_t type;     /* client type */
>         char name[64];                  /* client name */
>         unsigned int filter;            /* filter flags */
>         unsigned char multicast_filter[8]; /* multicast filter bitmap */
>         unsigned char event_filter[32]; /* event filter bitmap */
>         int num_ports;                  /* RO: number of ports */
>         int event_lost;                 /* number of lost events */
>         int card;
>         int pid;
>         char reserved[64 - sizeof(struct _int_snd_seq_client_info_new) + sizeof(_int_snd_seq_client_info_old)]; /* for future use */
> };
> 
> This should handle all alignment and type size issues automatically.

You'd need to define old and new structs without reserved field.
The calculation still works but it's bogus.


> If you really want this, I can provide the apropriate patches.

No, I never want such a thing :)


> > >  
> > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
> > > index 58e79e0..ac07ab7 100644
> > > --- a/sound/core/seq/seq_clientmgr.c
> > > +++ b/sound/core/seq/seq_clientmgr.c
> > > @@ -364,6 +364,7 @@ static int snd_seq_open(struct inode *inode, struct file *file)
> > >  	/* fill client data */
> > >  	user->file = file;
> > >  	sprintf(client->name, "Client-%d", c);
> > > +	client->data.user.owner = get_pid(task_pid(current));
> > >  
> > >  	/* make others aware this new client */
> > >  	snd_seq_system_client_ev_client_start(c);
> > > @@ -380,6 +381,7 @@ static int snd_seq_release(struct inode *inode, struct file *file)
> > >  		seq_free_client(client);
> > >  		if (client->data.user.fifo)
> > >  			snd_seq_fifo_delete(&client->data.user.fifo);
> > > +		put_pid(client->data.user.owner);
> > >  		kfree(client);
> > >  	}
> > 
> > Shouldn't the counterpart (put_pid()) be called anywhere else?
> 
> The only way to free a client structure is seq_free_client1 (static).
> This is called by snd_seq_open before get_pid [=> irelevant] and 
> seq_free_client (static).
> seq_free_client is called by snd_seq_release, which calls put_pid and
> snd_seq_delete_kernel_client, which destroys a kernel client.

OK, I misread the patch where to place the code.  Then this should be
fine.


Takashi