Hi Charles and Clemens, Sorry to be late. On May 5 2016 20:46, Charles Keepax wrote: > On Thu, May 05, 2016 at 11:39:34AM +0200, Clemens Ladisch wrote: >> Takashi Sakamoto wrote: >>> On May 4 2016 22:59, Charles Keepax wrote: >>>> if (PCM_RUNTIME_CHECK(substream)) >>>> - return -ENXIO; >>>> + return POLLIN | POLLRDNORM | POLLERR; >>> >>> [...] >>> On the other hand, I think POLLOUT, POLLIN, POLLWRNORM and POLLRDNORM >>> should not be included in the value. PCM_RUNTIME_CHECK() ensures PCM >>> substream or PCM runtime is NULL. This means that subsequent I/O >>> operations are failed, at least for handling PCM frames. >>> >>> I think it better to return 'POLLERR | POLLHUP'. >> >> To expand on this: POLLIN/POLLOUT imply that it is possible to read/ >> write data without blocking. Sockets and pipes combine POLLHUP with >> POLLIN because the read() (or recv()) returns 0 bytes without blocking >> to indicate the end of the stream. >> >> But in this situation, snd_pcm_read*/write* will always fail, so it is, >> strictly speaking, indeed not appropriate to set POLLIN/OUT. >> >> On the other hand, PCM devices do include the POLLIN/OUT bits when they >> are in a wrong state. (This is probably to catch programs that do not >> check the error bits; with POLLIN/OUT set, these programs will try to >> read/write, and will then get the error code.) >> >> So for consistency, the bits should be included. (Or the other error >> case fixed to remove these bits.) I agree with the Clemens's view. So this patch is OK to me. Reviewd-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx> But if we have the intention for including POLLIN/OUT, it's better to add some comments about it. > Thanks for the explaination guys. I definitely agree that all > the return values should be consistent. I am happy to change the > values if people prefer but I guess the decision really rests > with Takashi and if he is happy to change the returns to > POLLERR | POLLHUP, as I guess there is the potential for some > user-space fall out. Perhaps I should do this as a seperate patch > on top of this chain, so we can review explicitly. I guess that this patch can be applied to ALSA PCM core separately from the others for ALSA Compress core. So it's better to post two series; one includes this patch, another includes the rest. > I have had a look and both tinyalsa and alsalib look like they > would handle the change correctly. In the same time, it's better for us to consider that userspace applications can be programmed directly to use kernel/userspace interface without these I/O libraries. Regards Takashi (not-maintainer) Sakamoto _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
