[v3,1/7] lib: Add library functions for sync related syscalls

These library functions are used to add common test for minimal sectors
written on device for various sync related syscalls.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 include/tst_sync_device.h | 17 ++++++++++
 lib/tst_sync_device.c     | 80 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100644 include/tst_sync_device.h
 create mode 100644 lib/tst_sync_device.c

Hi!
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  include/tst_sync_device.h | 17 ++++++++++
>  lib/tst_sync_device.c     | 80 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>  create mode 100644 include/tst_sync_device.h
>  create mode 100644 lib/tst_sync_device.c
> 
> diff --git a/include/tst_sync_device.h b/include/tst_sync_device.h
> new file mode 100644
> index 0000000..b07c490
> --- /dev/null
> +++ b/include/tst_sync_device.h
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> + * Author: Sumit Garg <sumit.garg@linaro.org>
> + */
> +
> +#ifndef TST_SYNC_DEVICE_H__
> +#define TST_SYNC_DEVICE_H__
> +
> +#include <stdbool.h>
> +
> +void tst_sync_device_init(const char *dev);
> +int tst_sync_device_write(const char *file, unsigned int size_mb);
> +bool tst_sync_device_check(unsigned int size_mb);
> +void tst_sync_device_cleanup(void);
> +
> +#endif
> diff --git a/lib/tst_sync_device.c b/lib/tst_sync_device.c
> new file mode 100644
> index 0000000..5a0a17c
> --- /dev/null
> +++ b/lib/tst_sync_device.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> + * Author: Sumit Garg <sumit.garg@linaro.org>
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/types.h>
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "lapi/stat.h"
> +#include "tst_sync_device.h"
> +
> +#define SIZE_MB			(1024*1024)
> +#define MODE			0644
> +
> +static char dev_stat_path[1024];
> +static char *buffer;
> +static int fd;
> +static unsigned long prev_write_sec;
> +
> +void tst_sync_device_init(const char *dev)
> +{
> +	struct stat st;
> +
> +	snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat",
> +		 strrchr(dev, '/') + 1);
> +
> +	if (stat(dev_stat_path, &st) != 0)
> +		tst_brk(TCONF, "Test device stat file: %s not found",
> +			dev_stat_path);
> +
> +	buffer = SAFE_MALLOC(SIZE_MB);
> +
> +	memset(buffer, 0, SIZE_MB);
> +}
> +
> +int tst_sync_device_write(const char *file, unsigned int size_mb)
> +{
> +	unsigned int counter;
                     ^
		     It's kind of strange to name this variable anything
		     else but i.

> +	SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> +			&prev_write_sec);
> +
> +	fd =  SAFE_OPEN(file, O_RDWR|O_CREAT, MODE);
            ^
	    Extra space.

> +	/* Filling the test file */
         ^
	 Useless comment, it's pretty clear what we do here.

> +	for (counter = 0; counter < size_mb; counter++)
> +		SAFE_WRITE(1, fd, buffer, SIZE_MB);
> +
> +	return fd;
> +}
> +
> +bool tst_sync_device_check(unsigned int size_mb)
> +{
> +	unsigned long write_sec = 0;
> +	bool res = false;
> +
> +	SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> +			&write_sec);
> +
> +	if ((write_sec - prev_write_sec) * 512 >=
> +	    (size_mb * SIZE_MB))
> +		res = true;
> +
> +	SAFE_CLOSE(fd);
> +
> +	return res;
> +}

I do not like that this API is tailored just to this specific usecase.

I would rather see the code that writes the file separated from the code
that measures the bytes written.

Maybe we just need a tst_dev_bytes_written() function that would return
the bytes written since the last call of the function so that we could
do:

	fd = SAFE_OPEN(...);

	tst_dev_blocks_written(tst_device.dev);

	tst_fill_fd(fd, 0, TST_MB, size_mb);

	TEST(fdsync(fd));

	if (TST_RET)
		tst_brk(TFAIL | TTERRNO, "syncfd() failed with %i", TST_RET);

	written = tst_dev_blocks_written(tst_device.dev);

	SAFE_CLOSE(fd);

	if (written >= size_mb * TST_DEV_BLOCKS_IN_MB)
		tst_res(TPASS, ...);
	else
		tst_res(TFAIL, ...);


The test a bit longer, but the library functions are more reusable, if
you do 'git grep -B 1 SAFE_WRITE' you can see that the tst_fill_fd
function that loops over SAFE_WRITE could be used in a few places
already.

> +void tst_sync_device_cleanup(void)
> +{
> +	if (buffer)
> +		free(buffer);
> +
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +}
> -- 
> 2.7.4
>

On Tue, 19 Feb 2019 at 19:36, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  include/tst_sync_device.h | 17 ++++++++++
> >  lib/tst_sync_device.c     | 80 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 97 insertions(+)
> >  create mode 100644 include/tst_sync_device.h
> >  create mode 100644 lib/tst_sync_device.c
> >
> > diff --git a/include/tst_sync_device.h b/include/tst_sync_device.h
> > new file mode 100644
> > index 0000000..b07c490
> > --- /dev/null
> > +++ b/include/tst_sync_device.h
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> > + * Author: Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#ifndef TST_SYNC_DEVICE_H__
> > +#define TST_SYNC_DEVICE_H__
> > +
> > +#include <stdbool.h>
> > +
> > +void tst_sync_device_init(const char *dev);
> > +int tst_sync_device_write(const char *file, unsigned int size_mb);
> > +bool tst_sync_device_check(unsigned int size_mb);
> > +void tst_sync_device_cleanup(void);
> > +
> > +#endif
> > diff --git a/lib/tst_sync_device.c b/lib/tst_sync_device.c
> > new file mode 100644
> > index 0000000..5a0a17c
> > --- /dev/null
> > +++ b/lib/tst_sync_device.c
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2019 Linaro Limited. All rights reserved.
> > + * Author: Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <sys/types.h>
> > +
> > +#define TST_NO_DEFAULT_MAIN
> > +#include "tst_test.h"
> > +#include "lapi/stat.h"
> > +#include "tst_sync_device.h"
> > +
> > +#define SIZE_MB                      (1024*1024)
> > +#define MODE                 0644
> > +
> > +static char dev_stat_path[1024];
> > +static char *buffer;
> > +static int fd;
> > +static unsigned long prev_write_sec;
> > +
> > +void tst_sync_device_init(const char *dev)
> > +{
> > +     struct stat st;
> > +
> > +     snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat",
> > +              strrchr(dev, '/') + 1);
> > +
> > +     if (stat(dev_stat_path, &st) != 0)
> > +             tst_brk(TCONF, "Test device stat file: %s not found",
> > +                     dev_stat_path);
> > +
> > +     buffer = SAFE_MALLOC(SIZE_MB);
> > +
> > +     memset(buffer, 0, SIZE_MB);
> > +}
> > +
> > +int tst_sync_device_write(const char *file, unsigned int size_mb)
> > +{
> > +     unsigned int counter;
>                      ^
>                      It's kind of strange to name this variable anything
>                      else but i.
>

Will use i instead.

> > +     SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> > +                     &prev_write_sec);
> > +
> > +     fd =  SAFE_OPEN(file, O_RDWR|O_CREAT, MODE);
>             ^
>             Extra space.
>

Will fix.

> > +     /* Filling the test file */
>          ^
>          Useless comment, it's pretty clear what we do here.
>

Will remove this comment.


> > +     for (counter = 0; counter < size_mb; counter++)
> > +             SAFE_WRITE(1, fd, buffer, SIZE_MB);
> > +
> > +     return fd;
> > +}
> > +
> > +bool tst_sync_device_check(unsigned int size_mb)
> > +{
> > +     unsigned long write_sec = 0;
> > +     bool res = false;
> > +
> > +     SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu",
> > +                     &write_sec);
> > +
> > +     if ((write_sec - prev_write_sec) * 512 >=
> > +         (size_mb * SIZE_MB))
> > +             res = true;
> > +
> > +     SAFE_CLOSE(fd);
> > +
> > +     return res;
> > +}
>
> I do not like that this API is tailored just to this specific usecase.
>
> I would rather see the code that writes the file separated from the code
> that measures the bytes written.
>

Agree. Actually my initial idea was to tailor it for maximal code re-use.

> Maybe we just need a tst_dev_bytes_written() function that would return
> the bytes written since the last call of the function so that we could
> do:
>
>         fd = SAFE_OPEN(...);
>
>         tst_dev_blocks_written(tst_device.dev);
>
>         tst_fill_fd(fd, 0, TST_MB, size_mb);
>
>         TEST(fdsync(fd));
>
>         if (TST_RET)
>                 tst_brk(TFAIL | TTERRNO, "syncfd() failed with %i", TST_RET);
>
>         written = tst_dev_blocks_written(tst_device.dev);
>
>         SAFE_CLOSE(fd);
>
>         if (written >= size_mb * TST_DEV_BLOCKS_IN_MB)
>                 tst_res(TPASS, ...);
>         else
>                 tst_res(TFAIL, ...);
>

Seems to be nicer approach. So rather than new file should I add
tst_dev_bytes_written() function in "lib/tst_device.c" file?

>
> The test a bit longer, but the library functions are more reusable, if
> you do 'git grep -B 1 SAFE_WRITE' you can see that the tst_fill_fd
> function that loops over SAFE_WRITE could be used in a few places
> already.
>

I am not able to locate tst_fill_fd function. Are you referring to
tst_fill_file function? If yes, then we could split that function to
create tst_fill_fd function.

-Sumit

> > +void tst_sync_device_cleanup(void)
> > +{
> > +     if (buffer)
> > +             free(buffer);
> > +
> > +     if (fd > 0)
> > +             SAFE_CLOSE(fd);
> > +}
> > --
> > 2.7.4
> >
>
> --
> Cyril Hrubis
> chrubis@suse.cz

Hi!
> > Maybe we just need a tst_dev_bytes_written() function that would return
> > the bytes written since the last call of the function so that we could
> > do:
> >
> >         fd = SAFE_OPEN(...);
> >
> >         tst_dev_blocks_written(tst_device.dev);
> >
> >         tst_fill_fd(fd, 0, TST_MB, size_mb);
> >
> >         TEST(fdsync(fd));
> >
> >         if (TST_RET)
> >                 tst_brk(TFAIL | TTERRNO, "syncfd() failed with %i", TST_RET);
> >
> >         written = tst_dev_blocks_written(tst_device.dev);
> >
> >         SAFE_CLOSE(fd);
> >
> >         if (written >= size_mb * TST_DEV_BLOCKS_IN_MB)
> >                 tst_res(TPASS, ...);
> >         else
> >                 tst_res(TFAIL, ...);
> >
> 
> Seems to be nicer approach. So rather than new file should I add
> tst_dev_bytes_written() function in "lib/tst_device.c" file?

Sounds good to me.

> > The test a bit longer, but the library functions are more reusable, if
> > you do 'git grep -B 1 SAFE_WRITE' you can see that the tst_fill_fd
> > function that loops over SAFE_WRITE could be used in a few places
> > already.
> >
> 
> I am not able to locate tst_fill_fd function. Are you referring to
> tst_fill_file function? If yes, then we could split that function to
> create tst_fill_fd function.

There is none, I was trying to state that there are several places that
use the same pattern and that such function would be generally useful.

And yes, buiding tst_fill_file on the top of tst_fill_fd sounds like a
good approach.

On Wed, 20 Feb 2019 at 17:34, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > Maybe we just need a tst_dev_bytes_written() function that would return
> > > the bytes written since the last call of the function so that we could
> > > do:
> > >
> > >         fd = SAFE_OPEN(...);
> > >
> > >         tst_dev_blocks_written(tst_device.dev);
> > >
> > >         tst_fill_fd(fd, 0, TST_MB, size_mb);
> > >
> > >         TEST(fdsync(fd));
> > >
> > >         if (TST_RET)
> > >                 tst_brk(TFAIL | TTERRNO, "syncfd() failed with %i", TST_RET);
> > >
> > >         written = tst_dev_blocks_written(tst_device.dev);
> > >
> > >         SAFE_CLOSE(fd);
> > >
> > >         if (written >= size_mb * TST_DEV_BLOCKS_IN_MB)
> > >                 tst_res(TPASS, ...);
> > >         else
> > >                 tst_res(TFAIL, ...);
> > >
> >
> > Seems to be nicer approach. So rather than new file should I add
> > tst_dev_bytes_written() function in "lib/tst_device.c" file?
>
> Sounds good to me.
>
> > > The test a bit longer, but the library functions are more reusable, if
> > > you do 'git grep -B 1 SAFE_WRITE' you can see that the tst_fill_fd
> > > function that loops over SAFE_WRITE could be used in a few places
> > > already.
> > >
> >
> > I am not able to locate tst_fill_fd function. Are you referring to
> > tst_fill_file function? If yes, then we could split that function to
> > create tst_fill_fd function.
>
> There is none, I was trying to state that there are several places that
> use the same pattern and that such function would be generally useful.
>

Ah, ok.

> And yes, buiding tst_fill_file on the top of tst_fill_fd sounds like a
> good approach.
>

Will incorporate in v4.

-Sumit

> --
> Cyril Hrubis
> chrubis@suse.cz