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);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