[Libvir] [PATCH] Enhance virBuffer code

[Jim Meyering]

Daniel Veillard <veillard at redhat.com> wrote:

> On Thu, Dec 13, 2007 at 11:21:31PM +0100, Jim Meyering wrote:
>> "Richard W.M. Jones" <rjones at redhat.com> wrote:
>> ...
>> > + * virBufferURIEncodeString:
>> > + * @buf:  the buffer to append to
>> > + * @str:  the string argument which will be URI-encoded
>> > + *
>> > + * Append the string to the buffer.  The string will be URI-encoded
>> > + * during the append (ie any non alpha-numeric characters are replaced
>> > + * with '%xx' hex sequences).
>> > + *
>> > + * Returns 0 successful, -1 in case of internal or API error.
>> > + */
>> > +int
>> > +virBufferURIEncodeString (virBufferPtr buf, const char *str)
>> > +{
>> > +    int grow_size = 0;
>> > +    const char *p;
>> > +    unsigned char uc;
>> > +    const char *hex = "0123456789abcdef";
>> > +
>> > +    for (p = str; *p; ++p) {
>> > +        /* Want to leave only strict 7 bit ASCII alphanumerics ... */
>> > +        if ((*p >= '0' && *p <= '9') ||
>> > +            (*p >= 'a' && *p <= 'z') ||
>> > +            (*p >= 'A' && *p <= 'Z'))
>> ...
>> > +    for (p = str; *p; ++p) {
>> > +        if ((*p >= '0' && *p <= '9') ||
>> > +            (*p >= 'a' && *p <= 'z') ||
>> > +            (*p >= 'A' && *p <= 'Z'))
>>
>> Hi Rich,
>>
>> What do you think of using this?
>>
>>   isascii (*p) && isalnum (*p)
>
>   I have learned to be very cautious of the is* macros because they
> tend to be local dependant whichis usually really not what you would
> like or expect. In that case this may work, but explicit ranges
> are 100% clear about what you intend to accept or not, that's why I
> usually prefer them.

You're right that we shouldn't use isalnum here.
However, we shouldn't use inequality comparisons, either.
While 0 <= c <= 9 is guaranteed to be ok for the digits, the
a..z and A..Z ranges need not be contiguous, i.e., with EBCDIC:

  http://www.natural-innovations.com/computing/asciiebcdic.html

so how about this instead?

int
is_alphanum (char c)
{
  switch (c)
  {
    /* generated by LC_ALL=C perl -e \
       "print map {qq(case '\$_': )}('a'..'z', 'A'..'Z', '0'..'9')"|fmt
    case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': case 'g':
    case 'h': case 'i': case 'j': case 'k': case 'l': case 'm': case 'n':
    case 'o': case 'p': case 'q': case 'r': case 's': case 't': case 'u':
    case 'v': case 'w': case 'x': case 'y': case 'z': case 'A': case 'B':
    case 'C': case 'D': case 'E': case 'F': case 'G': case 'H': case 'I':
    case 'J': case 'K': case 'L': case 'M': case 'N': case 'O': case 'P':
    case 'Q': case 'R': case 'S': case 'T': case 'U': case 'V': case 'W':
    case 'X': case 'Y': case 'Z': case '0': case '1': case '2': case '3':
    case '4': case '5': case '6': case '7': case '8': case '9':
      return 1;
    default:
      return 0;
  }
}

Of course, systems for which this can make a difference (z/OS, S390)
may not be libvirt portability targets, but better safe than sorry.