Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread John Fawcett
On 12/25/2016 01:17 AM, Wietse Venema wrote:
> John Fawcett:
>> Revised patch to improve error reporting when no result set containing
>> data is returned
> This code is now part of postfix-3.2-20161224-nonprod, slightly
> edited to simplify error handling. I would be interested to hear
> if it still works with queries that don't call a stored procedure.
>
>   Wietse

Wietse

your release has literally crossed with my testing and preparation of a
revised fix. I will post it now and hope you can use that. I am sorry
for your wasted time. I have done comparisons on this version to
unpatched Postfix. I confirm as you picked up that the old patch gives
incorrect error messages (though the results are consistent) compared to
unpatched Postfix, but in various situations. I had to change the logic.

John




Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

2016-12-25 Thread John Fawcett
On 12/18/2016 09:38 PM, John Fawcett wrote:
> On 12/18/2016 02:09 AM, Wietse Venema wrote:
>> What if Postfix made an old-style query? I think it should just
>> report the old-style error here.
>>
>>  Wietse
> I agree. It might be as simple as changing
>
> msg_warn("dict_mysql: stored procedure did not return any result set
> containing data");
>
> to
>
> msg_warn("mysql query failed: %s", mysql_error(host->db));
>
>
> But I think I need to do some tests with unmodified postfix and then compare 
> behaviour to the patched version to make sure the warnings are really the 
> same.
>
> John
>
I have revised the patch and changed the logic. The error messages are
now in line with the exisiting behaviour for queries that return
results, queries that return no results and for queries that have
errors. There is one minor difference in error messages in the case of a
query that contains no SELECT. The result returned by mysql is
indistinguishable from a stored procedure that contains no SELECT and as
I am explicitly setting errno to ENOSUP, the message given for the query
would now be:

postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
postmap: warning: mysql query failed:
postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
error: Operation not supported

instead of the existing

postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
postmap: warning: mysql query failed:
postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
error: Success

Queries containing errors continue to have errno 0 as currently

postmap -q erba.tv mysql:/etc/postfix/test_query_error.cf
postmap: warning: mysql query failed: Unknown column 'domains' in
'field list'
postmap: fatal: table mysql:/etc/postfix/test_query_error.cf: query
error: Success

though I would opt to set it in this case too.

This version of the patch includes some documentation updates to clarify
that the stored procedure must execute exactly one SELECT statement. The
code has been modified for the revised error reporting and readability.

--- postfix-3.2-20161204_orig/proto/mysql_table2016-10-01
15:44:52.0 +0200
+++ postfix-3.2-20161204/proto/mysql_table2016-12-25
09:04:09.230528031 +0100
@@ -289,6 +289,52 @@
 #certificate.
 # .sp
 #This parameter is available with Postfix 2.11 and later.
+# USING MYSQL STORED PROCEDURES
+# .ad
+# .fi
+#With Postfix 3.2 it is possible to call a stored procedure
+#instead of using a SELECT statement in the query, e.g.
+#
+# .nf
+#query = CALL lookup('%s')
+# .fi
+#
+#The preivously described '%' expansions can be used in the
+#parameter(s) to the stored procedure.
+#
+#The stored procedure must return data with a single result
+#set, that is the stored procedure must execute exactly one
+#SELECT statement on every code path. If you have complex
+#logic and for some paths you want to return no result you
+#will need to include a SELECT statement that returns no
+#rows. One example of a query that returns no rows is
+#
+# .nf
+#SELECT 1 FROM DUAL WHERE FALSE
+# .fi
+#
+#but you may use your own query.
+#
+#Stored procedures that return multiple result sets containing
+#data, that is stored procedures that execute multiple SELECT
+#statements, are not supported.  Stored procedures in
+#mysql produce an additional result set without data which
+#indicates the final status of the stored procedure. If this
+#final status is an error then any previous returned data
+#will not be used.
+#
+# The following is an example of a stored procedure returning
+# a single result set containing data:
+#
+# .nf
+#CREATE [DEFINER=`user`@`host`] PROCEDURE
+#`lookup`(IN `param` VARCHAR(255))
+#READS SQL DATA
+#SQL SECURITY INVOKER
+#BEGIN
+#select goto from alias where address=param;
+#END
+# .fi
 # OBSOLETE QUERY INTERFACE
 # .ad
 # .fi
diff -ur postfix-3.2-20161204_orig/src/global/dict_mysql.c
postfix-3.2-20161204/src/global/dict_mysql.c
--- postfix-3.2-20161204_orig/src/global/dict_mysql.c2016-09-25
17:27:11.0 +0200
+++ postfix-3.2-20161204/src/global/dict_mysql.c2016-12-24
19:56:24.967466042 +0100
@@ -187,6 +187,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef STRCASECMP_IN_STRINGS_H
 #include 
@@ -519,6 +520,10 @@
 {
 HOST   *host;
 MYSQL_RES *res = 0;
+MYSQL_RES *temp_res = 0;
+int status=0;
+int num_result_sets=0;
+int query_error=0;
 
 while ((host = dict_mysql_get_active(dict_mysql)) != NULL) {
 #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 4
@@ -536,9 +541,56 @@
 #endif
 
 if (!(mysql_query(host->db, vstring_str(query {
-if ((res = mysql_store_result(host->db)) == 0) {
+query_error = 0;
+num_result_sets = 0;
+res = 0;
+do {
+temp_res = mysql_store_result(host->db);
+/* did statement ret

Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread John Fawcett
On 12/25/2016 09:30 AM, John Fawcett wrote:
> On 12/25/2016 01:17 AM, Wietse Venema wrote:
>> John Fawcett:
>>> Revised patch to improve error reporting when no result set containing
>>> data is returned
>> This code is now part of postfix-3.2-20161224-nonprod, slightly
>> edited to simplify error handling. I would be interested to hear
>> if it still works with queries that don't call a stored procedure.
>>
>>  Wietse
> Wietse
>
> your release has literally crossed with my testing and preparation of a
> revised fix. I will post it now and hope you can use that. I am sorry
> for your wasted time. I have done comparisons on this version to
> unpatched Postfix. I confirm as you picked up that the old patch gives
> incorrect error messages (though the results are consistent) compared to
> unpatched Postfix, but in various situations. I had to change the logic.
>
> John
>
>
I downloaded and tested your version. It is almost functionally
equivalent to my new version (for queries and stored procedures).

Your version like my new one has the changed errno for queries that have
no select

postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
postmap: warning: mysql query failed:
postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
error: Operation not supported

instead of unpatched postfix

postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
postmap: warning: mysql query failed:
postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
error: Success

The only difference I found functionally was in the case of more than
two result sets. My new version prints the error message only once

postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf
postmap: warning: mysql query failed:
postmap: warning: dict_mysql: multiple result sets returning data are
not supported
postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error:
Operation not supported

instead of this (my old version and your released version)

postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf
postmap: warning: mysql query failed: multiple result sets returning
data are not supported
postmap: warning: mysql query failed: multiple result sets returning
data are not supported
postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error:
Operation not supported

So I confirm your version, unless you think my latest version is more
readable.

John



Re: Postfix delivery problem

2016-12-25 Thread G. Schlisio
> Georg
> 
> I don't think there is enough evidence at the moment to say with
> certainty that any change in glibc has introduced the problem, since you
> were using that for a while now without seeing issues.
> 
> I'd still be interested in knowing what output the test program gives on
> the affected server. 
> 
> best wishes to you too.
> 
> John

oh, right, the test program.

me@dukun ~ > ./test2 
Not found
me@dukun ~ > echo $?
1

i hope this is the information you looked for? i didnt think about it
too much at the moment…
georg


Re: Postfix delivery problem

2016-12-25 Thread John Fawcett
On 12/25/2016 11:10 AM, G. Schlisio wrote:
>> Georg
>>
>> I don't think there is enough evidence at the moment to say with
>> certainty that any change in glibc has introduced the problem, since you
>> were using that for a while now without seeing issues.
>>
>> I'd still be interested in knowing what output the test program gives on
>> the affected server. 
>>
>> best wishes to you too.
>>
>> John
> oh, right, the test program.
>
> me@dukun ~ > ./test2 
> Not found
> me@dukun ~ > echo $?
> 1
>
> i hope this is the information you looked for? i didnt think about it
> too much at the moment…
> georg

thanks. That result could cause some headaches.

To be consistent with the trace posted at http://termbin.com/ram9 and
specifically these lines

Dez 23 12:21:44 dukun postfix/local[4396]: deliver_dotforward[3]: local 
 recip a...@dukun.de exten  deliver a...@dukun.de exp_from
Dez 23 12:21:44 dukun postfix/local[4396]: warning: error looking up passwd 
info for : Invalid argument

the expected result from the test program is a message about invalid
argument. You might try to see if postfix is still giving you the same
trace as before for the user  (with the forward_path workaround
removed) or if now it is working.

That postfix gets one response and the test program a different response
on the same server without changing anything else (like nsswitch.conf)
or restarting the server etc. is puzzling. I suppose the test you tried
in postfix was with that exact same user  and that is not an
anonymization of a longer username.

Whatever the case on your server, I think this issue is going to need to
be addressed, presumably in archlinux or glibc since I found a
reproducable case of your original problem on a stock archlinux install
with glibc 2.24:

sendmail -bv 

Dec 25 11:51:48 archlinux postfix/local[2974]: warning: error looking up
passwd info for : Invalid argument

This is reproducable with the test program (so independently of
Postfix). Could you try to get this addressed otherwise more people are
going to start finding problems with postfix local delivery on archlinux
platform (and potentially with other platforms if the cause is within
glibc itself and those platforms start to update to newer releases). I
say "if the cause is within glibc" since it may be something that also
depends on what nsswitch.conf configurations are being used.

In the example below, the getpwnam_r routine returns a correct answer
for an inexistent user for strings up to 31 chars. From 32 chars onwards
instead of returning not found it retuns EINVAL (invalid argument).

/test AAA
Not found
./test 
getpwnam_r: Invalid argument

John





Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread Wietse Venema
John Fawcett:
[ Charset windows-1252 converted... ]
> On 12/25/2016 09:30 AM, John Fawcett wrote:
> > On 12/25/2016 01:17 AM, Wietse Venema wrote:
> >> John Fawcett:
> >>> Revised patch to improve error reporting when no result set containing
> >>> data is returned
> >> This code is now part of postfix-3.2-20161224-nonprod, slightly
> >> edited to simplify error handling. I would be interested to hear
> >> if it still works with queries that don't call a stored procedure.
> >>
> >>Wietse
> > Wietse
> >
> > your release has literally crossed with my testing and preparation of a
> > revised fix. I will post it now and hope you can use that. I am sorry
> > for your wasted time. I have done comparisons on this version to
> > unpatched Postfix. I confirm as you picked up that the old patch gives
> > incorrect error messages (though the results are consistent) compared to
> > unpatched Postfix, but in various situations. I had to change the logic.
> >
> > John
> >
> >
> I downloaded and tested your version. It is almost functionally
> equivalent to my new version (for queries and stored procedures).
> 
> Your version like my new one has the changed errno for queries that have
> no select
> 
> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
> postmap: warning: mysql query failed:
> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
> error: Operation not supported
> 
> instead of unpatched postfix
> 
> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
> postmap: warning: mysql query failed:
> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
> error: Success

I think that Postfix should not report 'Success' for a failed query.
EOPNOTSUPP or EINVAL would be more appropriate.

> The only difference I found functionally was in the case of more than
> two result sets. My new version prints the error message only once
> 
> postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf
> postmap: warning: mysql query failed:
> postmap: warning: dict_mysql: multiple result sets returning data are
> not supported
> postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error:
> Operation not supported

This can be fixed with inserting a 'break' after every error, but
perhaps this code should not be looping over result sets in the
first place. There are only two iterations, and each iteration does
very different things.

I'll diff your new patch against the previous version and see what
changes you made.


Re: Postfix delivery problem

2016-12-25 Thread Wietse Venema
John Fawcett:
> for an inexistent user for strings up to 31 chars. From 32 chars onwards
> instead of returning not found it retuns EINVAL (invalid argument).
> 
> ./test AAA
> Not found
> ./test 
> getpwnam_r: Invalid argument

Perhaps they want programs to call sysconf(_SC_LOGIN_NAME_MAX)
(note that the result includes the null terminator).

Wietse

#include 
#include 
#include 
#include 
#include 
#include 

int main(int argc, char *argv[])
{
struct passwd pwd;
struct passwd *result;
char   *buf;
size_t  bufsize;
int s;

if (argc != 2) {
fprintf(stderr, "Usage: %s username\n", argv[0]);
exit(EXIT_FAILURE);
}
bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
if (bufsize == -1)  /* Value was indeterminate */
bufsize = 16384;/* Should be more than enough */

buf = malloc(bufsize);
if (buf == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}
/* _SC_LOGIN_NAME_MAX includes the null terminator */
if (strlen(argv[1]) >= sysconf(_SC_LOGIN_NAME_MAX)) {
fprintf(stderr, "warning: name exceeds _SC_LOGIN_NAME_MAX\n");
result = 0;
s = 0;
} else {
s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result);
}
if (result == NULL) {
if (s == 0)
printf("Not found\n");
else {
errno = s;
perror("getpwnam_r");
}
exit(EXIT_FAILURE);
}
printf("Name: %s; UID: %ld\n", pwd.pw_gecos, (long) pwd.pw_uid);
free(buf);
exit(EXIT_SUCCESS);
}


Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread John Fawcett
On 12/25/2016 04:46 PM, Wietse Venema wrote:
> John Fawcett:
> [ Charset windows-1252 converted... ]
>> On 12/25/2016 09:30 AM, John Fawcett wrote:
>>> On 12/25/2016 01:17 AM, Wietse Venema wrote:
 John Fawcett:
> Revised patch to improve error reporting when no result set containing
> data is returned
 This code is now part of postfix-3.2-20161224-nonprod, slightly
 edited to simplify error handling. I would be interested to hear
 if it still works with queries that don't call a stored procedure.

Wietse
>>> Wietse
>>>
>>> your release has literally crossed with my testing and preparation of a
>>> revised fix. I will post it now and hope you can use that. I am sorry
>>> for your wasted time. I have done comparisons on this version to
>>> unpatched Postfix. I confirm as you picked up that the old patch gives
>>> incorrect error messages (though the results are consistent) compared to
>>> unpatched Postfix, but in various situations. I had to change the logic.
>>>
>>> John
>>>
>>>
>> I downloaded and tested your version. It is almost functionally
>> equivalent to my new version (for queries and stored procedures).
>>
>> Your version like my new one has the changed errno for queries that have
>> no select
>>
>> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
>> postmap: warning: mysql query failed:
>> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
>> error: Operation not supported
>>
>> instead of unpatched postfix
>>
>> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf
>> postmap: warning: mysql query failed:
>> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query
>> error: Success
> I think that Postfix should not report 'Success' for a failed query.
> EOPNOTSUPP or EINVAL would be more appropriate.
>
>> The only difference I found functionally was in the case of more than
>> two result sets. My new version prints the error message only once
>>
>> postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf
>> postmap: warning: mysql query failed:
>> postmap: warning: dict_mysql: multiple result sets returning data are
>> not supported
>> postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error:
>> Operation not supported
> This can be fixed with inserting a 'break' after every error, but
> perhaps this code should not be looping over result sets in the
> first place. There are only two iterations, and each iteration does
> very different things.
>
> I'll diff your new patch against the previous version and see what
> changes you made.

if you break out of the loop without reading all the result data you
will get the following mysql error on the next query.

Commands out of sync; you can't run this command now

John

John



Re: Postfix delivery problem

2016-12-25 Thread John Fawcett
On 12/25/2016 05:12 PM, Wietse Venema wrote:
> John Fawcett:
>> for an inexistent user for strings up to 31 chars. From 32 chars onwards
>> instead of returning not found it retuns EINVAL (invalid argument).
>>
>> ./test AAA
>> Not found
>> ./test 
>> getpwnam_r: Invalid argument
> Perhaps they want programs to call sysconf(_SC_LOGIN_NAME_MAX)
> (note that the result includes the null terminator).
>
> Wietse
>
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
>
> int main(int argc, char *argv[])
> {
> struct passwd pwd;
> struct passwd *result;
> char   *buf;
> size_t  bufsize;
> int s;
>
> if (argc != 2) {
> fprintf(stderr, "Usage: %s username\n", argv[0]);
> exit(EXIT_FAILURE);
> }
> bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
> if (bufsize == -1)  /* Value was indeterminate */
> bufsize = 16384;/* Should be more than enough 
> */
>
> buf = malloc(bufsize);
> if (buf == NULL) {
> perror("malloc");
> exit(EXIT_FAILURE);
> }
> /* _SC_LOGIN_NAME_MAX includes the null terminator */
> if (strlen(argv[1]) >= sysconf(_SC_LOGIN_NAME_MAX)) {
> fprintf(stderr, "warning: name exceeds _SC_LOGIN_NAME_MAX\n");
> result = 0;
> s = 0;
> } else {
> s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result);
> }
> if (result == NULL) {
> if (s == 0)
> printf("Not found\n");
> else {
> errno = s;
> perror("getpwnam_r");
> }
> exit(EXIT_FAILURE);
> }
> printf("Name: %s; UID: %ld\n", pwd.pw_gecos, (long) pwd.pw_uid);
> free(buf);
> exit(EXIT_SUCCESS);
> }

I tried that on archlinux. The above program still produces EINVAL for
login names between 32 and 255 inclusive.

_SC_LOGIN_NAME_MAX is 256 on that platform.

John



Re: Postfix delivery problem

2016-12-25 Thread G. Schlisio
> 
> I tried that on archlinux. The above program still produces EINVAL for
> login names between 32 and 255 inclusive.
> 
> _SC_LOGIN_NAME_MAX is 256 on that platform.
> 
> John
> 

hi,

earlier i tried with literal "AA", which was probably not what you
meant.
it produced a "not found".
using an affected user name yields (with both your programs):
getpwnam_r: Invalid argument
while an existing shell account yields its UID:
Name: ; UID: 1001

for me its not clear whats the difference between a literal  string
and an existing username not in passwd (why one produces an error and
one doesnt).

sorry for not reading carefully earlier…

georg


Re: Postfix delivery problem

2016-12-25 Thread John Fawcett
On 12/25/2016 06:30 PM, G. Schlisio wrote:
>> I tried that on archlinux. The above program still produces EINVAL for
>> login names between 32 and 255 inclusive.
>>
>> _SC_LOGIN_NAME_MAX is 256 on that platform.
>>
>> John
>>
> hi,
>
> earlier i tried with literal "AA", which was probably not what you
> meant.
> it produced a "not found".
> using an affected user name yields (with both your programs):
> getpwnam_r: Invalid argument
> while an existing shell account yields its UID:
> Name: ; UID: 1001
>
> for me its not clear whats the difference between a literal  string
> and an existing username not in passwd (why one produces an error and
> one doesnt).
>
> sorry for not reading carefully earlier…
>
> georg

Hi Georg

thanks for that, so at least we have consistent behaviour which is good.
I had got the  from your logging without realizing it was
anonymized.

Now the problem to solve is why the user names you are testing with give
the invalid argument. One reason would be if they are longer than 31
chars. But if they are not then there is something else, for example do
they contain a dot or some other character that archlinux does not
accept in user names?

At this point I suspect that your system is providing EINVAL for any
name that could not be a valid local username (with one exception that
it allows uppercase which is not valid in usernames). According to the
archlinux useradd manual:

   "Usernames must start with a lower case letter or an underscore,
followed by lower case letters, digits,
 underscores, or dashes. They can end with a dollar sign. In
regular expression terms:
 [a-z_][a-z0-9_-]*[$]?

   Usernames may only be up to 32 characters long."

This is a problem because the getpwnam_r routine on archlinux is
deviating from the specifications by not always returning 0 for users
that are not found. This should be addressed in archlinux or upstream in
glibc. Can you take it back to the archlinux list and see if you can get
this addressed?

John



Re: Postfix delivery problem

2016-12-25 Thread John Fawcett
On 12/25/2016 07:40 PM, John Fawcett wrote:
> Hi Georg
> thanks for that, so at least we have consistent behaviour which is good.
> I had got the  from your logging without realizing it was
> anonymized.
>
> Now the problem to solve is why the user names you are testing with give
> the invalid argument. One reason would be if they are longer than 31
> chars. But if they are not then there is something else, for example do
> they contain a dot or some other character that archlinux does not
> accept in user names?
>
> At this point I suspect that your system is providing EINVAL for any
> name that could not be a valid local username (with one exception that
> it allows uppercase which is not valid in usernames). According to the
> archlinux useradd manual:
>
>"Usernames must start with a lower case letter or an underscore,
> followed by lower case letters, digits,
>  underscores, or dashes. They can end with a dollar sign. In
> regular expression terms:
>  [a-z_][a-z0-9_-]*[$]?
>
>Usernames may only be up to 32 characters long."
>
> This is a problem because the getpwnam_r routine on archlinux is
> deviating from the specifications by not always returning 0 for users
> that are not found. This should be addressed in archlinux or upstream in
> glibc. Can you take it back to the archlinux list and see if you can get
> this addressed?
>
> John
>
I managed to find where this is happening. It is not in glibc but in
systemd.

If your /etc/nsswitch.conf has something like this:

passwd: compat mymachines systemd

then the routines that are being used are systemd ones.

The checks being done are here in the function valid_user_group_name:

https://github.com/systemd/systemd/blob/master/src/basic/user-util.c

and in the case that those checks fail then _nss_systemd_getpwnam_r from
systemd libraries returns EINVAL

https://github.com/systemd/systemd/blob/master/src/nss-systemd/nss-systemd.c

if (!valid_user_group_name(name)) {
r = -EINVAL;
goto fail;
}

...

fail:
*errnop = -r;
return NSS_STATUS_UNAVAIL;

So the problem is that systemd version of getpwnam_r is deviating from
the standard of returning 0 for not found users.

Either the systemd library module or the nsswitch.conf is probably what
changed during your upgrade.

John






Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread Wietse Venema
John Fawcett:
> > I'll diff your new patch against the previous version and see what
> > changes you made.
> 
> if you break out of the loop without reading all the result data you
> will get the following mysql error on the next query.
> 
> Commands out of sync; you can't run this command now

Noted. Meanwhile I have separated 1) the code that handles the
result set with the lookup result, from 2) the code that handles
all other result sets.

Without interleaving 1) and 2), the code becomes easier to follow.
It's uploaded as postfix-3.2-20161225-nonprod; please check it out.

Wietse


Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread John Fawcett
On 12/25/2016 08:45 PM, Wietse Venema wrote:
> John Fawcett:
>>> I'll diff your new patch against the previous version and see what
>>> changes you made.
>> if you break out of the loop without reading all the result data you
>> will get the following mysql error on the next query.
>>
>> Commands out of sync; you can't run this command now
> Noted. Meanwhile I have separated 1) the code that handles the
> result set with the lookup result, from 2) the code that handles
> all other result sets.
>
> Without interleaving 1) and 2), the code becomes easier to follow.
> It's uploaded as postfix-3.2-20161225-nonprod; please check it out.
>
>   Wietse
I confirm it behaves as expected on positive and negative cases. The
code is indeed much easier to follow.

John


Re: MySQL stored-procedure support for Postfix 3.2

2016-12-25 Thread Wietse Venema
John Fawcett:
> > Noted. Meanwhile I have separated 1) the code that handles the
> > result set with the lookup result, from 2) the code that handles
> > all other result sets.
> >
> > Without interleaving 1) and 2), the code becomes easier to follow.
> > It's uploaded as postfix-3.2-20161225-nonprod; please check it out.
> >
> I confirm it behaves as expected on positive and negative cases. The
> code is indeed much easier to follow.

I see room for two improvements:

- Don't loop on mysql_next_result() if that function returns an
  error, to avoid going into an infinite loop.

- Do call mysql_next_result() after mysql_store_result() etc. error
  just to be sure that we stay in sync. This won't log any Postfix
  warnings when the query_error flag was already raised.

Wietse