Re: [alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges

At Mon, 21 May 2012 21:38:22 +0200 (CEST), Benoît Thébaudeau wrote:

Hi Takashi, Clemens, all,

Takashi Iwai wrote:

...

Benoît Thébaudeau wrote:

...

Clemens Ladisch wrote:

...

I'll see if I can cobble together some TLV checking code ...

Great.

...

...

Because it's useless to wait until the end of the loop to return -EINVAL,

It is not necessary to optimize for error cases.

It's not an optimization, but a simplification. Otherwise, pos would have to be tested again after the loop to differentiate the loop-skipped case from the at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.

Unless it's really a hot path, such a micro-optimization doesn't give much value.

Rather better to keep the standard style that makes people easier to read the code. A do-while loop with a large block is usually less readable than a normal while loop.

Would you agree with the following revision?

Yes, the patch looks good to me. But I couldn't apply it because the mail is encoded in quoted-printable. Could you fix it, or if it's difficult, give an attachment?

thanks,

Takashi

[PATCH] alsa-lib/tlv: improve robustness of raw value ranges

snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is exported, it is better for robustness and consistency to parse the range properly, which this patch does.

Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com

--- alsa-lib/src/control/tlv.c +++ alsa-lib/src/control/tlv.c @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int { switch (tlv[0]) { case SND_CTL_TLVT_DB_RANGE: {

•            long dbmin, dbmax, prev_rangemax;
  

•            long dbmin, dbmax, prev_submax;
             unsigned int pos, len;
             len = int_index(tlv[1]);
  

•            if (len > MAX_TLV_RANGE_SIZE)
  

•                    return -EINVAL;
  

•            if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
  

•                                     &dbmin, &dbmax))
  

•            if (len < 6 || len > MAX_TLV_RANGE_SIZE)
                     return -EINVAL;
  

•            if (db_gain <= dbmin) {
  

•                    *value = rangemin;
  

•                    return 0;
  

•            } else if (db_gain >= dbmax) {
  

•                    *value = rangemax;
  

•                    return 0;
  

•            }
             pos = 2;
  

•            prev_rangemax = 0;
  

•            prev_submax = 0;
             while (pos + 4 <= len) {
  

•                    rangemin = (int)tlv[pos];
  

•                    rangemax = (int)tlv[pos + 1];
  

•                    long submin, submax;
  

•                    submin = (int)tlv[pos];
  

•                    submax = (int)tlv[pos + 1];
  

•                    if (rangemax < submax)
  

•                            submax = rangemax;
                     if (!snd_tlv_get_dB_range(tlv + pos + 2,
  

•                                              rangemin, rangemax,
  

•                                              submin, submax,
                                               &dbmin, &dbmax) &&
                         db_gain >= dbmin && db_gain <= dbmax)
                             return snd_tlv_convert_from_dB(tlv + pos + 2,
  

•                                                           rangemin, rangemax,
  

•                                                           submin, submax,
                                                            db_gain, value, xdir);
                     else if (db_gain < dbmin) {
  

•                            *value = xdir ? rangemin : prev_rangemax;
  

•                            *value = xdir || pos == 2 ? submin : prev_submax;
                             return 0;
                     }
  

•                    prev_rangemax = rangemax;
  

•                    prev_submax = submax;
  

•                    if (rangemax == submax)
  

•                            break;
                     pos += int_index(tlv[pos + 3]) + 4;
             }
  

•            return -EINVAL;
  

•            *value = prev_submax;
  

•            return 0;
     }
     case SND_CTL_TLVT_DB_SCALE: {
             int min, step, max;