Re: [PATCH] Provide card number / PID via sequencer client info

[Martin Koegler <martin.koegler@chello.at>]

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. 
 
> > @@ -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.

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

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.

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

> >  
> > 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.

Are I'm missing a code path?

Regards,
Martin