Re: [4/5] msvcrt: Move write flag setting code for reuse.

[Piotr Caban]

On 06/14/14 01:23, Grazvydas Ignotas wrote:
> dlls/msvcrt/file.c |   27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c
> index 7523bbb..d6fb9ef 100644
> --- a/dlls/msvcrt/file.c
> +++ b/dlls/msvcrt/file.c
> @@ -3641,6 +3641,17 @@ MSVCRT_wchar_t * CDECL MSVCRT_fgetws(MSVCRT_wchar_t *s, int size, MSVCRT_FILE* f
>     return buf_start;
>   }
>
> +static inline int msvcrt_set_write_direction(MSVCRT_FILE* file)
> +{
> - if(!(file->_flag & MSVCRT__IOWRT)) {
> -
>    if(file->_flag & MSVCRT__IORW)
> -
>        file->_flag |= MSVCRT__IOWRT;
> -
>    else
> -
>        return MSVCRT_EOF;
> - }
> - return 0;
>
> +}

I don't see a good reason for making this code reusable. The code is 
used to inform how the buffer is used (if it's used for writing or 
reading). This means that it doesn't make sense to set it if buffer is 
not used. The only valid use of it I can see in wine sources is _flsbuf.

I've wrote a test that shows that _IOWRT flag is not set if _bufsiz==0 
or in the case you're implementing. You can see it's code and result on 
windows here:
https://testbot.winehq.org/JobDetails.pl?Key=7552

[4/5] msvcrt: Move write flag setting code for reuse.

[Grazvydas Ignotas]

On Tue, Jun 17, 2014 at 1:16 PM, Piotr Caban  wrote:
> On 06/14/14 01:23, Grazvydas Ignotas wrote:
> > dlls/msvcrt/file.c |   27 +++++++++++++++------------
> >   1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c
> > index 7523bbb..d6fb9ef 100644
> > --- a/dlls/msvcrt/file.c
> > +++ b/dlls/msvcrt/file.c
> > @@ -3641,6 +3641,17 @@ MSVCRT_wchar_t * CDECL MSVCRT_fgetws(MSVCRT_wchar_t
> > *s, int size, MSVCRT_FILE* f
> >     return buf_start;
> >   }
> >
> > +static inline int msvcrt_set_write_direction(MSVCRT_FILE* file)
> > +{
> > - if(!(file->_flag & MSVCRT__IOWRT)) {
> > -
> >    if(file->_flag & MSVCRT__IORW)
> > -
> >        file->_flag |= MSVCRT__IOWRT;
> > -
> >    else
> > -
> >        return MSVCRT_EOF;
> > - }
> > - return 0;
> >
> > +}
>
> I don't see a good reason for making this code reusable. The code is used to
> inform how the buffer is used (if it's used for writing or reading). This
> means that it doesn't make sense to set it if buffer is not used. The only
> valid use of it I can see in wine sources is _flsbuf.
>
> I've wrote a test that shows that _IOWRT flag is not set if _bufsiz==0 or in
> the case you're implementing. You can see it's code and result on windows
> here:
> https://testbot.winehq.org/JobDetails.pl?Key=7552

I see, thanks for the test.

Not setting the flag in that case however uncovers another problem
where after rewind/fflush _cnt is not 0, so _flsbuf doesn't get called
and _IOWRT doesn't get set, as a result next fflush doesn't flush the
buffer at all. I've added a few patches that try to deal with that.