| Message ID | 166930551818.827792.10663674346122681963.stgit@bahia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: Fix some return statements in the synth backend | expand |
On Thursday, November 24, 2022 4:58:38 PM CET Greg Kurz wrote: > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions > currently return a positive errno value on failure. This causes > checkpatch.pl to spit several errors like the one below: > > ERROR: return of an errno should typically be -ve (return -EAGAIN) > #79: FILE: hw/9pfs/9p-synth.c:79: > + return EAGAIN; > > Simply change the sign. This has no consequence since callers > assert() the returned value to be equal to 0. > > While here also get rid of the uneeded ret variables as suggested > by return_directly.cocci. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-synth.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next I would have expected more locations like that. Thanks! Best regards, Christian Schoenebeck
Greg Kurz <groug@kaod.org> writes: > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions > currently return a positive errno value on failure. This causes > checkpatch.pl to spit several errors like the one below: > > ERROR: return of an errno should typically be -ve (return -EAGAIN) > #79: FILE: hw/9pfs/9p-synth.c:79: > + return EAGAIN; > > Simply change the sign. This has no consequence since callers > assert() the returned value to be equal to 0. Out of curiosity: why is assert() appropriate? > While here also get rid of the uneeded ret variables as suggested > by return_directly.cocci. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Signed-off-by: Markus Armbruster <armbru@redhat.com>
On Mon, 28 Nov 2022 08:35:22 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Greg Kurz <groug@kaod.org> writes: > > > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions > > currently return a positive errno value on failure. This causes > > checkpatch.pl to spit several errors like the one below: > > > > ERROR: return of an errno should typically be -ve (return -EAGAIN) > > #79: FILE: hw/9pfs/9p-synth.c:79: > > + return EAGAIN; > > > > Simply change the sign. This has no consequence since callers > > assert() the returned value to be equal to 0. > > Out of curiosity: why is assert() appropriate? > Most of the code base comes from the original synth backend which was designed to expose QEMU internals to the guest using 9p. The hope of the virtio-9p authors was that each QEMU subsystem would create its own tree using these two functions (note that they are declared extern). Of course these never happened and the synth backend remained nearly dead code for years, until finally it got re-used to implement 9p qtest. In this context, failure to create a synthetic directory or file means the related test has a bug (e.g. messing with the paths used by some other test). This code likely needs improvements but we never got to it. > > While here also get rid of the uneeded ret variables as suggested > > by return_directly.cocci. > > > > Reported-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> >
Greg Kurz <groug@kaod.org> writes: > On Mon, 28 Nov 2022 08:35:22 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Greg Kurz <groug@kaod.org> writes: >> >> > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions >> > currently return a positive errno value on failure. This causes >> > checkpatch.pl to spit several errors like the one below: >> > >> > ERROR: return of an errno should typically be -ve (return -EAGAIN) >> > #79: FILE: hw/9pfs/9p-synth.c:79: >> > + return EAGAIN; >> > >> > Simply change the sign. This has no consequence since callers >> > assert() the returned value to be equal to 0. >> >> Out of curiosity: why is assert() appropriate? >> > > Most of the code base comes from the original synth backend which > was designed to expose QEMU internals to the guest using 9p. The > hope of the virtio-9p authors was that each QEMU subsystem would > create its own tree using these two functions (note that they > are declared extern). Of course these never happened and the synth > backend remained nearly dead code for years, until finally it got > re-used to implement 9p qtest. In this context, failure to create a > synthetic directory or file means the related test has a bug (e.g. > messing with the paths used by some other test). This code likely > needs improvements but we never got to it. I was about to suggest putting this in a file comment, but then I saw /* * Not so fast! You might want to read the 9p developer docs first: * https://wiki.qemu.org/Documentation/9p */ and behind the link, there's a paragraph "3. synth fs driver". Perhaps a brief note on the use of assert() in synth_init() would still make sense. Up to you. Thanks! [...]
On Monday, November 28, 2022 11:18:48 AM CET Markus Armbruster wrote: > Greg Kurz <groug@kaod.org> writes: > > > On Mon, 28 Nov 2022 08:35:22 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Greg Kurz <groug@kaod.org> writes: > >> > >> > The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions > >> > currently return a positive errno value on failure. This causes > >> > checkpatch.pl to spit several errors like the one below: > >> > > >> > ERROR: return of an errno should typically be -ve (return -EAGAIN) > >> > #79: FILE: hw/9pfs/9p-synth.c:79: > >> > + return EAGAIN; > >> > > >> > Simply change the sign. This has no consequence since callers > >> > assert() the returned value to be equal to 0. > >> > >> Out of curiosity: why is assert() appropriate? > >> > > > > Most of the code base comes from the original synth backend which > > was designed to expose QEMU internals to the guest using 9p. The > > hope of the virtio-9p authors was that each QEMU subsystem would > > create its own tree using these two functions (note that they > > are declared extern). Of course these never happened and the synth > > backend remained nearly dead code for years, until finally it got > > re-used to implement 9p qtest. In this context, failure to create a > > synthetic directory or file means the related test has a bug (e.g. > > messing with the paths used by some other test). This code likely > > needs improvements but we never got to it. > > I was about to suggest putting this in a file comment, but then I saw > > /* > * Not so fast! You might want to read the 9p developer docs first: > * https://wiki.qemu.org/Documentation/9p > */ > > and behind the link, there's a paragraph "3. synth fs driver". > > Perhaps a brief note on the use of assert() in synth_init() would still > make sense. Up to you. Like what comment would you expect there? The synth driver is a simplified hack fs driver with hard coded directories & files, only used for 9p protocol conformance test cases. Best regards, Christian Schoenebeck
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c index 1c5813e4ddc6..f62c40b639fc 100644 --- a/hw/9pfs/9p-synth.c +++ b/hw/9pfs/9p-synth.c @@ -72,14 +72,13 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode *parent, int mode, int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode, const char *name, V9fsSynthNode **result) { - int ret; V9fsSynthNode *node, *tmp; if (!synth_fs) { - return EAGAIN; + return -EAGAIN; } if (!name || (strlen(name) >= NAME_MAX)) { - return EINVAL; + return -EINVAL; } if (!parent) { parent = &synth_root; @@ -87,8 +86,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode, QEMU_LOCK_GUARD(&synth_mutex); QLIST_FOREACH(tmp, &parent->child, sibling) { if (!strcmp(tmp->name, name)) { - ret = EEXIST; - return ret; + return -EEXIST; } } /* Add the name */ @@ -98,22 +96,20 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode, v9fs_add_dir_node(node, node->attr->mode, ".", node->attr, node->attr->inode); *result = node; - ret = 0; - return ret; + return 0; } int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode, const char *name, v9fs_synth_read read, v9fs_synth_write write, void *arg) { - int ret; V9fsSynthNode *node, *tmp; if (!synth_fs) { - return EAGAIN; + return -EAGAIN; } if (!name || (strlen(name) >= NAME_MAX)) { - return EINVAL; + return -EINVAL; } if (!parent) { parent = &synth_root; @@ -122,8 +118,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode, QEMU_LOCK_GUARD(&synth_mutex); QLIST_FOREACH(tmp, &parent->child, sibling) { if (!strcmp(tmp->name, name)) { - ret = EEXIST; - return ret; + return -EEXIST; } } /* Add file type and remove write bits */ @@ -138,8 +133,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode, node->private = arg; pstrcpy(node->name, sizeof(node->name), name); QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling); - ret = 0; - return ret; + return 0; } static void synth_fill_statbuf(V9fsSynthNode *node, struct stat *stbuf)
The qemu_v9fs_synth_mkdir() and qemu_v9fs_synth_add_file() functions currently return a positive errno value on failure. This causes checkpatch.pl to spit several errors like the one below: ERROR: return of an errno should typically be -ve (return -EAGAIN) #79: FILE: hw/9pfs/9p-synth.c:79: + return EAGAIN; Simply change the sign. This has no consequence since callers assert() the returned value to be equal to 0. While here also get rid of the uneeded ret variables as suggested by return_directly.cocci. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-synth.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)