Re: [Nmh-workers] 1.1rc3 - add rfc3461 DSN support

[Ralph Corderoy]

Hi Valdis,

Just some complaints about the patch.  Feel free to ignore as you'd made
the effort and I haven't  :-)

>  int
> -sm_winit (int mode, char *from)
> +sm_winit (int mode, char *from, char *ret, char *envid)
>  {
>      char *smtpcom;
> +    char tmpstr[BUFSIZ];
> +
> +    tmpstr[0] = '\0';
>  
>  #ifdef MPOP
>      if (sm_ispool && !sm_wfp) {
> @@ -544,7 +547,18 @@ sm_winit (int mode, char *from)
>             break;
>      }
>  
> -    switch (smtalk (SM_MAIL, "%s FROM:<%s>", smtpcom, from)) {
> +    if ((ret != NULLCP || envid != NULLCP) && EHLOset ("DSN")) {
> +       if (ret != NULLCP && strlen(ret) > 8) ret[8] = '\0';
> +       if (ret) sprintf(tmpstr, " RET=%s", ret);
> +       if (envid != NULLCP && strlen(envid) > 100) envid[100] = '\0';
> +       if (envid) {
> +           strcat(tmpstr, " ENVID=");
> +           strcat(tmpstr, envid);
> +       }
> +    }
> +
> +    switch (smtalk (SM_MAIL, "%s FROM:<%s>%s", smtpcom, from,
> +               tmpstr ? tmpstr : "")) {

I'd hate to see NULLCP, NULLIP, etc., make an appearance.  The above
would seem less noisy as

    if ((ret || envid) && EHLOset("DSN")) {
        if (ret) {
            if (strlen(ret) > 8) ret[8] = '\0';
            sprintf(tmpstr, " RET=%s", ret);
        }
        if (envid) {
            if (strlen(envid) > 100) envid[100] = '\0';
            strcat(tmpstr, " ENVID=");
            strcat(tmpstr, envid);
        }
    }

Also,

    switch (smtalk (SM_MAIL, "%s FROM:<%s>%s", smtpcom, from,
        tmpstr ? tmpstr : "")) {

is checking tmpstr which will always be true.  Given *tmpstr is
initialised to 0 it may as well me just

    switch (smtalk(SM_MAIL, "%s FROM:<%s>%s", smtpcom, from, tmpstr)) {

This occurs elsewhere in the patch too.

> +static char *notify = NULLCP, *ret = NULLCP, *envid = NULLCP;

Need these have an explicit notification?

Cheers,


Ralph.