| Message ID | 1401738082-28488-1-git-send-email-agoode@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Mon, 2 Jun 2014 15:41:22 -0400, Adam Goode wrote: > > Sometimes PORT_EXIT messages are lost when a process is exiting. > This happens if you subscribe to the announce port with client A, > then subscribe to the announce port with client B, then kill client A. > Client B will not see the PORT_EXIT message because client A's port is > closing and is earlier in the announce port subscription list. The > for each loop will try to send the announcement to client A and fail, > then will stop trying to broadcast to other ports. Killing B works fine > since the announcement will already have gone to A. The CLIENT_EXIT > message does not get lost. > > How to reproduce problem: > > *** termA > $ aseqdump -p 0:1 > 0:1 Port subscribed 0:1 -> 128:0 > > *** termB > $ aseqdump -p 0:1 > > *** termA > 0:1 Client start client 129 > 0:1 Port start 129:0 > 0:1 Port subscribed 0:1 -> 129:0 > > *** termB > 0:1 Port subscribed 0:1 -> 129:0 > > *** termA > ^C > > *** termB > 0:1 Client exit client 128 > <--- expected Port exit as well (before client exit) > > Signed-off-by: Adam Goode <agoode@google.com> Thanks. The changes look OK to me, though, I'd prefer a variable name "result" instead of "sticky_err". And you don't have to initialize err any longer there. Meanwhile, it's another question what to be returned from the function. Such broadcasting case, would it be better to return always num_ev when it's positive, or always report any (non-serious) error? Takashi > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c > index 9ca5e64..f9b5315 100644 > --- a/sound/core/seq/seq_clientmgr.c > +++ b/sound/core/seq/seq_clientmgr.c > @@ -660,7 +660,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client, > int atomic, int hop) > { > struct snd_seq_subscribers *subs; > - int err = 0, num_ev = 0; > + int err = 0, sticky_err = 0, num_ev = 0; > struct snd_seq_event event_saved; > struct snd_seq_client_port *src_port; > struct snd_seq_port_subs_info *grp; > @@ -685,8 +685,12 @@ static int deliver_to_subscribers(struct snd_seq_client *client, > subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIME_REAL); > err = snd_seq_deliver_single_event(client, event, > 0, atomic, hop); > - if (err < 0) > - break; > + if (err < 0) { > + /* save first error that occurs and continue */ > + if (!sticky_err) > + sticky_err = err; > + continue; > + } > num_ev++; > /* restore original event record */ > *event = event_saved; > @@ -697,7 +701,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client, > up_read(&grp->list_mutex); > *event = event_saved; /* restore */ > snd_seq_port_unlock(src_port); > - return (err < 0) ? err : num_ev; > + return (sticky_err < 0) ? sticky_err : num_ev; > } > > > @@ -709,7 +713,7 @@ static int port_broadcast_event(struct snd_seq_client *client, > struct snd_seq_event *event, > int atomic, int hop) > { > - int num_ev = 0, err = 0; > + int num_ev = 0, err = 0, sticky_err = 0; > struct snd_seq_client *dest_client; > struct snd_seq_client_port *port; > > @@ -724,14 +728,18 @@ static int port_broadcast_event(struct snd_seq_client *client, > err = snd_seq_deliver_single_event(NULL, event, > SNDRV_SEQ_FILTER_BROADCAST, > atomic, hop); > - if (err < 0) > - break; > + if (err < 0) { > + /* save first error that occurs and continue */ > + if (!sticky_err) > + sticky_err = err; > + continue; > + } > num_ev++; > } > read_unlock(&dest_client->ports_lock); > snd_seq_client_unlock(dest_client); > event->dest.port = SNDRV_SEQ_ADDRESS_BROADCAST; /* restore */ > - return (err < 0) ? err : num_ev; > + return (sticky_err < 0) ? sticky_err : num_ev; > } > > /* > @@ -741,7 +749,7 @@ static int port_broadcast_event(struct snd_seq_client *client, > static int broadcast_event(struct snd_seq_client *client, > struct snd_seq_event *event, int atomic, int hop) > { > - int err = 0, num_ev = 0; > + int err = 0, sticky_err = 0, num_ev = 0; > int dest; > struct snd_seq_addr addr; > > @@ -760,12 +768,16 @@ static int broadcast_event(struct snd_seq_client *client, > err = snd_seq_deliver_single_event(NULL, event, > SNDRV_SEQ_FILTER_BROADCAST, > atomic, hop); > - if (err < 0) > - break; > + if (err < 0) { > + /* save first error that occurs and continue */ > + if (!sticky_err) > + sticky_err = err; > + continue; > + } > num_ev += err; > } > event->dest = addr; /* restore */ > - return (err < 0) ? err : num_ev; > + return (sticky_err < 0) ? sticky_err : num_ev; > } > > > -- > 1.9.1.423.g4596e3a >
On Tue, Jun 3, 2014 at 1:42 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Mon, 2 Jun 2014 15:41:22 -0400, > Adam Goode wrote: > > > > Sometimes PORT_EXIT messages are lost when a process is exiting. > > This happens if you subscribe to the announce port with client A, > > then subscribe to the announce port with client B, then kill client A. > > Client B will not see the PORT_EXIT message because client A's port is > > closing and is earlier in the announce port subscription list. The > > for each loop will try to send the announcement to client A and fail, > > then will stop trying to broadcast to other ports. Killing B works fine > > since the announcement will already have gone to A. The CLIENT_EXIT > > message does not get lost. > > > > How to reproduce problem: > > > > *** termA > > $ aseqdump -p 0:1 > > 0:1 Port subscribed 0:1 -> 128:0 > > > > *** termB > > $ aseqdump -p 0:1 > > > > *** termA > > 0:1 Client start client 129 > > 0:1 Port start 129:0 > > 0:1 Port subscribed 0:1 -> 129:0 > > > > *** termB > > 0:1 Port subscribed 0:1 -> 129:0 > > > > *** termA > > ^C > > > > *** termB > > 0:1 Client exit client 128 > > <--- expected Port exit as well (before client exit) > > > > Signed-off-by: Adam Goode <agoode@google.com> > > Thanks. The changes look OK to me, though, I'd prefer a variable name > "result" instead of "sticky_err". And you don't have to initialize > err any longer there. > > Meanwhile, it's another question what to be returned from the > function. Such broadcasting case, would it be better to return always > num_ev when it's positive, or always report any (non-serious) error? > > Thanks, I will send another patch with "result" instead. Right now, the function will always report an error if any error occurs. My patch should not change this behavior. I believe my patch has the effect equivalent to sorting the list such that the failed ports are all at the end. If you would like to change the behavior of the function, that is another issue. Thanks, Adam
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 9ca5e64..f9b5315 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -660,7 +660,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client, int atomic, int hop) { struct snd_seq_subscribers *subs; - int err = 0, num_ev = 0; + int err = 0, sticky_err = 0, num_ev = 0; struct snd_seq_event event_saved; struct snd_seq_client_port *src_port; struct snd_seq_port_subs_info *grp; @@ -685,8 +685,12 @@ static int deliver_to_subscribers(struct snd_seq_client *client, subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIME_REAL); err = snd_seq_deliver_single_event(client, event, 0, atomic, hop); - if (err < 0) - break; + if (err < 0) { + /* save first error that occurs and continue */ + if (!sticky_err) + sticky_err = err; + continue; + } num_ev++; /* restore original event record */ *event = event_saved; @@ -697,7 +701,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client, up_read(&grp->list_mutex); *event = event_saved; /* restore */ snd_seq_port_unlock(src_port); - return (err < 0) ? err : num_ev; + return (sticky_err < 0) ? sticky_err : num_ev; } @@ -709,7 +713,7 @@ static int port_broadcast_event(struct snd_seq_client *client, struct snd_seq_event *event, int atomic, int hop) { - int num_ev = 0, err = 0; + int num_ev = 0, err = 0, sticky_err = 0; struct snd_seq_client *dest_client; struct snd_seq_client_port *port; @@ -724,14 +728,18 @@ static int port_broadcast_event(struct snd_seq_client *client, err = snd_seq_deliver_single_event(NULL, event, SNDRV_SEQ_FILTER_BROADCAST, atomic, hop); - if (err < 0) - break; + if (err < 0) { + /* save first error that occurs and continue */ + if (!sticky_err) + sticky_err = err; + continue; + } num_ev++; } read_unlock(&dest_client->ports_lock); snd_seq_client_unlock(dest_client); event->dest.port = SNDRV_SEQ_ADDRESS_BROADCAST; /* restore */ - return (err < 0) ? err : num_ev; + return (sticky_err < 0) ? sticky_err : num_ev; } /* @@ -741,7 +749,7 @@ static int port_broadcast_event(struct snd_seq_client *client, static int broadcast_event(struct snd_seq_client *client, struct snd_seq_event *event, int atomic, int hop) { - int err = 0, num_ev = 0; + int err = 0, sticky_err = 0, num_ev = 0; int dest; struct snd_seq_addr addr; @@ -760,12 +768,16 @@ static int broadcast_event(struct snd_seq_client *client, err = snd_seq_deliver_single_event(NULL, event, SNDRV_SEQ_FILTER_BROADCAST, atomic, hop); - if (err < 0) - break; + if (err < 0) { + /* save first error that occurs and continue */ + if (!sticky_err) + sticky_err = err; + continue; + } num_ev += err; } event->dest = addr; /* restore */ - return (err < 0) ? err : num_ev; + return (sticky_err < 0) ? sticky_err : num_ev; }
Sometimes PORT_EXIT messages are lost when a process is exiting. This happens if you subscribe to the announce port with client A, then subscribe to the announce port with client B, then kill client A. Client B will not see the PORT_EXIT message because client A's port is closing and is earlier in the announce port subscription list. The for each loop will try to send the announcement to client A and fail, then will stop trying to broadcast to other ports. Killing B works fine since the announcement will already have gone to A. The CLIENT_EXIT message does not get lost. How to reproduce problem: *** termA $ aseqdump -p 0:1 0:1 Port subscribed 0:1 -> 128:0 *** termB $ aseqdump -p 0:1 *** termA 0:1 Client start client 129 0:1 Port start 129:0 0:1 Port subscribed 0:1 -> 129:0 *** termB 0:1 Port subscribed 0:1 -> 129:0 *** termA ^C *** termB 0:1 Client exit client 128 <--- expected Port exit as well (before client exit) Signed-off-by: Adam Goode <agoode@google.com>