On Tue, 27 Jun 2017 10:57:55 +0200,
Takashi Sakamoto wrote:
>
> Hi,
>
> On Jun 27 2017 17:15, Takashi Iwai wrote:
> > On Tue, 27 Jun 2017 10:09:37 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> Hi,
> >>
> >> On Jun 27 2017 00:09, Takashi Iwai wrote:
> >>> On Sun, 25 Jun 2017 06:41:19 +0200,
> >>> Takashi Sakamoto wrote:
> >>>>
> >>>> Handling mapping operation for status/control data includes some
> >>>> supplemental operations for fallback mode. It's better to have helper
> >>>> functions for this purpose.
> >>>>
> >>>> This commit adds the helper functions.
> >>>> ---
> >>>> src/pcm/pcm_hw.c | 48 ++++++++++++++++++++++++++++++++++--------------
> >>>> 1 file changed, 34 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
> >>>> index a648d12c..abf4afe0 100644
> >>>> --- a/src/pcm/pcm_hw.c
> >>>> +++ b/src/pcm/pcm_hw.c
> >>>> @@ -31,6 +31,7 @@
> >>>> #include <stdlib.h>
> >>>> #include <stddef.h>
> >>>> #include <unistd.h>
> >>>> +#include <stdbool.h>
> >>>> #include <signal.h>
> >>>> #include <string.h>
> >>>> #include <fcntl.h>
> >>>> @@ -866,7 +867,7 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
> >>>> return xfern.result;
> >>>> }
> >>>> -static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
> >>>> +static int map_status_data(snd_pcm_t *pcm)
> >>>> {
> >>>> snd_pcm_hw_t *hw = pcm->private_data;
> >>>> struct snd_pcm_sync_ptr sync_ptr;
> >>>> @@ -900,7 +901,7 @@ static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
> >>>> return 0;
> >>>> }
> >>>> -static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
> >>>> +static int map_control_data(snd_pcm_t *pcm)
> >>>> {
> >>>> snd_pcm_hw_t *hw = pcm->private_data;
> >>>> void *ptr;
> >>>> @@ -922,10 +923,28 @@ static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
> >>>> return 0;
> >>>> }
> >>>> -static int snd_pcm_hw_munmap_status(snd_pcm_t *pcm)
> >>>> +static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
> >>>> {
> >>>> snd_pcm_hw_t *hw = pcm->private_data;
> >>>> int err;
> >>>> +
> >>>> + hw->sync_ptr_ioctl = (int)force_fallback;
> >>>> +
> >>>> + err = map_status_data(pcm);
> >>>> + if (err < 0)
> >>>> + return err;
> >>>> +
> >>>> + err = map_control_data(pcm);
> >>>> + if (err < 0)
> >>>> + return err;
> >>>
> >>> This would leave the status mmap in the error path?
> >>
> >> In the error path, snd_pcm_hw_open_fd() calls snd_pcm_close(), then
> >> The status data is going to be unmapped.
> >>
> >> snd_pcm_hw_open_fd()
> >> ->snd_pcm_close()
> >> ->snd_pcm_hw_close()
> >> ->unmap_status_and_control_data()
> >> ->unmap_status_data()
> >> ->munmap(2) or free(3)
> >
> > Ah then there was already a bug there. It free/munmap unconditionally
> > for both status and control even for an error path of the mmap of
> > control (while status was mmapped).
>
> Yep. These codes were originally written with a loose manner. But it
> would be allowed because both 'mmap(NULL, non-zero)' and 'free(NULL)'
> generates no error and 'sync_ptr_ioctl' works fine, and 'sync_ptr' is
> assigned to NULL after deallocated.
Yes, that's a bit fragile way, but it works.
BTW, the whole patchset seems missing your sign-off.
I can add it manually, though.
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
