| Message ID | 160320666564.255209.11628044710614310582.stgit@bahia.lan |
|---|---|
| State | Accepted |
| Commit | 1c450e6efe14a7c76f4e75d7316c9fdf00e757c0 |
| Headers | show |
| Series | tests/9pfs: Code refactoring | expand |
On Dienstag, 20. Oktober 2020 17:11:05 CEST Greg Kurz wrote: > fs_version() is a top level test function. Factor out the sugar > to a separate helper instead of hijacking it in other tests. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > tests/qtest/virtio-9p-test.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > index c15908f27b3d..63f91aaf77e6 100644 > --- a/tests/qtest/virtio-9p-test.c > +++ b/tests/qtest/virtio-9p-test.c > @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req) > v9fs_req_free(req); > } > > -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) > +static void do_fs_version(QVirtio9P *v9p) > { > - QVirtio9P *v9p = obj; > - alloc = t_alloc; > const char *version = "9P2000.L"; > uint16_t server_len; > char *server_version; > @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data, > QGuestAllocator *t_alloc) g_free(server_version); > } So the naming convention from now on shall be do_fs_*() for non-toplevel functions there. Not that I care too much about the precise prefix, but how about just do_*() for them instead? Except of that, your patches look fine to me. Best regards, Christian Schoenebeck
On Tue, 20 Oct 2020 17:34:05 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Dienstag, 20. Oktober 2020 17:11:05 CEST Greg Kurz wrote: > > fs_version() is a top level test function. Factor out the sugar > > to a separate helper instead of hijacking it in other tests. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > tests/qtest/virtio-9p-test.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > > index c15908f27b3d..63f91aaf77e6 100644 > > --- a/tests/qtest/virtio-9p-test.c > > +++ b/tests/qtest/virtio-9p-test.c > > @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req) > > v9fs_req_free(req); > > } > > > > -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) > > +static void do_fs_version(QVirtio9P *v9p) > > { > > - QVirtio9P *v9p = obj; > > - alloc = t_alloc; > > const char *version = "9P2000.L"; > > uint16_t server_len; > > char *server_version; > > @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data, > > QGuestAllocator *t_alloc) g_free(server_version); > > } > > So the naming convention from now on shall be do_fs_*() for non-toplevel > functions there. Not that I care too much about the precise prefix, but how > about just do_*() for them instead? > I've prepended "do_" to the existing names by pure laziness but I'm fine with any prefix or naming convention actually. So just tell me what you prefer and I'll send a v2. > Except of that, your patches look fine to me. > > Best regards, > Christian Schoenebeck > >
On Dienstag, 20. Oktober 2020 17:41:56 CEST Greg Kurz wrote: > On Tue, 20 Oct 2020 17:34:05 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Dienstag, 20. Oktober 2020 17:11:05 CEST Greg Kurz wrote: > > > fs_version() is a top level test function. Factor out the sugar > > > to a separate helper instead of hijacking it in other tests. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > > > > tests/qtest/virtio-9p-test.c | 14 +++++++++----- > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > > > index c15908f27b3d..63f91aaf77e6 100644 > > > --- a/tests/qtest/virtio-9p-test.c > > > +++ b/tests/qtest/virtio-9p-test.c > > > @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req) > > > > > > v9fs_req_free(req); > > > > > > } > > > > > > -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) > > > +static void do_fs_version(QVirtio9P *v9p) > > > > > > { > > > > > > - QVirtio9P *v9p = obj; > > > - alloc = t_alloc; > > > > > > const char *version = "9P2000.L"; > > > uint16_t server_len; > > > char *server_version; > > > > > > @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data, > > > QGuestAllocator *t_alloc) g_free(server_version); > > > > > > } > > > > So the naming convention from now on shall be do_fs_*() for non-toplevel > > functions there. Not that I care too much about the precise prefix, but > > how > > about just do_*() for them instead? > > I've prepended "do_" to the existing names by pure laziness but I'm > fine with any prefix or naming convention actually. > > So just tell me what you prefer and I'll send a v2. It's really just more pleasant for the eye to have the prefix a bit shorter. So use do_*() or any other kind of xx_*() or xxx_*() prefix that comes to your mind. It will be fine with me. Thanks Greg! Best regards, Christian Schoenebeck
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index c15908f27b3d..63f91aaf77e6 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req) v9fs_req_free(req); } -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) +static void do_fs_version(QVirtio9P *v9p) { - QVirtio9P *v9p = obj; - alloc = t_alloc; const char *version = "9P2000.L"; uint16_t server_len; char *server_version; @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) g_free(server_version); } +static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc) +{ + alloc = t_alloc; + do_fs_version(obj); +} + static void fs_attach(void *obj, void *data, QGuestAllocator *t_alloc) { QVirtio9P *v9p = obj; alloc = t_alloc; P9Req *req; - fs_version(v9p, NULL, t_alloc); + do_fs_version(v9p); req = v9fs_tattach(v9p, 0, getuid(), 0); v9fs_req_wait_for_reply(req, NULL); v9fs_rattach(req, NULL); @@ -831,7 +835,7 @@ static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc) v9fs_qid root_qid, *wqid; P9Req *req; - fs_version(v9p, NULL, t_alloc); + do_fs_version(v9p); req = v9fs_tattach(v9p, 0, getuid(), 0); v9fs_req_wait_for_reply(req, NULL); v9fs_rattach(req, &root_qid);
fs_version() is a top level test function. Factor out the sugar to a separate helper instead of hijacking it in other tests. Signed-off-by: Greg Kurz <groug@kaod.org> --- tests/qtest/virtio-9p-test.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)