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

[Sumit Garg]

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