Message ID | 20181008173125.19678-29-armbru@redhat.com |
---|---|
State | New |
Headers | show |
Series | Replace some unwise uses of error_report() & friends | expand |
On 10/8/18 12:31 PM, Markus Armbruster wrote: > Calling error_report() from within a a function that takes an Error ** s/a a/a/ > argument is suspicious. qemu_fsdev_add() does that, and its caller > fsdev_init_func() then fails without setting an error. Its caller > main(), via qemu_opts_foreach(), is fine with it, but clean it up > anyway. >
On Mon, 8 Oct 2018 19:31:22 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Calling error_report() from within a a function that takes an Error ** > argument is suspicious. qemu_fsdev_add() does that, and its caller > fsdev_init_func() then fails without setting an error. Its caller > main(), via qemu_opts_foreach(), is fine with it, but clean it up > anyway. > > Cc: Greg Kurz <groug@kaod.org> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- Yeah, this was somewhere down in my TODO list. Thanks for the clean up Markus. Acked-by: Greg Kurz <groug@kaod.org> > fsdev/qemu-fsdev-dummy.c | 2 +- > fsdev/qemu-fsdev.c | 12 +++++------- > fsdev/qemu-fsdev.h | 2 +- > hw/9pfs/xen-9p-backend.c | 7 ++++++- > vl.c | 8 +++----- > 5 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c > index 6dc0fbc4c4..489cd29081 100644 > --- a/fsdev/qemu-fsdev-dummy.c > +++ b/fsdev/qemu-fsdev-dummy.c > @@ -15,7 +15,7 @@ > #include "qemu/config-file.h" > #include "qemu/module.h" > > -int qemu_fsdev_add(QemuOpts *opts) > +int qemu_fsdev_add(QemuOpts *opts, Error **errp) > { > return 0; > } > diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c > index 8a4afbffbd..7a3b87cc9e 100644 > --- a/fsdev/qemu-fsdev.c > +++ b/fsdev/qemu-fsdev.c > @@ -30,7 +30,7 @@ static FsDriverTable FsDrivers[] = { > { .name = "proxy", .ops = &proxy_ops}, > }; > > -int qemu_fsdev_add(QemuOpts *opts) > +int qemu_fsdev_add(QemuOpts *opts, Error **errp) > { > int i; > struct FsDriverListEntry *fsle; > @@ -38,10 +38,9 @@ int qemu_fsdev_add(QemuOpts *opts) > const char *fsdriver = qemu_opt_get(opts, "fsdriver"); > const char *writeout = qemu_opt_get(opts, "writeout"); > bool ro = qemu_opt_get_bool(opts, "readonly", 0); > - Error *local_err = NULL; > > if (!fsdev_id) { > - error_report("fsdev: No id specified"); > + error_setg(errp, "fsdev: No id specified"); > return -1; > } > > @@ -53,11 +52,11 @@ int qemu_fsdev_add(QemuOpts *opts) > } > > if (i == ARRAY_SIZE(FsDrivers)) { > - error_report("fsdev: fsdriver %s not found", fsdriver); > + error_setg(errp, "fsdev: fsdriver %s not found", fsdriver); > return -1; > } > } else { > - error_report("fsdev: No fsdriver specified"); > + error_setg(errp, "fsdev: No fsdriver specified"); > return -1; > } > > @@ -76,8 +75,7 @@ int qemu_fsdev_add(QemuOpts *opts) > } > > if (fsle->fse.ops->parse_opts) { > - if (fsle->fse.ops->parse_opts(opts, &fsle->fse, &local_err)) { > - error_report_err(local_err); > + if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) { > g_free(fsle->fse.fsdev_id); > g_free(fsle); > return -1; > diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h > index 65e4b1cfab..d9716b4144 100644 > --- a/fsdev/qemu-fsdev.h > +++ b/fsdev/qemu-fsdev.h > @@ -38,7 +38,7 @@ typedef struct FsDriverListEntry { > QTAILQ_ENTRY(FsDriverListEntry) next; > } FsDriverListEntry; > > -int qemu_fsdev_add(QemuOpts *opts); > +int qemu_fsdev_add(QemuOpts *opts, Error **errp); > FsDriverEntry *get_fsdev_fsentry(char *id); > extern FileOperations local_ops; > extern FileOperations handle_ops; > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c > index 6026780f95..3f54a21c76 100644 > --- a/hw/9pfs/xen-9p-backend.c > +++ b/hw/9pfs/xen-9p-backend.c > @@ -14,6 +14,7 @@ > #include "hw/9pfs/9p.h" > #include "hw/xen/xen_backend.h" > #include "hw/9pfs/xen-9pfs.h" > +#include "qapi/error.h" > #include "qemu/config-file.h" > #include "qemu/option.h" > #include "fsdev/qemu-fsdev.h" > @@ -355,6 +356,7 @@ static int xen_9pfs_free(struct XenDevice *xendev) > > static int xen_9pfs_connect(struct XenDevice *xendev) > { > + Error *err = NULL; > int i; > Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev); > V9fsState *s = &xen_9pdev->state; > @@ -452,7 +454,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev) > qemu_opt_set(fsdev, "path", xen_9pdev->path, NULL); > qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL); > qemu_opts_set_id(fsdev, s->fsconf.fsdev_id); > - qemu_fsdev_add(fsdev); > + qemu_fsdev_add(fsdev, &err); > + if (err) { > + error_report_err(err); > + } > v9fs_device_realize_common(s, &xen_9p_transport, NULL); > > return 0; > diff --git a/vl.c b/vl.c > index b6f9212f09..4aa976c4cc 100644 > --- a/vl.c > +++ b/vl.c > @@ -2249,7 +2249,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp) > #ifdef CONFIG_VIRTFS > static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp) > { > - return qemu_fsdev_add(opts); > + return qemu_fsdev_add(opts, errp); > } > #endif > > @@ -4188,10 +4188,8 @@ int main(int argc, char **argv, char **envp) > chardev_init_func, NULL, &error_fatal); > > #ifdef CONFIG_VIRTFS > - if (qemu_opts_foreach(qemu_find_opts("fsdev"), > - fsdev_init_func, NULL, NULL)) { > - exit(1); > - } > + qemu_opts_foreach(qemu_find_opts("fsdev"), > + fsdev_init_func, NULL, &error_fatal); > #endif > > if (qemu_opts_foreach(qemu_find_opts("device"),
Eric Blake <eblake@redhat.com> writes: > On 10/8/18 12:31 PM, Markus Armbruster wrote: >> Calling error_report() from within a a function that takes an Error ** > > s/a a/a/ Fixing, thanks! >> argument is suspicious. qemu_fsdev_add() does that, and its caller >> fsdev_init_func() then fails without setting an error. Its caller >> main(), via qemu_opts_foreach(), is fine with it, but clean it up >> anyway. >>
diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c index 6dc0fbc4c4..489cd29081 100644 --- a/fsdev/qemu-fsdev-dummy.c +++ b/fsdev/qemu-fsdev-dummy.c @@ -15,7 +15,7 @@ #include "qemu/config-file.h" #include "qemu/module.h" -int qemu_fsdev_add(QemuOpts *opts) +int qemu_fsdev_add(QemuOpts *opts, Error **errp) { return 0; } diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c index 8a4afbffbd..7a3b87cc9e 100644 --- a/fsdev/qemu-fsdev.c +++ b/fsdev/qemu-fsdev.c @@ -30,7 +30,7 @@ static FsDriverTable FsDrivers[] = { { .name = "proxy", .ops = &proxy_ops}, }; -int qemu_fsdev_add(QemuOpts *opts) +int qemu_fsdev_add(QemuOpts *opts, Error **errp) { int i; struct FsDriverListEntry *fsle; @@ -38,10 +38,9 @@ int qemu_fsdev_add(QemuOpts *opts) const char *fsdriver = qemu_opt_get(opts, "fsdriver"); const char *writeout = qemu_opt_get(opts, "writeout"); bool ro = qemu_opt_get_bool(opts, "readonly", 0); - Error *local_err = NULL; if (!fsdev_id) { - error_report("fsdev: No id specified"); + error_setg(errp, "fsdev: No id specified"); return -1; } @@ -53,11 +52,11 @@ int qemu_fsdev_add(QemuOpts *opts) } if (i == ARRAY_SIZE(FsDrivers)) { - error_report("fsdev: fsdriver %s not found", fsdriver); + error_setg(errp, "fsdev: fsdriver %s not found", fsdriver); return -1; } } else { - error_report("fsdev: No fsdriver specified"); + error_setg(errp, "fsdev: No fsdriver specified"); return -1; } @@ -76,8 +75,7 @@ int qemu_fsdev_add(QemuOpts *opts) } if (fsle->fse.ops->parse_opts) { - if (fsle->fse.ops->parse_opts(opts, &fsle->fse, &local_err)) { - error_report_err(local_err); + if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) { g_free(fsle->fse.fsdev_id); g_free(fsle); return -1; diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h index 65e4b1cfab..d9716b4144 100644 --- a/fsdev/qemu-fsdev.h +++ b/fsdev/qemu-fsdev.h @@ -38,7 +38,7 @@ typedef struct FsDriverListEntry { QTAILQ_ENTRY(FsDriverListEntry) next; } FsDriverListEntry; -int qemu_fsdev_add(QemuOpts *opts); +int qemu_fsdev_add(QemuOpts *opts, Error **errp); FsDriverEntry *get_fsdev_fsentry(char *id); extern FileOperations local_ops; extern FileOperations handle_ops; diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c index 6026780f95..3f54a21c76 100644 --- a/hw/9pfs/xen-9p-backend.c +++ b/hw/9pfs/xen-9p-backend.c @@ -14,6 +14,7 @@ #include "hw/9pfs/9p.h" #include "hw/xen/xen_backend.h" #include "hw/9pfs/xen-9pfs.h" +#include "qapi/error.h" #include "qemu/config-file.h" #include "qemu/option.h" #include "fsdev/qemu-fsdev.h" @@ -355,6 +356,7 @@ static int xen_9pfs_free(struct XenDevice *xendev) static int xen_9pfs_connect(struct XenDevice *xendev) { + Error *err = NULL; int i; Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev); V9fsState *s = &xen_9pdev->state; @@ -452,7 +454,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev) qemu_opt_set(fsdev, "path", xen_9pdev->path, NULL); qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL); qemu_opts_set_id(fsdev, s->fsconf.fsdev_id); - qemu_fsdev_add(fsdev); + qemu_fsdev_add(fsdev, &err); + if (err) { + error_report_err(err); + } v9fs_device_realize_common(s, &xen_9p_transport, NULL); return 0; diff --git a/vl.c b/vl.c index b6f9212f09..4aa976c4cc 100644 --- a/vl.c +++ b/vl.c @@ -2249,7 +2249,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp) #ifdef CONFIG_VIRTFS static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp) { - return qemu_fsdev_add(opts); + return qemu_fsdev_add(opts, errp); } #endif @@ -4188,10 +4188,8 @@ int main(int argc, char **argv, char **envp) chardev_init_func, NULL, &error_fatal); #ifdef CONFIG_VIRTFS - if (qemu_opts_foreach(qemu_find_opts("fsdev"), - fsdev_init_func, NULL, NULL)) { - exit(1); - } + qemu_opts_foreach(qemu_find_opts("fsdev"), + fsdev_init_func, NULL, &error_fatal); #endif if (qemu_opts_foreach(qemu_find_opts("device"),
Calling error_report() from within a a function that takes an Error ** argument is suspicious. qemu_fsdev_add() does that, and its caller fsdev_init_func() then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. Cc: Greg Kurz <groug@kaod.org> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- fsdev/qemu-fsdev-dummy.c | 2 +- fsdev/qemu-fsdev.c | 12 +++++------- fsdev/qemu-fsdev.h | 2 +- hw/9pfs/xen-9p-backend.c | 7 ++++++- vl.c | 8 +++----- 5 files changed, 16 insertions(+), 15 deletions(-)