(eg. virBufferEscapeString [...]

On Thu, Dec 13, 2007 at 06:27:18PM +0000, Richard W.M. Jones wrote: > Richard W.M. Jones wrote: > >(eg. virBufferEscapeString [...] > > Erm, that should be 'cf' not 'eg'. The remote driver in CVS doesn't work with this patch applied - it just refuses the connection everytime. It looks like a conditional got reversed - see attached patch

+ * 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'))

On Thu, Dec 13, 2007 at 11:21:31PM +0100, Jim Meyering wrote: > "Richard W.M. Jones" <rjones(a)redhat.com&gt; 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.

Daniel Veillard <veillard(a)redhat.com&gt; wrote: > On Thu, Dec 13, 2007 at 11:21:31PM +0100, Jim Meyering wrote: >> "Richard W.M. Jones" <rjones(a)redhat.com&gt; 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:

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > On Fri, Dec 14, 2007 at 08:34:13AM +0100, Jim Meyering wrote: >> Daniel Veillard <veillard(a)redhat.com&gt; wrote: >> > On Thu, Dec 13, 2007 at 11:21:31PM +0100, Jim Meyering wrote: >> >> "Richard W.M. Jones" <rjones(a)redhat.com&gt; 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: > > Seriously who gives a damn about EBCDIC anymore. Daniel and Daniel, Personally, I don't care at all, but I suspect that people using z/OS and OS/390 care, among others. Some might even be Red Hat customers. What's wrong with making the code portable, just in case?

> This just makes it totally unreadable. This just makes *what* totally unreadable?

I proposed to do this: for (p = str; *p; ++p) { is_alphanum (*p) ... for (p = str; *p; ++p) { is_alphanum (*p)

What do you think of using this? isascii (*p) && isalnum (*p)

"Richard W.M. Jones" <rjones(a)redhat.com&gt; wrote: > Jim Meyering wrote: >> What do you think of using this? >> >> isascii (*p) && isalnum (*p) > I'm not sure I'm qualified to say what this does on EBCDIC, but quite > likely lots of other code breaks there too anyway. This is nicely > self-documenting anyway. As Daniel suggested, isalnum is locale-sensitive. If there's a locale with an alphabetic byte that is outside the logical a-zA-Z range, yet still within 0..127, then the above expression will give a false-positive for that byte. I've been inclined to stop worrying about EBCDIC for years, but a quick search on the web finds that people are still stuck using it, and do report bugs in ASCII-assuming code. This is why autoconf goes to the trouble of doing this: tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ not this: tr a-z A-Z to convert to upper case.

"Richard W.M. Jones" <rjones(a)redhat.com&gt; wrote: > Jim Meyering wrote: >> "Richard W.M. Jones" <rjones(a)redhat.com&gt; wrote: >>> Jim Meyering wrote: >>>> What do you think of using this? >>>> >>>> isascii (*p) && isalnum (*p) >>> I'm not sure I'm qualified to say what this does on EBCDIC, but quite >>> likely lots of other code breaks there too anyway. This is nicely >>> self-documenting anyway. >> As Daniel suggested, isalnum is locale-sensitive. >> If there's a locale with an alphabetic byte that is outside >> the logical a-zA-Z range, yet still within 0..127, then the above >> expression will give a false-positive for that byte. >> >> I've been inclined to stop worrying about EBCDIC for years, but a quick >> search on the web finds that people are still stuck using it, and do >> report bugs in ASCII-assuming code. >> >> This is why autoconf goes to the trouble of doing this: >> tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ >> not this: >> tr a-z A-Z >> to convert to upper case. > Another factor to consider here is that it doesn't matter if this > function over-escapes, but it does matter if the function > under-escapes. That is to say, it could escape every character as a > %xx hex code, which would be ugly and slightly inefficient but not > wrong. IMHO, if you don't use the all-enumerating switch-based code that Daniel objects to, it'd be good to document (in both loops) that the test is inaccurate with EBCDIC, and explain why it's ok to get false positives. Without comments, people might be tempted to use a similar test in a context where the differences matter.

I stand by my suggestion to use the switch statement.

OK, how about this? Rich. + for (p = str; *p; ++p) { + /* Want to escape only A-Z and 0-9. This may not work on EBCDIC. */ + if (isascii (*p) && isalnum (*p)) + grow_size++; + else + grow_size += 3; /* %ab */ + }