Re: [1/4] msvcrt/tests: Add a test for fwrite flushing behavior. (try 2)

[Piotr Caban]

Hi,

On 06/13/14 01:27, Grazvydas Ignotas wrote:
> +static void test_write_flush(void)
> +{
> - char iobuf[1024];
> - char *tempf;
> - FILE *file;
> -
> - tempf = _tempnam(".","wne");
> - file = fopen(tempf, "wb+");
> - ok(file != NULL, "unable to create test file\n");
> - /* the default should be 4K */
> - test_write_flush_size(file, 4096);

It would be nice to read the size from the file->_bufsiz instead of 
hard-coding 4096. This value is not initialized while the file is opened 
so you will need to call something before reading it's value.
> - setvbuf(file, iobuf, _IOFBF, sizeof(iobuf));
> - test_write_flush_size(file, sizeof(iobuf));
> - fclose(file);

Current implemententation of setvbuf is broken in wine. This code will 
cause free(iobuf) to be called. I think that setvbuf needs to be fixed 
before this test can be added.
> - unlink(tempf);
> - free(tempf);

_tempnam returns internal buffer, free should not be called on it.

Thanks,
Piotr

[1/4] msvcrt/tests: Add a test for fwrite flushing behavior. (try 2)

[Grazvydas Ignotas]

On Fri, Jun 13, 2014 at 12:00 PM, Piotr Caban  wrote:
> Hi,
>
> On 06/13/14 01:27, Grazvydas Ignotas wrote:
> > +static void test_write_flush(void)
> > +{
> > - char iobuf[1024];
> > - char *tempf;
> > - FILE *file;
> > -
> > - tempf = _tempnam(".","wne");
> > - file = fopen(tempf, "wb+");
> > - ok(file != NULL, "unable to create test file\n");
> > - /* the default should be 4K */
> > - test_write_flush_size(file, 4096);
>
> It would be nice to read the size from the file->_bufsiz instead of
> hard-coding 4096. This value is not initialized while the file is opened so
> you will need to call something before reading it's value.

One of the goals of this test was to test if the default buffer size
is really 4K, after reading file->_bufsiz that will no longer be
tested. Also no test currently is accessing FILE members, so I'd
prefer to keep it as-is.
> > - setvbuf(file, iobuf, _IOFBF, sizeof(iobuf));
> > - test_write_flush_size(file, sizeof(iobuf));
> > - fclose(file);
>
> Current implemententation of setvbuf is broken in wine. This code will cause
> free(iobuf) to be called. I think that setvbuf needs to be fixed before this
> test can be added.

ok
> > - unlink(tempf);
> > - free(tempf);
>
> _tempnam returns internal buffer, free should not be called on it.

Are you sure? From what I see wine does MSVCRT__strdup(tmpbuf), unix
manpage also says it should be freed, many tests are already freeing
it.

[1/4] msvcrt/tests: Add a test for fwrite flushing behavior. (try 2)

[Piotr Caban]

On 06/13/14 18:11, Grazvydas Ignotas wrote:
> On Fri, Jun 13, 2014 at 12:00 PM, Piotr Caban  wrote:
> > Hi,
> >
> > On 06/13/14 01:27, Grazvydas Ignotas wrote:
> > > +static void test_write_flush(void)
> > > +{
> > > - char iobuf[1024];
> > > - char *tempf;
> > > - FILE *file;
> > > -
> > > - tempf = _tempnam(".","wne");
> > > - file = fopen(tempf, "wb+");
> > > - ok(file != NULL, "unable to create test file\n");
> > > - /* the default should be 4K */
> > > - test_write_flush_size(file, 4096);
> >
> > It would be nice to read the size from the file->_bufsiz instead of
> > hard-coding 4096. This value is not initialized while the file is opened so
> > you will need to call something before reading it's value.
>
> One of the goals of this test was to test if the default buffer size
> is really 4K, after reading file->_bufsiz that will no longer be
> tested. Also no test currently is accessing FILE members, so I'd
> prefer to keep it as-is.

I don't see any problem with accessing FILE members in tests. If you 
want to make sure it's set to 4096 just add following test:
ok(file->_bufsiz == 4096, ...);

You are generally testing behavior that depends on _bufsiz so I think 
it's cleaner to pass it there.
> > > - unlink(tempf);
> > > - free(tempf);
> >
> > _tempnam returns internal buffer, free should not be called on it.
>
> Are you sure? From what I see wine does MSVCRT__strdup(tmpbuf), unix
> manpage also says it should be freed, many tests are already freeing
> it.

You're right, this buffer needs to be freed, sorry.