Re: [PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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

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