| 1 | Hello, | 1 | Hello, |
|---|---|---|---|
| 2 | 2 | ||
| 3 | First patch fixes some error handling paths that incorrectly used | 3 | First patch from David introduces a new helper to fetch xenstore nodes, |
| 4 | error_prepend() in the Xen console driver. Second patch removes usage | 4 | while second patch removes the usage of scanf related functions with the |
| 5 | of the 'm' character in scanf directives, as it's not supported on | 5 | "%ms" format specifier, as it's not supported by the FreeBSD scanf libc |
| 6 | FreeBSD (see usages of "%ms"). | 6 | implementation. |
| 7 | 7 | ||
| 8 | Thanks, Roger. | 8 | Thanks, Roger. |
| 9 | 9 | ||
| 10 | Roger Pau Monné (2): | 10 | David Woodhouse (1): |
| 11 | xen/console: fix error handling in xen_console_device_create() | 11 | hw/xen: Add xs_node_read() helper function |
| 12 | |||
| 13 | Roger Pau Monné (1): | ||
| 12 | xen: do not use '%ms' scanf specifier | 14 | xen: do not use '%ms' scanf specifier |
| 13 | 15 | ||
| 14 | hw/char/xen_console.c | 15 +++++++++++---- | 16 | hw/block/xen-block.c | 3 ++- |
| 15 | hw/xen/xen-bus.c | 7 +++++-- | 17 | hw/char/xen_console.c | 14 ++++++++------ |
| 16 | 2 files changed, 16 insertions(+), 6 deletions(-) | 18 | hw/xen/trace-events | 1 + |
| 19 | hw/xen/xen-bus-helper.c | 22 ++++++++++++++++++++++ | ||
| 20 | hw/xen/xen-bus.c | 14 ++++++++++++-- | ||
| 21 | include/hw/xen/xen-bus-helper.h | 4 ++++ | ||
| 22 | include/hw/xen/xen-bus.h | 1 + | ||
| 23 | 7 files changed, 50 insertions(+), 9 deletions(-) | ||
| 24 | |||
| 17 | -- | 25 | -- |
| 18 | 2.46.0 | 26 | 2.46.0 |
| 19 | 27 | ||
| 20 | 28 | diff view generated by jsdifflib |
| 1 | The usage of error_prepend() in some of the error contexts of | 1 | From: David Woodhouse <dwmw@amazon.co.uk> |
|---|---|---|---|
| 2 | xen_console_device_create() is incorrect, as `errp` hasn't been initialized. | ||
| 3 | This leads to the following segmentation fault on error paths resulting from | ||
| 4 | xenstore reads: | ||
| 5 | 2 | ||
| 6 | Program terminated with signal SIGSEGV, Segmentation fault. | 3 | This returns the full contents of the node, having created the node path |
| 7 | Address not mapped to object. | 4 | from the printf-style format string provided in its arguments. |
| 8 | fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50) | ||
| 9 | at ../qemu-xen-dir-remote/util/error.c:142 | ||
| 10 | 142 g_string_append(newmsg, (*errp)->msg); | ||
| 11 | [...] | ||
| 12 | (gdb) bt | ||
| 13 | (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50) at ../qemu-xen-dir-remote/util/error.c:142 | ||
| 14 | (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ") | ||
| 15 | at ../qemu-xen-dir-remote/util/error.c:152 | ||
| 16 | (backend=0x43944de00660, opts=0x43944c929000, errp=0x15cd0165ae10) | ||
| 17 | at ../qemu-xen-dir-remote/hw/char/xen_console.c:555 | ||
| 18 | 5 | ||
| 19 | Replace usages of error_prepend() with error_setg() where appropriate. | 6 | This will save various callers from having to do so for themselves (and |
| 7 | from using xs_node_scanf() with the non-portable %ms format string. | ||
| 20 | 8 | ||
| 21 | Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model') | 9 | Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> |
| 10 | [remove double newline and constify trace parameters] | ||
| 22 | Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> | 11 | Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> |
| 23 | --- | 12 | --- |
| 24 | Cc: Stefano Stabellini <sstabellini@kernel.org> | 13 | Cc: Stefano Stabellini <sstabellini@kernel.org> |
| 25 | Cc: Anthony PERARD <anthony@xenproject.org> | 14 | Cc: Anthony PERARD <anthony@xenproject.org> |
| 26 | Cc: Paul Durrant <paul@xen.org> | 15 | Cc: Paul Durrant <paul@xen.org> |
| 27 | Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> | 16 | Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> |
| 28 | Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> | ||
| 29 | Cc: Paolo Bonzini <pbonzini@redhat.com> | ||
| 30 | Cc: xen-devel@lists.xenproject.org | 17 | Cc: xen-devel@lists.xenproject.org |
| 31 | --- | 18 | --- |
| 32 | hw/char/xen_console.c | 4 ++-- | 19 | hw/xen/trace-events | 1 + |
| 33 | 1 file changed, 2 insertions(+), 2 deletions(-) | 20 | hw/xen/xen-bus-helper.c | 22 ++++++++++++++++++++++ |
| 21 | include/hw/xen/xen-bus-helper.h | 4 ++++ | ||
| 22 | 3 files changed, 27 insertions(+) | ||
| 34 | 23 | ||
| 35 | diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c | 24 | diff --git a/hw/xen/trace-events b/hw/xen/trace-events |
| 36 | index XXXXXXX..XXXXXXX 100644 | 25 | index XXXXXXX..XXXXXXX 100644 |
| 37 | --- a/hw/char/xen_console.c | 26 | --- a/hw/xen/trace-events |
| 38 | +++ b/hw/char/xen_console.c | 27 | +++ b/hw/xen/trace-events |
| 39 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, | 28 | @@ -XXX,XX +XXX,XX @@ xs_node_create(const char *node) "%s" |
| 40 | } | 29 | xs_node_destroy(const char *node) "%s" |
| 41 | 30 | xs_node_vprintf(char *path, char *value) "%s %s" | |
| 42 | if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) { | 31 | xs_node_vscanf(char *path, char *value) "%s %s" |
| 43 | - error_prepend(errp, "failed to read console device type: "); | 32 | +xs_node_read(const char *path, const char *value) "%s %s" |
| 44 | + error_setg(errp, "failed to read console device type: "); | 33 | xs_node_watch(char *path) "%s" |
| 45 | goto fail; | 34 | xs_node_unwatch(char *path) "%s" |
| 46 | } | 35 | |
| 47 | 36 | diff --git a/hw/xen/xen-bus-helper.c b/hw/xen/xen-bus-helper.c | |
| 48 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, | 37 | index XXXXXXX..XXXXXXX 100644 |
| 49 | } else if (number) { | 38 | --- a/hw/xen/xen-bus-helper.c |
| 50 | cd = serial_hd(number); | 39 | +++ b/hw/xen/xen-bus-helper.c |
| 51 | if (!cd) { | 40 | @@ -XXX,XX +XXX,XX @@ int xs_node_scanf(struct qemu_xs_handle *h, xs_transaction_t tid, |
| 52 | - error_prepend(errp, "console: No serial device #%ld found: ", | 41 | return rc; |
| 53 | + error_setg(errp, "console: No serial device #%ld found: ", | 42 | } |
| 54 | number); | 43 | |
| 55 | goto fail; | 44 | +char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid, |
| 56 | } | 45 | + unsigned int *len, Error **errp, |
| 46 | + const char *node_fmt, ...) | ||
| 47 | +{ | ||
| 48 | + char *path, *value; | ||
| 49 | + va_list ap; | ||
| 50 | + | ||
| 51 | + va_start(ap, node_fmt); | ||
| 52 | + path = g_strdup_vprintf(node_fmt, ap); | ||
| 53 | + va_end(ap); | ||
| 54 | + | ||
| 55 | + value = qemu_xen_xs_read(h, tid, path, len); | ||
| 56 | + trace_xs_node_read(path, value); | ||
| 57 | + if (!value) { | ||
| 58 | + error_setg_errno(errp, errno, "failed to read from '%s'", path); | ||
| 59 | + } | ||
| 60 | + | ||
| 61 | + g_free(path); | ||
| 62 | + | ||
| 63 | + return value; | ||
| 64 | +} | ||
| 65 | + | ||
| 66 | struct qemu_xs_watch *xs_node_watch(struct qemu_xs_handle *h, const char *node, | ||
| 67 | const char *key, xs_watch_fn fn, | ||
| 68 | void *opaque, Error **errp) | ||
| 69 | diff --git a/include/hw/xen/xen-bus-helper.h b/include/hw/xen/xen-bus-helper.h | ||
| 70 | index XXXXXXX..XXXXXXX 100644 | ||
| 71 | --- a/include/hw/xen/xen-bus-helper.h | ||
| 72 | +++ b/include/hw/xen/xen-bus-helper.h | ||
| 73 | @@ -XXX,XX +XXX,XX @@ int xs_node_scanf(struct qemu_xs_handle *h, xs_transaction_t tid, | ||
| 74 | const char *node, const char *key, Error **errp, | ||
| 75 | const char *fmt, ...) | ||
| 76 | G_GNUC_SCANF(6, 7); | ||
| 77 | +char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid, | ||
| 78 | + unsigned int *len, Error **errp, | ||
| 79 | + const char *node_fmt, ...) | ||
| 80 | + G_GNUC_PRINTF(5, 6); | ||
| 81 | |||
| 82 | /* Watch node/key unless node is empty, in which case watch key */ | ||
| 83 | struct qemu_xs_watch *xs_node_watch(struct qemu_xs_handle *h, const char *node, | ||
| 57 | -- | 84 | -- |
| 58 | 2.46.0 | 85 | 2.46.0 |
| 59 | 86 | ||
| 60 | 87 | diff view generated by jsdifflib |
| 1 | The 'm' parameter used to request auto-allocation of the destination variable | 1 | The 'm' parameter used to request auto-allocation of the destination variable |
|---|---|---|---|
| 2 | is not supported on FreeBSD, and as such leads to failures to parse. | 2 | is not supported on FreeBSD, and as such leads to failures to parse. |
| 3 | 3 | ||
| 4 | What's more, the current usage of '%ms' with xs_node_scanf() is pointless, as | 4 | What's more, the current usage of '%ms' with xs_node_scanf() is pointless, as |
| 5 | it just leads to a double allocation of the same string. Instead use | 5 | it just leads to a double allocation of the same string. Instead use |
| 6 | qemu_xen_xs_read() to read the whole xenstore node. | 6 | xs_node_read() to read the whole xenstore node. |
| 7 | 7 | ||
| 8 | Fixes: a783f8ad4ec9 ('xen: add a mechanism to automatically create XenDevice-s...') | 8 | Fixes: a783f8ad4ec9 ('xen: add a mechanism to automatically create XenDevice-s...') |
| 9 | Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model') | 9 | Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model') |
| 10 | Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> | 10 | Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> |
| 11 | --- | 11 | --- |
| 12 | Changes since v2: | ||
| 13 | - New version of xs_node_read(). | ||
| 14 | - Fix usage of %ms in xen-block.c | ||
| 15 | |||
| 16 | Changes since v1: | ||
| 17 | - Introduce xs_node_read() helper. | ||
| 18 | - Merge with errp fixes. | ||
| 19 | --- | ||
| 12 | Cc: Stefano Stabellini <sstabellini@kernel.org> | 20 | Cc: Stefano Stabellini <sstabellini@kernel.org> |
| 13 | Cc: Anthony PERARD <anthony@xenproject.org> | 21 | Cc: Anthony PERARD <anthony@xenproject.org> |
| 14 | Cc: Paul Durrant <paul@xen.org> | 22 | Cc: Paul Durrant <paul@xen.org> |
| 15 | Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> | 23 | Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> |
| 24 | Cc: Kevin Wolf <kwolf@redhat.com> | ||
| 25 | Cc: Hanna Reitz <hreitz@redhat.com> | ||
| 16 | Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> | 26 | Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> |
| 17 | Cc: Paolo Bonzini <pbonzini@redhat.com> | 27 | Cc: Paolo Bonzini <pbonzini@redhat.com> |
| 18 | Cc: xen-devel@lists.xenproject.org | 28 | Cc: xen-devel@lists.xenproject.org |
| 29 | Cc: qemu-block@nongnu.org | ||
| 19 | --- | 30 | --- |
| 20 | hw/char/xen_console.c | 11 +++++++++-- | 31 | hw/block/xen-block.c | 3 ++- |
| 21 | hw/xen/xen-bus.c | 7 +++++-- | 32 | hw/char/xen_console.c | 6 ++++-- |
| 22 | 2 files changed, 14 insertions(+), 4 deletions(-) | 33 | hw/xen/xen-bus.c | 14 ++++++++++++-- |
| 34 | include/hw/xen/xen-bus.h | 1 + | ||
| 35 | 4 files changed, 19 insertions(+), 5 deletions(-) | ||
| 23 | 36 | ||
| 37 | diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c | ||
| 38 | index XXXXXXX..XXXXXXX 100644 | ||
| 39 | --- a/hw/block/xen-block.c | ||
| 40 | +++ b/hw/block/xen-block.c | ||
| 41 | @@ -XXX,XX +XXX,XX @@ static void xen_block_connect(XenDevice *xendev, Error **errp) | ||
| 42 | return; | ||
| 43 | } | ||
| 44 | |||
| 45 | - if (xen_device_frontend_scanf(xendev, "protocol", "%ms", &str) != 1) { | ||
| 46 | + str = xen_device_frontend_read(xendev, "protocol"); | ||
| 47 | + if (!str) { | ||
| 48 | /* x86 defaults to the 32-bit protocol even for 64-bit guests. */ | ||
| 49 | if (object_dynamic_cast(OBJECT(qdev_get_machine()), "x86-machine")) { | ||
| 50 | protocol = BLKIF_PROTOCOL_X86_32; | ||
| 24 | diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c | 51 | diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c |
| 25 | index XXXXXXX..XXXXXXX 100644 | 52 | index XXXXXXX..XXXXXXX 100644 |
| 26 | --- a/hw/char/xen_console.c | 53 | --- a/hw/char/xen_console.c |
| 27 | +++ b/hw/char/xen_console.c | 54 | +++ b/hw/char/xen_console.c |
| 28 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, | 55 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, |
| 29 | const char *name = xen_backend_get_name(backend); | ||
| 30 | unsigned long number; | ||
| 31 | char *fe = NULL, *type = NULL, *output = NULL; | ||
| 32 | + const char *node_path; | ||
| 33 | char label[32]; | ||
| 34 | XenDevice *xendev = NULL; | ||
| 35 | XenConsole *con; | ||
| 36 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, | ||
| 37 | goto fail; | 56 | goto fail; |
| 38 | } | 57 | } |
| 39 | 58 | ||
| 40 | - if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) { | 59 | - if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) { |
| 41 | + node_path = g_strdup_printf("%s/type", fe); | 60 | + type = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "type"); |
| 42 | + type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL); | ||
| 43 | + g_free(node_path); | ||
| 44 | + if (!type) { | 61 | + if (!type) { |
| 45 | error_setg(errp, "failed to read console device type: "); | 62 | error_prepend(errp, "failed to read console device type: "); |
| 46 | goto fail; | 63 | goto fail; |
| 47 | } | 64 | } |
| 48 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, | 65 | @@ -XXX,XX +XXX,XX @@ static void xen_console_device_create(XenBackendInstance *backend, |
| 49 | 66 | ||
| 50 | snprintf(label, sizeof(label), "xencons%ld", number); | 67 | snprintf(label, sizeof(label), "xencons%ld", number); |
| 51 | 68 | ||
| 52 | - if (xs_node_scanf(xsh, XBT_NULL, fe, "output", NULL, "%ms", &output) == 1) { | 69 | - if (xs_node_scanf(xsh, XBT_NULL, fe, "output", NULL, "%ms", &output) == 1) { |
| 53 | + node_path = g_strdup_printf("%s/output", fe); | 70 | + output = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "output"); |
| 54 | + output = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL); | 71 | + if (output) { |
| 55 | + g_free(node_path); | ||
| 56 | + if (!output) { | ||
| 57 | /* | 72 | /* |
| 58 | * FIXME: sure we want to support implicit | 73 | * FIXME: sure we want to support implicit |
| 59 | * muxed monitors here? | 74 | * muxed monitors here? |
| 60 | diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c | 75 | diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c |
| 61 | index XXXXXXX..XXXXXXX 100644 | 76 | index XXXXXXX..XXXXXXX 100644 |
| 62 | --- a/hw/xen/xen-bus.c | 77 | --- a/hw/xen/xen-bus.c |
| 63 | +++ b/hw/xen/xen-bus.c | 78 | +++ b/hw/xen/xen-bus.c |
| 64 | @@ -XXX,XX +XXX,XX @@ again: | 79 | @@ -XXX,XX +XXX,XX @@ again: |
| 65 | |||
| 66 | opts = qdict_new(); | ||
| 67 | for (i = 0; i < n; i++) { | ||
| 68 | + const char *node_path; | ||
| 69 | char *val; | ||
| 70 | |||
| 71 | /* | ||
| 72 | @@ -XXX,XX +XXX,XX @@ again: | ||
| 73 | !strcmp(key[i], "hotplug-status")) | 80 | !strcmp(key[i], "hotplug-status")) |
| 74 | continue; | 81 | continue; |
| 75 | 82 | ||
| 76 | - if (xs_node_scanf(xenbus->xsh, tid, path, key[i], NULL, "%ms", | 83 | - if (xs_node_scanf(xenbus->xsh, tid, path, key[i], NULL, "%ms", |
| 77 | - &val) == 1) { | 84 | - &val) == 1) { |
| 78 | + node_path = g_strdup_printf("%s/%s", path, key[i]); | 85 | + val = xs_node_read(xenbus->xsh, tid, NULL, NULL, "%s/%s", path, key[i]); |
| 79 | + val = qemu_xen_xs_read(xenbus->xsh, tid, node_path, NULL); | ||
| 80 | + g_free(node_path); | ||
| 81 | + if (val) { | 86 | + if (val) { |
| 82 | qdict_put_str(opts, key[i], val); | 87 | qdict_put_str(opts, key[i], val); |
| 83 | free(val); | 88 | free(val); |
| 84 | } | 89 | } |
| 90 | @@ -XXX,XX +XXX,XX @@ int xen_device_frontend_scanf(XenDevice *xendev, const char *key, | ||
| 91 | return rc; | ||
| 92 | } | ||
| 93 | |||
| 94 | +char *xen_device_frontend_read(XenDevice *xendev, const char *key) | ||
| 95 | +{ | ||
| 96 | + XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev))); | ||
| 97 | + | ||
| 98 | + g_assert(xenbus->xsh); | ||
| 99 | + | ||
| 100 | + return xs_node_read(xenbus->xsh, XBT_NULL, NULL, NULL, "%s/%s", | ||
| 101 | + xendev->frontend_path, key);; | ||
| 102 | +} | ||
| 103 | + | ||
| 104 | static void xen_device_frontend_set_state(XenDevice *xendev, | ||
| 105 | enum xenbus_state state, | ||
| 106 | bool publish) | ||
| 107 | diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h | ||
| 108 | index XXXXXXX..XXXXXXX 100644 | ||
| 109 | --- a/include/hw/xen/xen-bus.h | ||
| 110 | +++ b/include/hw/xen/xen-bus.h | ||
| 111 | @@ -XXX,XX +XXX,XX @@ void xen_device_frontend_printf(XenDevice *xendev, const char *key, | ||
| 112 | int xen_device_frontend_scanf(XenDevice *xendev, const char *key, | ||
| 113 | const char *fmt, ...) | ||
| 114 | G_GNUC_SCANF(3, 4); | ||
| 115 | +char *xen_device_frontend_read(XenDevice *xendev, const char *key); | ||
| 116 | |||
| 117 | void xen_device_set_max_grant_refs(XenDevice *xendev, unsigned int nr_refs, | ||
| 118 | Error **errp); | ||
| 85 | -- | 119 | -- |
| 86 | 2.46.0 | 120 | 2.46.0 |
| 87 | 121 | ||
| 88 | 122 | diff view generated by jsdifflib |