[PATCH 03/16] Add pa_channels_valid()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> I think this makes the code a bit nicer to read and write. This also
> reduces the chances of off-by-one errors when checking the bounds of
> channel count values.

I reviewed, rebased and pushed the first three patches of this series as
they seem independent of the rest (working on those now)

the new _valid() functions actually fix inconsistencies w.r.t _MAX (which 
is a valid channel count, sample rate but not a valid format)

thanks, p.

> ---
>  src/daemon/daemon-conf.c                     | 2 +-
>  src/map-file                                 | 1 +
>  src/modules/alsa/alsa-ucm.c                  | 6 +++---
>  src/modules/bluetooth/module-bluez4-device.c | 2 +-
>  src/modules/jack/module-jack-sink.c          | 3 +--
>  src/modules/jack/module-jack-source.c        | 3 +--
>  src/modules/jack/module-jackdbus-detect.c    | 2 +-
>  src/pulse/channelmap.c                       | 8 +++-----
>  src/pulse/sample.c                           | 7 +++++--
>  src/pulse/sample.h                           | 4 ++++
>  src/pulse/volume.c                           | 5 ++---
>  src/pulsecore/modargs.c                      | 3 +--
>  12 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/src/daemon/daemon-conf.c b/src/daemon/daemon-conf.c
> index de3bdc4..ce777a6 100644
> --- a/src/daemon/daemon-conf.c
> +++ b/src/daemon/daemon-conf.c
> @@ -387,7 +387,7 @@ static int parse_sample_channels(pa_config_parser_state *state) {
>  
>      i = state->data;
>  
> -    if (pa_atoi(state->rvalue, &n) < 0 || n > (int32_t) PA_CHANNELS_MAX || n <= 0) {
> +    if (pa_atoi(state->rvalue, &n) < 0 || !pa_channels_valid(n)) {
>          pa_log(_("[%s:%u] Invalid sample channels '%s'."), state->filename, state->lineno, state->rvalue);
>          return -1;
>      }
> diff --git a/src/map-file b/src/map-file
> index 071c141..fbad1a4 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -25,6 +25,7 @@ pa_channel_map_valid;
>  pa_channel_position_from_string;
>  pa_channel_position_to_pretty_string;
>  pa_channel_position_to_string;
> +pa_channels_valid;
>  pa_context_add_autoload;
>  pa_context_connect;
>  pa_context_disconnect;
> diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
> index 0ed2470..c88fc77 100644
> --- a/src/modules/alsa/alsa-ucm.c
> +++ b/src/modules/alsa/alsa-ucm.c
> @@ -214,7 +214,7 @@ static int ucm_get_device_property(
>      value = pa_proplist_gets(device->proplist, PA_ALSA_PROP_UCM_PLAYBACK_CHANNELS);
>      if (value) { /* output */
>          /* get channels */
> -        if (pa_atou(value, &ui) == 0 && ui < PA_CHANNELS_MAX)
> +        if (pa_atou(value, &ui) == 0 && pa_channels_valid(ui))
>              device->playback_channels = ui;
>          else
>              pa_log("UCM playback channels %s for device %s out of range", value, device_name);
> @@ -234,7 +234,7 @@ static int ucm_get_device_property(
>      value = pa_proplist_gets(device->proplist, PA_ALSA_PROP_UCM_CAPTURE_CHANNELS);
>      if (value) { /* input */
>          /* get channels */
> -        if (pa_atou(value, &ui) == 0 && ui < PA_CHANNELS_MAX)
> +        if (pa_atou(value, &ui) == 0 && pa_channels_valid(ui))
>              device->capture_channels = ui;
>          else
>              pa_log("UCM capture channels %s for device %s out of range", value, device_name);
> @@ -1128,7 +1128,7 @@ static void alsa_mapping_add_ucm_modifier(pa_alsa_mapping *m, pa_alsa_ucm_modifi
>          /* FIXME: channel_str is unsanitized input from the UCM configuration,
>           * we should do proper error handling instead of asserting.
>           * https://bugs.freedesktop.org/show_bug.cgi?id=71823 */
> -        pa_assert_se(pa_atou(channel_str, &channels) == 0 && channels < PA_CHANNELS_MAX);
> +        pa_assert_se(pa_atou(channel_str, &channels) == 0 && pa_channels_valid(channels));
>          pa_log_debug("Got channel count %" PRIu32 " for modifier", channels);
>      }
>  
> diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
> index f419cb9..83e603f 100644
> --- a/src/modules/bluetooth/module-bluez4-device.c
> +++ b/src/modules/bluetooth/module-bluez4-device.c
> @@ -2463,7 +2463,7 @@ int pa__init(pa_module *m) {
>  
>      channels = u->sample_spec.channels;
>      if (pa_modargs_get_value_u32(ma, "channels", &channels) < 0 ||
> -        channels <= 0 || channels > PA_CHANNELS_MAX) {
> +        !pa_channels_valid(channels)) {
>          pa_log_error("Failed to get channels from module arguments");
>          goto fail;
>      }
> diff --git a/src/modules/jack/module-jack-sink.c b/src/modules/jack/module-jack-sink.c
> index dccf032..ab97758 100644
> --- a/src/modules/jack/module-jack-sink.c
> +++ b/src/modules/jack/module-jack-sink.c
> @@ -350,8 +350,7 @@ int pa__init(pa_module*m) {
>          channels = m->core->default_sample_spec.channels;
>  
>      if (pa_modargs_get_value_u32(ma, "channels", &channels) < 0 ||
> -        channels <= 0 ||
> -        channels > PA_CHANNELS_MAX) {
> +        !pa_channels_valid(channels)) {
>          pa_log("Failed to parse channels= argument.");
>          goto fail;
>      }
> diff --git a/src/modules/jack/module-jack-source.c b/src/modules/jack/module-jack-source.c
> index 8f550e1..5d9668b 100644
> --- a/src/modules/jack/module-jack-source.c
> +++ b/src/modules/jack/module-jack-source.c
> @@ -297,8 +297,7 @@ int pa__init(pa_module*m) {
>          channels = m->core->default_sample_spec.channels;
>  
>      if (pa_modargs_get_value_u32(ma, "channels", &channels) < 0 ||
> -        channels <= 0 ||
> -        channels >= PA_CHANNELS_MAX) {
> +        !pa_channels_valid(channels)) {
>          pa_log("failed to parse channels= argument.");
>          goto fail;
>      }
> diff --git a/src/modules/jack/module-jackdbus-detect.c b/src/modules/jack/module-jackdbus-detect.c
> index 5c29772..160794b 100644
> --- a/src/modules/jack/module-jackdbus-detect.c
> +++ b/src/modules/jack/module-jackdbus-detect.c
> @@ -238,7 +238,7 @@ int pa__init(pa_module *m) {
>          goto fail;
>      }
>  
> -    if (pa_modargs_get_value_u32(ma, "channels", &u->channels) < 0 || u->channels > PA_CHANNELS_MAX) {
> +    if (pa_modargs_get_value_u32(ma, "channels", &u->channels) < 0 || !pa_channels_valid(u->channels)) {
>          pa_log("Failed to parse channels= argument.");
>          goto fail;
>      }
> diff --git a/src/pulse/channelmap.c b/src/pulse/channelmap.c
> index fec0623..72e4130 100644
> --- a/src/pulse/channelmap.c
> +++ b/src/pulse/channelmap.c
> @@ -199,8 +199,7 @@ pa_channel_map* pa_channel_map_init_stereo(pa_channel_map *m) {
>  
>  pa_channel_map* pa_channel_map_init_auto(pa_channel_map *m, unsigned channels, pa_channel_map_def_t def) {
>      pa_assert(m);
> -    pa_assert(channels > 0);
> -    pa_assert(channels <= PA_CHANNELS_MAX);
> +    pa_assert(pa_channels_valid(channels));
>      pa_assert(def < PA_CHANNEL_MAP_DEF_MAX);
>  
>      pa_channel_map_init(m);
> @@ -401,8 +400,7 @@ pa_channel_map* pa_channel_map_init_extend(pa_channel_map *m, unsigned channels,
>      unsigned c;
>  
>      pa_assert(m);
> -    pa_assert(channels > 0);
> -    pa_assert(channels <= PA_CHANNELS_MAX);
> +    pa_assert(pa_channels_valid(channels));
>      pa_assert(def < PA_CHANNEL_MAP_DEF_MAX);
>  
>      pa_channel_map_init(m);
> @@ -617,7 +615,7 @@ int pa_channel_map_valid(const pa_channel_map *map) {
>  
>      pa_assert(map);
>  
> -    if (map->channels <= 0 || map->channels > PA_CHANNELS_MAX)
> +    if (!pa_channels_valid(map->channels))
>          return 0;
>  
>      for (c = 0; c < map->channels; c++)
> diff --git a/src/pulse/sample.c b/src/pulse/sample.c
> index 70f7f72..82c1b01 100644
> --- a/src/pulse/sample.c
> +++ b/src/pulse/sample.c
> @@ -111,12 +111,15 @@ int pa_sample_rate_valid(uint32_t rate) {
>      return rate > 0 && rate <= PA_RATE_MAX;
>  }
>  
> +int pa_channels_valid(uint8_t channels) {
> +    return channels > 0 && channels <= PA_CHANNELS_MAX;
> +}
> +
>  int pa_sample_spec_valid(const pa_sample_spec *spec) {
>      pa_assert(spec);
>  
>      if (PA_UNLIKELY(!pa_sample_rate_valid(spec->rate) ||
> -        spec->channels <= 0 ||
> -        spec->channels > PA_CHANNELS_MAX ||
> +        !pa_channels_valid(spec->channels) ||
>          !pa_sample_format_valid(spec->format)))
>          return 0;
>  
> diff --git a/src/pulse/sample.h b/src/pulse/sample.h
> index b8abc1c..86973d4 100644
> --- a/src/pulse/sample.h
> +++ b/src/pulse/sample.h
> @@ -295,6 +295,10 @@ int pa_sample_format_valid(unsigned format) PA_GCC_PURE;
>  /** Return non-zero if the rate is within the supported range. \since 5.0 */
>  int pa_sample_rate_valid(uint32_t rate) PA_GCC_PURE;
>  
> +/** Return non-zero if the channel count is within the supported range.
> + * \since 5.0 */
> +int pa_channels_valid(uint8_t channels) PA_GCC_PURE;
> +
>  /** Return non-zero when the sample type specification is valid */
>  int pa_sample_spec_valid(const pa_sample_spec *spec) PA_GCC_PURE;
>  
> diff --git a/src/pulse/volume.c b/src/pulse/volume.c
> index fd67254..6927392 100644
> --- a/src/pulse/volume.c
> +++ b/src/pulse/volume.c
> @@ -73,8 +73,7 @@ pa_cvolume* pa_cvolume_set(pa_cvolume *a, unsigned channels, pa_volume_t v) {
>      int i;
>  
>      pa_assert(a);
> -    pa_assert(channels > 0);
> -    pa_assert(channels <= PA_CHANNELS_MAX);
> +    pa_assert(pa_channels_valid(channels));
>  
>      a->channels = (uint8_t) channels;
>  
> @@ -533,7 +532,7 @@ int pa_cvolume_valid(const pa_cvolume *v) {
>  
>      pa_assert(v);
>  
> -    if (v->channels <= 0 || v->channels > PA_CHANNELS_MAX)
> +    if (!pa_channels_valid(v->channels))
>          return 0;
>  
>      for (c = 0; c < v->channels; c++)
> diff --git a/src/pulsecore/modargs.c b/src/pulsecore/modargs.c
> index 15776a0..06973cb 100644
> --- a/src/pulsecore/modargs.c
> +++ b/src/pulsecore/modargs.c
> @@ -391,8 +391,7 @@ int pa_modargs_get_sample_spec(pa_modargs *ma, pa_sample_spec *rss) {
>  
>      channels = ss.channels;
>      if ((pa_modargs_get_value_u32(ma, "channels", &channels)) < 0 ||
> -        channels <= 0 ||
> -        channels >= PA_CHANNELS_MAX)
> +        !pa_channels_valid(channels))
>          return -1;
>      ss.channels = (uint8_t) channels;
>  
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux