| Message ID | 1563356676-2384-1-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v5,1/3] lib: alter find_free_loopdev() | expand |
On Wed, Jul 17, 2019 at 12:45 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote: > > Alter find_free_loopdev() to tst_find_free_loopdev(path, path_len), > it passes the dev_path inside of the tst_device.c and NULL from other > tests. It returns the free loopdev minor (and -1 for no free loopdev). > We can call tst_find_free_loopdev(NULL, 0) to get a free minor number > without changing lib internal state. > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > doc/test-writing-guidelines.txt | 12 ++++++++++++ > include/tst_device.h | 5 +++++ > lib/tst_device.c | 34 +++++++++++++++++---------------- > 3 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt > index 4b1e7d25b..c65c707e6 100644 > --- a/doc/test-writing-guidelines.txt > +++ b/doc/test-writing-guidelines.txt > @@ -1045,6 +1045,18 @@ IMPORTANT: All testcases should use 'tst_umount()' instead of 'umount(2)' to > ------------------------------------------------------------------------------- > #include "tst_test.h" > > +int tst_find_free_loopdev(const char *path, size_t path_len); > +------------------------------------------------------------------------------- > + > +This function finds a free loopdev and returns the free loopdev minor (-1 for no > +free loopdev). If path is non-NULL, it will be filled with free loopdev path. > +We can call tst_find_free_loopdev(NULL, 0) in tests to get a free minor number > +without changing lib internal state. > + > +[source,c] > +------------------------------------------------------------------------------- > +#include "tst_test.h" > + > unsigned long tst_dev_bytes_written(const char *dev); > ------------------------------------------------------------------------------- > > diff --git a/include/tst_device.h b/include/tst_device.h > index 61902b7e0..42b9fa95b 100644 > --- a/include/tst_device.h > +++ b/include/tst_device.h > @@ -44,6 +44,11 @@ int tst_umount(const char *path); > */ > int tst_clear_device(const char *dev); > > +/* > + * Finds a free loop device for use and returns the free loopdev minor(-1 for no > + * free loopdev). If path is non-NULL, it will be filled with free loopdev path. > + */ > +int tst_find_free_loopdev(const char *path, size_t path_len); > /* > * Reads test block device stat file and returns the bytes written since the > * last call of this function. > diff --git a/lib/tst_device.c b/lib/tst_device.c > index 65fcc1337..f2516fb08 100644 > --- a/lib/tst_device.c > +++ b/lib/tst_device.c > @@ -53,22 +53,22 @@ static const char *dev_variants[] = { > "/dev/block/loop%i" > }; > > -static int set_dev_path(int dev) > +static int set_dev_path(int dev, char *path, size_t path_len) > { > unsigned int i; > struct stat st; > > for (i = 0; i < ARRAY_SIZE(dev_variants); i++) { > - snprintf(dev_path, sizeof(dev_path), dev_variants[i], dev); > + snprintf(path, path_len, dev_variants[i], dev); > > - if (stat(dev_path, &st) == 0 && S_ISBLK(st.st_mode)) > + if (stat(path, &st) == 0 && S_ISBLK(st.st_mode)) > return 1; > } > > return 0; > } > > -static int find_free_loopdev(void) > +int tst_find_free_loopdev(char *path, size_t path_len) Another option is to leave this function internal and export a wrapper for the exact needed functionality: /* Just find a free minor number */ int tst_find_free_loopdev(void) { return find_free_loopdev(NULL, 0); } I don't mind wither way. > { > int ctl_fd, dev_fd, rc, i; > struct loop_info loopinfo; > @@ -80,12 +80,14 @@ static int find_free_loopdev(void) > rc = ioctl(ctl_fd, LOOP_CTL_GET_FREE); > close(ctl_fd); > if (rc >= 0) { > - set_dev_path(rc); > - tst_resm(TINFO, "Found free device '%s'", dev_path); > - return 0; > + if (path) > + set_dev_path(rc, path, path_len); > + tst_resm(TINFO, "Found free device %d '%s'", > + rc, path ?: ""); > + return rc; > } > tst_resm(TINFO, "Couldn't find free loop device"); > - return 1; > + return -1; > } > > switch (errno) { > @@ -104,24 +106,24 @@ static int find_free_loopdev(void) > * Older way is to iterate over /dev/loop%i and /dev/loop/%i and try > * LOOP_GET_STATUS ioctl() which fails for free loop devices. > */ > - for (i = 0; i < 256; i++) { > + for (i = 0; path && i < 256; i++) { > > - if (!set_dev_path(i)) > + if (!set_dev_path(i, path, path_len)) > continue; > > - dev_fd = open(dev_path, O_RDONLY); > + dev_fd = open(path, O_RDONLY); > > if (dev_fd < 0) > continue; > > if (ioctl(dev_fd, LOOP_GET_STATUS, &loopinfo) == 0) { > - tst_resm(TINFO, "Device '%s' in use", dev_path); > + tst_resm(TINFO, "Device '%s' in use", path); > } else { > if (errno != ENXIO) > continue; > - tst_resm(TINFO, "Found free device '%s'", dev_path); > + tst_resm(TINFO, "Found free device '%s'", path); > close(dev_fd); > - return 0; > + return i; > } > > close(dev_fd); > @@ -129,7 +131,7 @@ static int find_free_loopdev(void) > > tst_resm(TINFO, "No free devices found"); > > - return 1; > + return -1; > } > > static int attach_device(const char *dev, const char *file) > @@ -274,7 +276,7 @@ const char *tst_acquire_device__(unsigned int size) > return NULL; > } > > - if (find_free_loopdev()) > + if (tst_find_free_loopdev(dev_path, sizeof(dev_path)) == -1) > return NULL; > > if (attach_device(dev_path, DEV_FILE)) > -- > 2.18.1 > > >
> On Wed, Jul 17, 2019 at 12:45 PM Yang Xu<xuyang2018.jy@cn.fujitsu.com> wrote: >> Alter find_free_loopdev() to tst_find_free_loopdev(path, path_len), >> it passes the dev_path inside of the tst_device.c and NULL from other >> tests. It returns the free loopdev minor (and -1 for no free loopdev). >> We can call tst_find_free_loopdev(NULL, 0) to get a free minor number >> without changing lib internal state. >> >> Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com> >> Reviewed-by: Amir Goldstein<amir73il@gmail.com> >> --- >> doc/test-writing-guidelines.txt | 12 ++++++++++++ >> include/tst_device.h | 5 +++++ >> lib/tst_device.c | 34 +++++++++++++++++---------------- >> 3 files changed, 35 insertions(+), 16 deletions(-) >> >> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt >> index 4b1e7d25b..c65c707e6 100644 >> --- a/doc/test-writing-guidelines.txt >> +++ b/doc/test-writing-guidelines.txt >> @@ -1045,6 +1045,18 @@ IMPORTANT: All testcases should use 'tst_umount()' instead of 'umount(2)' to >> ------------------------------------------------------------------------------- >> #include "tst_test.h" >> >> +int tst_find_free_loopdev(const char *path, size_t path_len); >> +------------------------------------------------------------------------------- >> + >> +This function finds a free loopdev and returns the free loopdev minor (-1 for no >> +free loopdev). If path is non-NULL, it will be filled with free loopdev path. >> +We can call tst_find_free_loopdev(NULL, 0) in tests to get a free minor number >> +without changing lib internal state. >> + >> +[source,c] >> +------------------------------------------------------------------------------- >> +#include "tst_test.h" >> + >> unsigned long tst_dev_bytes_written(const char *dev); >> ------------------------------------------------------------------------------- >> >> diff --git a/include/tst_device.h b/include/tst_device.h >> index 61902b7e0..42b9fa95b 100644 >> --- a/include/tst_device.h >> +++ b/include/tst_device.h >> @@ -44,6 +44,11 @@ int tst_umount(const char *path); >> */ >> int tst_clear_device(const char *dev); >> >> +/* >> + * Finds a free loop device for use and returns the free loopdev minor(-1 for no >> + * free loopdev). If path is non-NULL, it will be filled with free loopdev path. >> + */ >> +int tst_find_free_loopdev(const char *path, size_t path_len); >> /* >> * Reads test block device stat file and returns the bytes written since the >> * last call of this function. >> diff --git a/lib/tst_device.c b/lib/tst_device.c >> index 65fcc1337..f2516fb08 100644 >> --- a/lib/tst_device.c >> +++ b/lib/tst_device.c >> @@ -53,22 +53,22 @@ static const char *dev_variants[] = { >> "/dev/block/loop%i" >> }; >> >> -static int set_dev_path(int dev) >> +static int set_dev_path(int dev, char *path, size_t path_len) >> { >> unsigned int i; >> struct stat st; >> >> for (i = 0; i< ARRAY_SIZE(dev_variants); i++) { >> - snprintf(dev_path, sizeof(dev_path), dev_variants[i], dev); >> + snprintf(path, path_len, dev_variants[i], dev); >> >> - if (stat(dev_path,&st) == 0&& S_ISBLK(st.st_mode)) >> + if (stat(path,&st) == 0&& S_ISBLK(st.st_mode)) >> return 1; >> } >> >> return 0; >> } >> >> -static int find_free_loopdev(void) >> +int tst_find_free_loopdev(char *path, size_t path_len) > Another option is to leave this function internal and export a wrapper > for the exact needed functionality: > > /* Just find a free minor number */ > int tst_find_free_loopdev(void) > { > return find_free_loopdev(NULL, 0); > } > > I don't mind wither way. Hi Amir The second way looks better for me. But I also want to hear the idea of cyril. >> { >> int ctl_fd, dev_fd, rc, i; >> struct loop_info loopinfo; >> @@ -80,12 +80,14 @@ static int find_free_loopdev(void) >> rc = ioctl(ctl_fd, LOOP_CTL_GET_FREE); >> close(ctl_fd); >> if (rc>= 0) { >> - set_dev_path(rc); >> - tst_resm(TINFO, "Found free device '%s'", dev_path); >> - return 0; >> + if (path) >> + set_dev_path(rc, path, path_len); >> + tst_resm(TINFO, "Found free device %d '%s'", >> + rc, path ?: ""); >> + return rc; >> } >> tst_resm(TINFO, "Couldn't find free loop device"); >> - return 1; >> + return -1; >> } >> >> switch (errno) { >> @@ -104,24 +106,24 @@ static int find_free_loopdev(void) >> * Older way is to iterate over /dev/loop%i and /dev/loop/%i and try >> * LOOP_GET_STATUS ioctl() which fails for free loop devices. >> */ >> - for (i = 0; i< 256; i++) { >> + for (i = 0; path&& i< 256; i++) { >> >> - if (!set_dev_path(i)) >> + if (!set_dev_path(i, path, path_len)) >> continue; >> >> - dev_fd = open(dev_path, O_RDONLY); >> + dev_fd = open(path, O_RDONLY); >> >> if (dev_fd< 0) >> continue; >> >> if (ioctl(dev_fd, LOOP_GET_STATUS,&loopinfo) == 0) { >> - tst_resm(TINFO, "Device '%s' in use", dev_path); >> + tst_resm(TINFO, "Device '%s' in use", path); >> } else { >> if (errno != ENXIO) >> continue; >> - tst_resm(TINFO, "Found free device '%s'", dev_path); >> + tst_resm(TINFO, "Found free device '%s'", path); >> close(dev_fd); >> - return 0; >> + return i; >> } >> >> close(dev_fd); >> @@ -129,7 +131,7 @@ static int find_free_loopdev(void) >> >> tst_resm(TINFO, "No free devices found"); >> >> - return 1; >> + return -1; >> } >> >> static int attach_device(const char *dev, const char *file) >> @@ -274,7 +276,7 @@ const char *tst_acquire_device__(unsigned int size) >> return NULL; >> } >> >> - if (find_free_loopdev()) >> + if (tst_find_free_loopdev(dev_path, sizeof(dev_path)) == -1) >> return NULL; >> >> if (attach_device(dev_path, DEV_FILE)) >> -- >> 2.18.1 >> >> >> > > . >
Hi! > > Another option is to leave this function internal and export a wrapper > > for the exact needed functionality: > > > > /* Just find a free minor number */ > > int tst_find_free_loopdev(void) > > { > > return find_free_loopdev(NULL, 0); > > } > > > > I don't mind wither way. > Hi Amir > > The second way looks better for me. But I also want to hear the idea of cyril. Well I wanted to keep the path exposed to user in a case that there would be a test that would need more than one device. But it's not like we cannot change the API later when it's actually needed.
> Hi! >>> Another option is to leave this function internal and export a wrapper >>> for the exact needed functionality: >>> >>> /* Just find a free minor number */ >>> int tst_find_free_loopdev(void) >>> { >>> return find_free_loopdev(NULL, 0); >>> } >>> >>> I don't mind wither way. >> Hi Amir >> >> The second way looks better for me. But I also want to hear the idea of cyril. > Well I wanted to keep the path exposed to user in a case that there > would be a test that would need more than one device. But it's not like > we cannot change the API later when it's actually needed. > Hi Cyril Ok. I will use the first way to expose the path. Also on my copy_file_range02, I will directly use path filled with this api because it doesn't change libary state. If you think this pathset is no problem, I will send v6 patch. Thanks Yang Xu
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index 4b1e7d25b..c65c707e6 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -1045,6 +1045,18 @@ IMPORTANT: All testcases should use 'tst_umount()' instead of 'umount(2)' to ------------------------------------------------------------------------------- #include "tst_test.h" +int tst_find_free_loopdev(const char *path, size_t path_len); +------------------------------------------------------------------------------- + +This function finds a free loopdev and returns the free loopdev minor (-1 for no +free loopdev). If path is non-NULL, it will be filled with free loopdev path. +We can call tst_find_free_loopdev(NULL, 0) in tests to get a free minor number +without changing lib internal state. + +[source,c] +------------------------------------------------------------------------------- +#include "tst_test.h" + unsigned long tst_dev_bytes_written(const char *dev); ------------------------------------------------------------------------------- diff --git a/include/tst_device.h b/include/tst_device.h index 61902b7e0..42b9fa95b 100644 --- a/include/tst_device.h +++ b/include/tst_device.h @@ -44,6 +44,11 @@ int tst_umount(const char *path); */ int tst_clear_device(const char *dev); +/* + * Finds a free loop device for use and returns the free loopdev minor(-1 for no + * free loopdev). If path is non-NULL, it will be filled with free loopdev path. + */ +int tst_find_free_loopdev(const char *path, size_t path_len); /* * Reads test block device stat file and returns the bytes written since the * last call of this function. diff --git a/lib/tst_device.c b/lib/tst_device.c index 65fcc1337..f2516fb08 100644 --- a/lib/tst_device.c +++ b/lib/tst_device.c @@ -53,22 +53,22 @@ static const char *dev_variants[] = { "/dev/block/loop%i" }; -static int set_dev_path(int dev) +static int set_dev_path(int dev, char *path, size_t path_len) { unsigned int i; struct stat st; for (i = 0; i < ARRAY_SIZE(dev_variants); i++) { - snprintf(dev_path, sizeof(dev_path), dev_variants[i], dev); + snprintf(path, path_len, dev_variants[i], dev); - if (stat(dev_path, &st) == 0 && S_ISBLK(st.st_mode)) + if (stat(path, &st) == 0 && S_ISBLK(st.st_mode)) return 1; } return 0; } -static int find_free_loopdev(void) +int tst_find_free_loopdev(char *path, size_t path_len) { int ctl_fd, dev_fd, rc, i; struct loop_info loopinfo; @@ -80,12 +80,14 @@ static int find_free_loopdev(void) rc = ioctl(ctl_fd, LOOP_CTL_GET_FREE); close(ctl_fd); if (rc >= 0) { - set_dev_path(rc); - tst_resm(TINFO, "Found free device '%s'", dev_path); - return 0; + if (path) + set_dev_path(rc, path, path_len); + tst_resm(TINFO, "Found free device %d '%s'", + rc, path ?: ""); + return rc; } tst_resm(TINFO, "Couldn't find free loop device"); - return 1; + return -1; } switch (errno) { @@ -104,24 +106,24 @@ static int find_free_loopdev(void) * Older way is to iterate over /dev/loop%i and /dev/loop/%i and try * LOOP_GET_STATUS ioctl() which fails for free loop devices. */ - for (i = 0; i < 256; i++) { + for (i = 0; path && i < 256; i++) { - if (!set_dev_path(i)) + if (!set_dev_path(i, path, path_len)) continue; - dev_fd = open(dev_path, O_RDONLY); + dev_fd = open(path, O_RDONLY); if (dev_fd < 0) continue; if (ioctl(dev_fd, LOOP_GET_STATUS, &loopinfo) == 0) { - tst_resm(TINFO, "Device '%s' in use", dev_path); + tst_resm(TINFO, "Device '%s' in use", path); } else { if (errno != ENXIO) continue; - tst_resm(TINFO, "Found free device '%s'", dev_path); + tst_resm(TINFO, "Found free device '%s'", path); close(dev_fd); - return 0; + return i; } close(dev_fd); @@ -129,7 +131,7 @@ static int find_free_loopdev(void) tst_resm(TINFO, "No free devices found"); - return 1; + return -1; } static int attach_device(const char *dev, const char *file) @@ -274,7 +276,7 @@ const char *tst_acquire_device__(unsigned int size) return NULL; } - if (find_free_loopdev()) + if (tst_find_free_loopdev(dev_path, sizeof(dev_path)) == -1) return NULL; if (attach_device(dev_path, DEV_FILE))