| Message ID | 03b427019be98d12761924f1e6132eefc82c80ec.1603149434.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: more local tests | expand |
On Tue, 20 Oct 2020 01:13:23 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > Split out walking a directory path to a separate new utility function > fs_walk_fid() and use that function in fs_mkdir(). > > The code difference saved this way is not much, but we'll use that new > fs_walk_fid() function in the upcoming patches, so it will avoid quite > some code duplication after all. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > index c15908f27b..dc724bbb1e 100644 > --- a/tests/qtest/virtio-9p-test.c > +++ b/tests/qtest/virtio-9p-test.c > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc) > g_free(wnames[0]); > } > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > - const char *path, const char *cname) > +/* utility function: walk to requested dir and return fid for that dir */ > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator *t_alloc, > + const char *path) > { Since fs_walk_fid() is a helper function, ie. not passed to qos_add_test(), any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc" based signature ? data and t_alloc aren't used at all and it seems that the function should rather take a QVirtio9P * directly instead of casting from a void *. Something like: static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path) { ... } Same remark applies to fs_mkdir() which isn't a top level test function either BTW (sorry for not having spotted this earlier). > QVirtio9P *v9p = obj; > - alloc = t_alloc; > char **wnames; > - char *const name = g_strdup(cname); > P9Req *req; > const uint32_t fid = genfid(); > > @@ -983,12 +982,26 @@ static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > v9fs_req_wait_for_reply(req, NULL); > v9fs_rwalk(req, NULL, NULL); > > + split_free(&wnames); > + return fid; > +} > + > +static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > + const char *path, const char *cname) > +{ > + QVirtio9P *v9p = obj; > + alloc = t_alloc; > + char *const name = g_strdup(cname); > + uint32_t fid; > + P9Req *req; > + > + fid = fs_walk_fid(v9p, data, t_alloc, path); > + > req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0); > v9fs_req_wait_for_reply(req, NULL); > v9fs_rmkdir(req, NULL); > > g_free(name); > - split_free(&wnames); > } > > static void fs_readdir_split_128(void *obj, void *data,
On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote: > On Tue, 20 Oct 2020 01:13:23 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > Split out walking a directory path to a separate new utility function > > fs_walk_fid() and use that function in fs_mkdir(). > > > > The code difference saved this way is not much, but we'll use that new > > fs_walk_fid() function in the upcoming patches, so it will avoid quite > > some code duplication after all. > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > > > tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > > index c15908f27b..dc724bbb1e 100644 > > --- a/tests/qtest/virtio-9p-test.c > > +++ b/tests/qtest/virtio-9p-test.c > > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, > > QGuestAllocator *t_alloc)> > > g_free(wnames[0]); > > > > } > > > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > > - const char *path, const char *cname) > > +/* utility function: walk to requested dir and return fid for that dir */ > > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator > > *t_alloc, + const char *path) > > > > { > > Since fs_walk_fid() is a helper function, ie. not passed to qos_add_test(), > any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc" > based signature ? data and t_alloc aren't used at all and it seems that the > function should rather take a QVirtio9P * directly instead of casting from > a void *. > > Something like: > > static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path) > { > ... > } > > > Same remark applies to fs_mkdir() which isn't a top level test function > either BTW (sorry for not having spotted this earlier). Good point. Typical case of being copy & waste induced. I'll change that. Best regards, Christian Schoenebeck
On Tue, 20 Oct 2020 15:43:21 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote: > > On Tue, 20 Oct 2020 01:13:23 +0200 > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > Split out walking a directory path to a separate new utility function > > > fs_walk_fid() and use that function in fs_mkdir(). > > > > > > The code difference saved this way is not much, but we'll use that new > > > fs_walk_fid() function in the upcoming patches, so it will avoid quite > > > some code duplication after all. > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > > > > tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > > > index c15908f27b..dc724bbb1e 100644 > > > --- a/tests/qtest/virtio-9p-test.c > > > +++ b/tests/qtest/virtio-9p-test.c > > > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, > > > QGuestAllocator *t_alloc)> > > > g_free(wnames[0]); > > > > > > } > > > > > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > > > - const char *path, const char *cname) > > > +/* utility function: walk to requested dir and return fid for that dir */ > > > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator > > > *t_alloc, + const char *path) > > > > > > { > > > > Since fs_walk_fid() is a helper function, ie. not passed to qos_add_test(), > > any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc" > > based signature ? data and t_alloc aren't used at all and it seems that the > > function should rather take a QVirtio9P * directly instead of casting from > > a void *. > > > > Something like: > > > > static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path) > > { > > ... > > } > > > > > > Same remark applies to fs_mkdir() which isn't a top level test function > > either BTW (sorry for not having spotted this earlier). > > Good point. Typical case of being copy & waste induced. I'll change that. > Well I guess I haven't set a good exemple by calling fs_attach() everywhere instead of factoring out an helper in the first place... I have some spare time, I'll fix that. > Best regards, > Christian Schoenebeck > >
On Tue, 20 Oct 2020 15:43:21 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote: > > On Tue, 20 Oct 2020 01:13:23 +0200 > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > Split out walking a directory path to a separate new utility function > > > fs_walk_fid() and use that function in fs_mkdir(). > > > > > > The code difference saved this way is not much, but we'll use that new > > > fs_walk_fid() function in the upcoming patches, so it will avoid quite > > > some code duplication after all. > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > > > > tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > > > index c15908f27b..dc724bbb1e 100644 > > > --- a/tests/qtest/virtio-9p-test.c > > > +++ b/tests/qtest/virtio-9p-test.c > > > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, > > > QGuestAllocator *t_alloc)> > > > g_free(wnames[0]); > > > > > > } > > > > > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > > > - const char *path, const char *cname) > > > +/* utility function: walk to requested dir and return fid for that dir */ > > > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator > > > *t_alloc, + const char *path) > > > > > > { > > > > Since fs_walk_fid() is a helper function, ie. not passed to qos_add_test(), > > any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc" > > based signature ? data and t_alloc aren't used at all and it seems that the > > function should rather take a QVirtio9P * directly instead of casting from > > a void *. > > > > Something like: > > > > static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path) > > { > > ... > > } > > > > > > Same remark applies to fs_mkdir() which isn't a top level test function > > either BTW (sorry for not having spotted this earlier). > > Good point. Typical case of being copy & waste induced. I'll change that. > Since this also affects other patches in this series and this might have a substantial impact, I'll wait for v2 to review if you don't mind. > Best regards, > Christian Schoenebeck > >
On Dienstag, 20. Oktober 2020 20:03:09 CEST Greg Kurz wrote: > On Tue, 20 Oct 2020 15:43:21 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote: > > > On Tue, 20 Oct 2020 01:13:23 +0200 > > > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > > Split out walking a directory path to a separate new utility function > > > > fs_walk_fid() and use that function in fs_mkdir(). > > > > > > > > The code difference saved this way is not much, but we'll use that new > > > > fs_walk_fid() function in the upcoming patches, so it will avoid quite > > > > some code duplication after all. > > > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > > --- > > > > > > > > tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- > > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/tests/qtest/virtio-9p-test.c > > > > b/tests/qtest/virtio-9p-test.c > > > > index c15908f27b..dc724bbb1e 100644 > > > > --- a/tests/qtest/virtio-9p-test.c > > > > +++ b/tests/qtest/virtio-9p-test.c > > > > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void > > > > *data, > > > > QGuestAllocator *t_alloc)> > > > > > > > > g_free(wnames[0]); > > > > > > > > } > > > > > > > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, > > > > - const char *path, const char *cname) > > > > +/* utility function: walk to requested dir and return fid for that > > > > dir */ > > > > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator > > > > *t_alloc, + const char *path) > > > > > > > > { > > > > > > Since fs_walk_fid() is a helper function, ie. not passed to > > > qos_add_test(), > > > any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc" > > > based signature ? data and t_alloc aren't used at all and it seems that > > > the > > > function should rather take a QVirtio9P * directly instead of casting > > > from > > > a void *. > > > > > > Something like: > > > > > > static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path) > > > { > > > ... > > > } > > > > > > > > > Same remark applies to fs_mkdir() which isn't a top level test function > > > either BTW (sorry for not having spotted this earlier). > > > > Good point. Typical case of being copy & waste induced. I'll change that. > > Since this also affects other patches in this series and this might > have a substantial impact, I'll wait for v2 to review if you don't > mind. Sure! There is probably no need to hurry anyway; since these are just test case changes, I think it will also be fine if I send the PR during freeze. You never had any issue with 9p hard links, right? I'm still investigating this in parallel. I already can rule out QEMU's sandbox (seccomp) feature, even after having disabled the latter that mentioned box is failing any link()/linkat() calls. Maybe it's a SELinux issue ... Best regards, Christian Schoenebeck
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index c15908f27b..dc724bbb1e 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc) g_free(wnames[0]); } -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, - const char *path, const char *cname) +/* utility function: walk to requested dir and return fid for that dir */ +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator *t_alloc, + const char *path) { QVirtio9P *v9p = obj; - alloc = t_alloc; char **wnames; - char *const name = g_strdup(cname); P9Req *req; const uint32_t fid = genfid(); @@ -983,12 +982,26 @@ static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, v9fs_req_wait_for_reply(req, NULL); v9fs_rwalk(req, NULL, NULL); + split_free(&wnames); + return fid; +} + +static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc, + const char *path, const char *cname) +{ + QVirtio9P *v9p = obj; + alloc = t_alloc; + char *const name = g_strdup(cname); + uint32_t fid; + P9Req *req; + + fid = fs_walk_fid(v9p, data, t_alloc, path); + req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0); v9fs_req_wait_for_reply(req, NULL); v9fs_rmkdir(req, NULL); g_free(name); - split_free(&wnames); } static void fs_readdir_split_128(void *obj, void *data,
Split out walking a directory path to a separate new utility function fs_walk_fid() and use that function in fs_mkdir(). The code difference saved this way is not much, but we'll use that new fs_walk_fid() function in the upcoming patches, so it will avoid quite some code duplication after all. Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- tests/qtest/virtio-9p-test.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)