Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Piotr Caban]

Hi,

On 11/02/15 11:08, Martin Storsjo wrote:
> @@ -724,12 +724,13 @@ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MS
>       struct _str_ctx_a ctx = {len, str};
>       int ret;
>
> -    if (options != UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION &&
> -        options != UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR)
> +    if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK)
>           FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
> -    ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : puts_clbk_str_a,
> +    ret = pf_printf_a(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ? puts_clbk_str_a : puts_clbk_str_c99_a,
>               &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist);
>       puts_clbk_str_a(&ctx, 1, &nullbyte);
> +    if ((options & UCRTBASE_PRINTF_TERMINATION_MASK) == 0 && ret >= len)
> +        ret = -2;
>       return ret;
>   }
> This code looks like you're trying to make the things other way around.
> Shouldn't UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR flag impact the
> callback being used? On the other hand
> UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION flag should probably
> only affect terminating NULL related behavior.

Maybe following code makes more sense:
ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR 
? puts_clbk_str_c99_a : puts_clbk_str_a, &ctx, format, locale, FALSE, 
FALSE, arg_clbk_valist, NULL, &valist);
if(puts_clbk_str_a(&ctx, 1, &nullbyte)==-1 && !(options & 
UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)) {
     if(len) str[len-1] = 0;
     return -2;
}
return ret;

What do you think about it?

Thanks,
Piotr

[PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Martin Storsjö]

On Mon, 2 Nov 2015, Piotr Caban wrote:
> Hi,
>
> On 11/02/15 11:08, Martin Storsjo wrote:
> > @@ -724,12 +724,13 @@ int CDECL MSVCRT__stdio_common_vsprintf( unsigned 
> > __int64 options, char *str, MS
> >       struct _str_ctx_a ctx = {len, str};
> >       int ret;
> >
> > -    if (options != UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION &&
> > -        options != UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR)
> > +    if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK)
> >           FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
> > -    ret = pf_printf_a(options & 
> > UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : 
> > puts_clbk_str_a,
> > +    ret = pf_printf_a(options & 
> > UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ? puts_clbk_str_a : 
> > puts_clbk_str_c99_a,
> >               &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, 
> > &valist);
> >       puts_clbk_str_a(&ctx, 1, &nullbyte);
> > +    if ((options & UCRTBASE_PRINTF_TERMINATION_MASK) == 0 && ret >= len)
> > +        ret = -2;
> >       return ret;
> >   }
> > This code looks like you're trying to make the things other way around.
> > Shouldn't UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR flag impact the
> > callback being used? On the other hand
> > UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION flag should probably
> > only affect terminating NULL related behavior.

Actually, I don't think of them as two separate flags, but as having 3 
different modes (with the fourth, with both flags being set, undefined). 
But I see what you're getting at.
> Maybe following code makes more sense:
> ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? 
> puts_clbk_str_c99_a : puts_clbk_str_a, &ctx, format, locale, FALSE, FALSE, 
> arg_clbk_valist, NULL, &valist);
> if(puts_clbk_str_a(&ctx, 1, &nullbyte)==-1 && !(options & 
> UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)) {
>    if(len) str[len-1] = 0;
>    return -2;
> }
> return ret;
>
> What do you think about it?

I guess this would work as well, although it took me a little while to 
figure out why (why the return -2 case won't be invoked when the standard 
snprintf behaviour is requested - because the nullbyte write will always 
succeed since the c99 callback left space for it).

After thinking another few minutes about it - sure, this is fine for me. 
Should I resend the full patchset, or only 4->10 (at least some of them 
will need some conflict resolution)? If I resend the full patchset, I 
guess I can fold in your sign-offs in 1-3 (as long as I don't modify those 
patches)?

// Martin

[PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Piotr Caban]

Hi,

I don't like the code I've proposed in last email. Something like this 
seems to be nicer:
static int puts_clbk_str_c99_a(void *ctx, int len, const char *str)
{
     struct _str_ctx_a *out = ctx;

     if(!out->buf)
         return len;

     if(out->len < len) {
         memcpy(out->buf, str, out->len);
         out->buf += out->len;
         out->len = 0;
         return len;
     }

     memcpy(out->buf, str, len);
     out->buf += len;
     out->len -= len;
     return len;
}

/*********************************************************************
  *              __stdio_common_vsprintf (MSVCRT.@)
  */
int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char 
*str, MSVCRT_size_t len, const char *format,
                                          MSVCRT__locale_t locale, 
__ms_va_list valist )
{
     static const char nullbyte = '\0';
     struct _str_ctx_a ctx = {len, str};
     int ret;

     if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK)
         FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
     ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale,
             FALSE, FALSE, arg_clbk_valist, NULL, &valist);
     puts_clbk_str_a(&ctx, 1, &nullbyte);

     if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)
         return ret>len ? -1 : ret;
     if(ret>=len) {
         if(len) str[len-1] = 0;
         return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) 
? ret : -2;
     }
     return ret;
}
if you think that your initial code is better please let me know. 
Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not needed.

On 11/02/15 13:45, Martin Storsjö wrote:
> After thinking another few minutes about it - sure, this is fine for me.
> Should I resend the full patchset, or only 4->10 (at least some of them
> will need some conflict resolution)? If I resend the full patchset, I
> guess I can fold in your sign-offs in 1-3 (as long as I don't modify
> those patches)?
> I can sign the patches after you resend them myself (I don't know what's 
> the policy in this case).

Thanks,
Piotr

[PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Alexandre Julliard]

Piotr Caban <piotr.caban(a)gmail.com> writes:
> On 11/02/15 13:45, Martin Storsjö wrote:
> > After thinking another few minutes about it - sure, this is fine for me.
> > Should I resend the full patchset, or only 4->10 (at least some of them
> > will need some conflict resolution)? If I resend the full patchset, I
> > guess I can fold in your sign-offs in 1-3 (as long as I don't modify
> > those patches)?
> > I can sign the patches after you resend them myself (I don't know
> > what's the policy in this case).

I prefer if you sign them again after a resend, if it's folded into the
patch email I won't notice it right away. But if the already signed off
patches are still valid, it's even better to wait for me to commit them,
and then resend only the remaining ones.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Martin Storsjö]

On Mon, 2 Nov 2015, Alexandre Julliard wrote:
> Piotr Caban <piotr.caban(a)gmail.com> writes:
> > On 11/02/15 13:45, Martin Storsjö wrote:
> > > After thinking another few minutes about it - sure, this is fine for me.
> > > Should I resend the full patchset, or only 4->10 (at least some of them
> > > will need some conflict resolution)? If I resend the full patchset, I
> > > guess I can fold in your sign-offs in 1-3 (as long as I don't modify
> > > those patches)?
> > > I can sign the patches after you resend them myself (I don't know
> > > what's the policy in this case).
>
> I prefer if you sign them again after a resend, if it's folded into the
> patch email I won't notice it right away. But if the already signed off
> patches are still valid, it's even better to wait for me to commit them,
> and then resend only the remaining ones.

They are still valid, so I'll resend the rest of the branch later after 
today's push.

// Martin

[PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

[Martin Storsjö]

On Mon, 2 Nov 2015, Piotr Caban wrote:
> Hi,
>
> I don't like the code I've proposed in last email. Something like this seems 
> to be nicer:
> static int puts_clbk_str_c99_a(void *ctx, int len, const char *str)
> {
>    struct _str_ctx_a *out = ctx;
>
> if(!out->buf)
>        return len;
>
> if(out->len < len) {
>        memcpy(out->buf, str, out->len);
>        out->buf += out->len;
>        out->len = 0;
>        return len;
>    }
>
> memcpy(out->buf, str, len);
>    out->buf += len;
>    out->len -= len;
>    return len;
> }
>
> /*********************************************************************
> *              __stdio_common_vsprintf (MSVCRT.@)
> */
> int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, 
> MSVCRT_size_t len, const char *format,
>                                         MSVCRT__locale_t locale, 
> __ms_va_list valist )
> {
>    static const char nullbyte = '\0';
>    struct _str_ctx_a ctx = {len, str};
>    int ret;
>
> if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK)
>        FIXME("options %s not handled\n", wine_dbgstr_longlong(options));
>    ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale,
>            FALSE, FALSE, arg_clbk_valist, NULL, &valist);
>    puts_clbk_str_a(&ctx, 1, &nullbyte);
>
> if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)
>        return ret>len ? -1 : ret;
>    if(ret>=len) {
>        if(len) str[len-1] = 0;
>        return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) ? ret 
> : -2;
>    }
>    return ret;
> }
> if you think that your initial code is better please let me know.

No, this feels like the most straightforward and explicit way of 
implementing it so far - thanks!
> Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not 
> needed.

I'd keep it here (as condition for the fixme warning - alternatively add 
"|| options == 0" to the condition), but remove it once that warning is 
removed in a later patch.

// Martin