Re: [evolution-patches] patch for fix fix non rfc2047 compliant i18n mailer
- From: Not Zed <notzed ximian com>
- To: cantona <cantona softhome net>
- Cc: Jeffrey Stedfast <fejj ximian com>, evolution-patches lists ximian com
- Subject: Re: [evolution-patches] patch for fix fix non rfc2047 compliant i18n mailer
- Date: Mon, 31 May 2004 10:24:26 +0800
Hi,
I'd really rather you started with my patch. This one is no good for a number of reasons:
- it creates a new temporary string when it isn't needed, it also leaks this string anyway
-- it adds a lot of overhead for valid strings, when it shouldn't
- it wont handle both q and b encoding. both types must be handled
- it wont work anyway, the toupper thing wont work. toupper might not work all the time anyway, we should probably fix the code that relies on it working (i think it may be locale dependent).
- at first glance it looks like it has a number of other possible problems with pointer references
So please, don't put the patch at that point in the code, any fixes need to happen 'in place' on the string, we shouldn't be creating another string just to 'fix up' problems we find.
Michael
On Sun, 2004-05-30 at 15:59 +0800, cantona wrote:
On Sat, 2004-05-29 at 19:42 -0400, Jeffrey Stedfast wrote:
> On Sat, 2004-05-29 at 18:13, cantona wrote:
> > Hi all,
> >
> > This is the patch for fix non rfc2047 compliant i18n header.
> > I have tested it with no problem and work properly with valid header.
> > If someone have time, please see it if something I can improve . (I am a
> > newbie coder)
> >
> > Thanks,
> > Cantona
>
> > Index: camel/ChangeLog
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
> > retrieving revision 1.2152
> > diff -u -r1.2152 ChangeLog
> > --- a/camel/ChangeLog 27 May 2004 17:56:51 -0000 1.2152
> > +++ b/camel/ChangeLog 29 May 2004 22:04:56 -0000
> > @@ -1,3 +1,10 @@
> > +2004-05-30 Cantona Su <paradisetux hotmail com>
> > +
> > + * camel-mime-utils.c (camel_fix_non_rfc): Added
> > camel_fix_non_rfc()
> > + for fix non rfc2047 compliant i18n mailer. #58555
> > + (camel_header_decode_string): call camel_fix_non_rfc() before
> > return
> > + the string to header_decode_text().
> > +
> > 2004-05-27 Jeffrey Stedfast <fejj novell com>
> >
> > Fixes bug #59191.
> > Index: camel/camel-mime-utils.c
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/camel/camel-mime-utils.c,v
> > retrieving revision 1.208
> > diff -u -r1.208 camel-mime-utils.c
> > --- a/camel/camel-mime-utils.c 24 May 2004 08:02:10 -0000 1.208
> > +++ b/camel/camel-mime-utils.c 29 May 2004 22:04:58 -0000
> > @@ -70,6 +70,7 @@
> > #define w(x)
> >
> > #define d(x)
> > +#define dd(x)
> > #define d2(x)
>
> would be better to remove dd() once you get a working patch to make it
> so that the patch doesn't alter as many lines without need.
>
> [snip]
>
> > @@ -1174,7 +1175,51 @@
> > {
> > if (in == NULL)
> > return NULL;
> > - return header_decode_text (in, strlen (in), default_charset);
> > + const char *enc;
>
> you can't declare a variable here. you MUST declare it at the top of the
> function in order to comply with ansi-c/c89
>
> > + enc = camel_fix_non_rfc(in);
>
> you have not yet declared camel_fix_non_rfc() so this will produce a
> warning. camel_fix_non_rfc() should be a provate function to this file
> (eg. use the keyword 'static'). Also, this is a bad name... should
> probably be more like fix_broken_rfc2047() or something to that effect.
>
> > + return header_decode_text (enc, strlen (enc),
> > default_charset);
>
> oops. you leaked 'enc'
>
> > +}
> > +
> > +/* fix for non rfc2047 compliant i18n mailers */
> > +const char *
> > +camel_fix_non_rfc(const char *in) {
>
> 1. this should not return const char *, because that's not what it
> actually returns :-)
>
> 2. put the brace on its own line like the rest of the functions in the
> file.
>
> > + GString *out;
> > + const char *str, *start, *temp;
> > + int len, i;
>
> please don't keep changing your indent. just use a single tab.
>
> > +
> > + str = in;
> > + out = g_string_new ("");
> > +
> > + do {
> > + start = strstr(str, "=?");
> > + temp = strstr(toupper(str), "?B?");
>
> this isn't doing what you think it does. read the man page for toupper()
> carefully :-)
>
> also, what about "?Q?" ? :-)
both ?Q? and non encoded-word is go to "len = start - str"
>
> > +
> > + if (start == str && (start = strstr(str+2, "=?")) &&
> > temp) {
> > + len = start - str + 3;
> > + } else {
> > + len = start - str;
> > + }
>
> what is this supposed to be doing?
>
if there is ?B? then it must encoded to =?something?B?something=?=
so it is suppose to "len = start - str + 3", since
"start = strstr(str+2, =?)",
"start" is point to last "=?" in "?b?" encoding. So + 3 for make it to
end of the encoding.
in ?Q? the "start" already in the end of the encoding
> what if initially start == str but the strstr then fails?
both ?Q? and non encoded-word is go to "len = start - str"
>
> > +
> > + dd(printf("encode word %.*s\n\n", len, str));
> > + g_string_append_len(out, str, len);
> > + g_string_append_c(out, ' ');
> > +
> > + temp = out->str;
> > + i = strrchr(temp, '=') - temp + 1;
>
> what if strrchr() returns NULL here?
opps I made a mistake.
> > +
> > + if(i > 0 && temp[i-2] == '?' &&
> > !camel_mime_is_lwsp(temp[i]))
> > + g_string_insert(out, i, ' ');
> > +
> > + if(start >= str)
> > + str += len;
> > +
> > + temp = NULL;
> > + } while (start != NULL);
> > +
> > + dd(printf("encode string %s\n\n", str));
> > + str = out->str;
> > + g_string_free(out, FALSE);
> > + return (const char *)str;
> > }
>
> Jeff
>
Thanks Jeff very much, I will change i and make it better!
Cantona.
>
> _______________________________________________
> evolution-patches mailing list
> evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
