pgsql: Force strings passed to and from plperl to be in UTF8 encoding.

On 02/06/2011 05:31 PM, Andrew Dunstan wrote:

Force strings passed to and from plperl to be in UTF8 encoding.

String are converted to UTF8 on the way into perl and to the
database encoding on the way back. This avoids a number of
observed anomalies, and ensures Perl a consistent view of the
world.

Some minor code cleanups are also accomplished.

Alex Hunsaker, reviewed by Andy Colson.

This has broken the buildfarm :-(

perl ppport.h reports:

*** WARNING: Uses HeUTF8, which may not be portable below perl
5.11.0, even with 'ppport.h'

Experimentation on a CentOS machine suggests we can cure it with this:

#ifndef HeUTF8
#define HeUTF8(he) ((HeKLEN(he) == HEf_SVKEY) ? \
SvUTF8(HeKEY_sv(he))
: \
(U32)HeKUTF8(he))
#endif

cheers

andrew

On Sun, Feb 6, 2011 at 18:02, Andrew Dunstan <andrew@dunslane.net> wrote:

On 02/06/2011 05:31 PM, Andrew Dunstan wrote:

Force strings passed to and from plperl to be in UTF8 encoding.

String are converted to UTF8 on the way into perl and to the
database encoding on the way back. This avoids a number of
observed anomalies, and ensures Perl a consistent view of the
world.

Some minor code cleanups are also accomplished.

Alex Hunsaker, reviewed by Andy Colson.

This has broken the buildfarm :-(

Drat.

perl ppport.h reports:

  *** WARNING: Uses HeUTF8, which may not be portable below perl
  5.11.0, even with 'ppport.h'
Experimentation on a CentOS machine suggests we can cure it with this:

  #ifndef HeUTF8
  #define HeUTF8(he)             ((HeKLEN(he) == HEf_SVKEY) ?            \
                                   SvUTF8(HeKEY_sv(he))
  :                 \
                                   (U32)HeKUTF8(he))
  #endif

Yeah, that should work. BTW I would have loved to add some regression
tests for some of this (like the example hek2cstr states). Is there
any way to do that?

On 02/06/2011 09:13 PM, Alex Hunsaker wrote:

I would have loved to add some regression
tests for some of this (like the example hek2cstr states). Is there
any way to do that?

I can't think of an obvious way. Anyone else?

cheers

ndrew

On Sun, Feb 6, 2011 at 15:31, Andrew Dunstan <andrew@dunslane.net> wrote:

Force strings passed to and from plperl to be in UTF8 encoding.

String are converted to UTF8 on the way into perl and to the
database encoding on the way back. This avoids a number of
observed anomalies, and ensures Perl a consistent view of the
world.

So I noticed a problem while playing with this in my discussion with
David Wheeler. pg_do_encoding() does nothing when the src encoding ==
the dest encoding. That means on a UTF-8 database we fail make sure
our strings are valid utf8.

An easy way to see this is to embed a null in the middle of a string:
=> create or replace function zerob() returns text as $$ return
"abcd\0efg"; $$ language plperl;
=> SELECT zerob();
abcd

Also It seems bogus to bogus to do any encoding conversion when we are
SQL_ASCII, and its really trivial to fix.

With the attached:
- when we are on a utf8 database make sure to verify our output string
in sv2cstr (we assume database strings coming in are already valid)

- Do no string conversion when we are SQL_ASCII in or out

- add plperl_helpers.h as a dep to plperl.o in our makefile

- remove some redundant calls to pg_verify_mbstr()

- as utf_e2u only as one caller dont pstrdup() instead have the caller
check (saves some cycles and memory)

On 12 February 2011 14:48, Alex Hunsaker <badalex@gmail.com> wrote:

On Sun, Feb 6, 2011 at 15:31, Andrew Dunstan <andrew@dunslane.net> wrote:

Force strings passed to and from plperl to be in UTF8 encoding.

String are converted to UTF8 on the way into perl and to the
database encoding on the way back. This avoids a number of
observed anomalies, and ensures Perl a consistent view of the
world.

So I noticed a problem while playing with this in my discussion with
David Wheeler. pg_do_encoding() does nothing when the src encoding ==
the dest encoding. That means on a UTF-8 database we fail make sure
our strings are valid utf8.

An easy way to see this is to embed a null in the middle of a string:
=> create or replace function zerob() returns text as $$ return
"abcd\0efg"; $$ language plperl;
=> SELECT zerob();
abcd

Also It seems bogus to bogus to do any encoding conversion when we are
SQL_ASCII, and its really trivial to fix.

With the attached:
- when we are on a utf8 database make sure to verify our output string
in sv2cstr (we assume database strings coming in are already valid)

- Do no string conversion when we are SQL_ASCII in or out

- add plperl_helpers.h as a dep to plperl.o in our makefile

- remove some redundant calls to pg_verify_mbstr()

- as utf_e2u only as one caller dont pstrdup() instead have the caller
check (saves some cycles and memory)

Is there a plan to commit this issue? I am still seeing this issue on
PG 9.1 STABLE branch. Attached is a small patch that targets only the
specific issue in the described testcase :

create or replace function zerob() returns text as $$ return
"abcd\0efg"; $$ language plperl;
SELECT zerob();

The patch does the perl data validation in the function utf_u2e() itself.

On Mon, Oct 3, 2011 at 04:20, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

Is there a plan to commit this issue? I am still seeing this issue on
PG 9.1 STABLE branch. Attached is a small patch that targets only the
specific issue in the described testcase :

create or replace function zerob() returns text as $$ return
"abcd\0efg"; $$ language plperl;
SELECT zerob();

The patch does the perl data validation in the function utf_u2e() itself.

I think thats fine, but as coded it will verify the string twice in
the GetDatabaseEncoding() != PG_UTF8 case (once for
pg_do_encoding_conversion() and again with the added
pg_verify_mbstr_len), which seems a bit wasteful.

It might be worth adding a regression test also...

On 3 October 2011 22:37, Alex Hunsaker <badalex@gmail.com> wrote:

On Mon, Oct 3, 2011 at 04:20, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

Is there a plan to commit this issue? I am still seeing this issue on
PG 9.1 STABLE branch. Attached is a small patch that targets only the
specific issue in the described testcase :

create or replace function zerob() returns text as $$ return
"abcd\0efg"; $$ language plperl;
SELECT zerob();

The patch does the perl data validation in the function utf_u2e() itself.

I think thats fine, but as coded it will verify the string twice in
the GetDatabaseEncoding() != PG_UTF8 case (once for
pg_do_encoding_conversion() and again with the added
pg_verify_mbstr_len), which seems a bit wasteful.

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. That's the
reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
condition.

It might be worth adding a regression test also...

I could not find any basic pl/perl tests in the regression
serial_schedule. I am not sure if we want to add just this scenario
without any basic tests for pl/perl ?

On 04.10.2011 08:35, Amit Khandekar wrote:

On 3 October 2011 22:37, Alex Hunsaker<badalex@gmail.com> wrote:

It might be worth adding a regression test also...

I could not find any basic pl/perl tests in the regression
serial_schedule. I am not sure if we want to add just this scenario
without any basic tests for pl/perl ?

See src/pl/plperl/[sql|expected]

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. That's the
reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
condition.

Consider a latin1 database where utf8_str was a string of ascii
characters. Then no conversion would take place and ret == utf8_str
but the string would be verified by pg_do_encdoing_conversion() and
verified again by your added check :-).

It might be worth adding a regression test also...

I could not find any basic pl/perl tests in the regression
serial_schedule. I am not sure if we want to add just this scenario
without any basic tests for pl/perl ?

I went ahead and added one in the attached based upon your example.

Look ok to you?

BTW thanks for the patch!

[ side note ]
I still think we should not be doing any conversion in the SQL_ASCII
case but this slimmed down patch should be less controversial.

On 4 October 2011 14:04, Alex Hunsaker <badalex@gmail.com> wrote:

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. That's the
reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
condition.

Consider a latin1 database where utf8_str was a string of ascii
characters. Then no conversion would take place and ret == utf8_str
but the string would be verified by pg_do_encdoing_conversion() and
verified again by your added check :-).

It might be worth adding a regression test also...

I could not find any basic pl/perl tests in the regression
serial_schedule. I am not sure if we want to add just this scenario
without any basic tests for pl/perl ?

I went ahead and added one in the attached based upon your example.

Look ok to you?

+ 	if(GetDatabaseEncoding() == PG_UTF8)
+ 		pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

In your patch, the above will again skip mb-validation if the database
encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
the un-converted string even if *one* of the src and dest encodings is
SQL_ASCII.

I think :
        if (ret == utf8_str)
+       {
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
                ret = pstrdup(ret);
+       }

This (ret == utf8_str) condition would be a reliable way for knowing
whether pg_do_encoding_conversion() has done the conversion at all.

I am ok with the new test. Thanks for doing it yourself !

On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

On 4 October 2011 14:04, Alex Hunsaker <badalex@gmail.com> wrote:

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. [...]

Consider a latin1 database where utf8_str was a string of ascii
characters. [...]

[Patch] Look ok to you?

+       if(GetDatabaseEncoding() == PG_UTF8)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

In your patch, the above will again skip mb-validation if the database
encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
the un-converted string even if *one* of the src and dest encodings is
SQL_ASCII.

*scratches head* I thought the point of SQL_ASCII was no encoding
conversion was done and so there would be nothing to verify.

Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
NULL bytes in the string when we are a single byte encoding.

I think :
       if (ret == utf8_str)
+       {
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
               ret = pstrdup(ret);
+       }

This (ret == utf8_str) condition would be a reliable way for knowing
whether pg_do_encoding_conversion() has done the conversion at all.

Yes. However (and maybe im nitpicking here), I dont see any reason to
verify certain strings twice if we can avoid it.

What do you think about:
+    /*
+    * when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
+    * will not do any conversion or verification. we need to do it
manually instead.
+    */
+       if( GetDatabaseEncoding() == PG_UTF8 ||
              GetDatabaseEncoding() == SQL_ASCII)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

On 4 October 2011 22:57, Alex Hunsaker <badalex@gmail.com> wrote:

On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

On 4 October 2011 14:04, Alex Hunsaker <badalex@gmail.com> wrote:

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. [...]

Consider a latin1 database where utf8_str was a string of ascii
characters. [...]

[Patch] Look ok to you?

+       if(GetDatabaseEncoding() == PG_UTF8)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

In your patch, the above will again skip mb-validation if the database
encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
the un-converted string even if *one* of the src and dest encodings is
SQL_ASCII.

*scratches head* I thought the point of SQL_ASCII was no encoding
conversion was done and so there would be nothing to verify.

Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
NULL bytes in the string when we are a single byte encoding.

I think :
       if (ret == utf8_str)
+       {
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
               ret = pstrdup(ret);
+       }

This (ret == utf8_str) condition would be a reliable way for knowing
whether pg_do_encoding_conversion() has done the conversion at all.

Yes. However (and maybe im nitpicking here), I dont see any reason to
verify certain strings twice if we can avoid it.

What do you think about:
+    /*
+    * when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
+    * will not do any conversion or verification. we need to do it
manually instead.
+    */
+       if( GetDatabaseEncoding() == PG_UTF8 ||
             GetDatabaseEncoding() == SQL_ASCII)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

You mean the final changes in plperl_helpers.h would look like
something like this right? :

static inline char *
utf_u2e(const char *utf8_str, size_t len)
{
char *ret = (char *) pg_do_encoding_conversion((unsigned
char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

        if (ret == utf8_str)
+       {
+               if (GetDatabaseEncoding() == PG_UTF8 ||
+                       GetDatabaseEncoding() == PG_SQL_ASCII)
+               {
+                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+               }
+
                ret = pstrdup(ret);
+       }
        return ret;
 }

Yeah I am ok with that. It's just an additional check besides (ret ==
utf8_str) to know if we really require validation.

On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

On 4 October 2011 22:57, Alex Hunsaker <badalex@gmail.com> wrote:

On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

On 4 October 2011 14:04, Alex Hunsaker <badalex@gmail.com> wrote:

On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. [...]

Consider a latin1 database where utf8_str was a string of ascii
characters. [...]

[Patch] Look ok to you?

+       if(GetDatabaseEncoding() == PG_UTF8)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

In your patch, the above will again skip mb-validation if the database
encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
the un-converted string even if *one* of the src and dest encodings is
SQL_ASCII.

*scratches head* I thought the point of SQL_ASCII was no encoding
conversion was done and so there would be nothing to verify.

Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
NULL bytes in the string when we are a single byte encoding.

I think :
       if (ret == utf8_str)
+       {
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
               ret = pstrdup(ret);
+       }

This (ret == utf8_str) condition would be a reliable way for knowing
whether pg_do_encoding_conversion() has done the conversion at all.

Yes. However (and maybe im nitpicking here), I dont see any reason to
verify certain strings twice if we can avoid it.

What do you think about:
+    /*
+    * when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
+    * will not do any conversion or verification. we need to do it
manually instead.
+    */
+       if( GetDatabaseEncoding() == PG_UTF8 ||
             GetDatabaseEncoding() == SQL_ASCII)
+               pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);

You mean the final changes in plperl_helpers.h would look like
something like this right? :

 static inline char *
 utf_u2e(const char *utf8_str, size_t len)
 {
       char       *ret = (char *) pg_do_encoding_conversion((unsigned
char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

       if (ret == utf8_str)
+       {
+               if (GetDatabaseEncoding() == PG_UTF8 ||
+                       GetDatabaseEncoding() == PG_SQL_ASCII)
+               {
+                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+               }
+
               ret = pstrdup(ret);
+       }
       return ret;
 }

Yes.

On Wed, Oct 5, 2011 at 00:30, Alex Hunsaker <badalex@gmail.com> wrote:

On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

You mean the final changes in plperl_helpers.h would look like
something like this right? :

 static inline char *
 utf_u2e(const char *utf8_str, size_t len)
 {
       char       *ret = (char *) pg_do_encoding_conversion((unsigned
char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

       if (ret == utf8_str)
+       {
+               if (GetDatabaseEncoding() == PG_UTF8 ||
+                       GetDatabaseEncoding() == PG_SQL_ASCII)
+               {
+                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+               }
+
               ret = pstrdup(ret);
+       }
       return ret;
 }

Yeah I am ok with that. It's just an additional check besides (ret ==
utf8_str) to know if we really require validation.

Find it attached. [ Note I didn't put the check inside the if (ret ==
utf8_str) as it seemed a bit cleaner (indentation wise) to have it
outside ]

On 5 October 2011 12:29, Alex Hunsaker <badalex@gmail.com> wrote:

On Wed, Oct 5, 2011 at 00:30, Alex Hunsaker <badalex@gmail.com> wrote:

On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

You mean the final changes in plperl_helpers.h would look like
something like this right? :

 static inline char *
 utf_u2e(const char *utf8_str, size_t len)
 {
       char       *ret = (char *) pg_do_encoding_conversion((unsigned
char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());

       if (ret == utf8_str)
+       {
+               if (GetDatabaseEncoding() == PG_UTF8 ||
+                       GetDatabaseEncoding() == PG_SQL_ASCII)
+               {
+                       pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+               }
+
               ret = pstrdup(ret);
+       }
       return ret;
 }

Yeah I am ok with that. It's just an additional check besides (ret ==
utf8_str) to know if we really require validation.

Find it attached. [ Note I didn't put the check inside the if (ret ==
utf8_str) as it seemed a bit cleaner (indentation wise) to have it
outside ]

I have no more issues with the patch.
Thanks!
-Amit

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

I have no more issues with the patch.
Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Oct 5, 2011 at 08:18, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

I have no more issues with the patch.
Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

On Wed, Oct 5, 2011 at 5:03 PM, Alex Hunsaker <badalex@gmail.com> wrote:

On Wed, Oct 5, 2011 at 08:18, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

I have no more issues with the patch.
Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue. Other people may have different
opinions, of course.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Oct 5, 2011, at 7:36 PM, Robert Haas wrote:

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue. Other people may have different
opinions, of course.

Proper encoding of text data to and from Perl is more important every day, as it's used for more and more uses beyond ASCII. I think it's important to backport such issues modulo behavioral changes.

Best,

David

On Wed, Oct 5, 2011 at 20:36, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 5:03 PM, Alex Hunsaker <badalex@gmail.com> wrote:

On Wed, Oct 5, 2011 at 08:18, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

I have no more issues with the patch.
Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue.  Other people may have different
opinions, of course.

Added. :-)

On 10/07/2011 12:51 PM, Alex Hunsaker wrote:

On Wed, Oct 5, 2011 at 20:36, Robert Haas<robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 5:03 PM, Alex Hunsaker<badalex@gmail.com> wrote:

On Wed, Oct 5, 2011 at 08:18, Robert Haas<robertmhaas@gmail.com> wrote:

On Wed, Oct 5, 2011 at 3:58 AM, Amit Khandekar
<amit.khandekar@enterprisedb.com> wrote:

I have no more issues with the patch.
Thanks!

I think this patch needs to be added to the open CommitFest, with
links to the reviews, and marked Ready for Committer.

The open commitfest? Even if its an "important" bug fix that should be
backpatched?

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue. Other people may have different
opinions, of course.

Added. :-)

I'm just starting to look at this, by way of a break in staring at
pg_dump code ;-). This just needs to be backpatched to 9.1, is that right?

cheers

andrew

On Wed, Nov 2, 2011 at 17:12, Andrew Dunstan <andrew@dunslane.net> wrote:

Considering that the issue appears to have been ignored from
mid-February until early October, I don't see why it should now get to
jump to the head of the queue.  Other people may have different
opinions, of course.

Added. :-)

I'm just starting to look at this, by way of a break in staring at pg_dump
code ;-). This just needs to be backpatched to 9.1, is that right?

Yes please.

9.0 did not have this problem (or at least if it did it was a separate issue).

On 10/05/2011 03:58 AM, Amit Khandekar wrote:

On 5 October 2011 12:29, Alex Hunsaker<badalex@gmail.com> wrote:

Find it attached. [ Note I didn't put the check inside the if (ret ==
utf8_str) as it seemed a bit cleaner (indentation wise) to have it
outside ]

I have no more issues with the patch.

Committed.

cheers

andrew