On Wed, Dec 07, 2022 at 12:50:42PM +0530, Viresh Kumar wrote:
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index fa3d61f1e882..ab3668b3b8a3 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c +static int make_virtio_mmio_node_device(libxl__gc *gc, void *fdt, uint64_t base,
uint32_t irq, const char *type,uint32_t backend_domid)+{
- int res, len = strlen(type);
- res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
- if (res) return res;
- /* Add device specific nodes */
- if (!strncmp(type, "virtio,device22", len)) {
So with `len` been the strlen() of `type`, I think that a type "v" or "virtio" or even "virtio,device" is going to create the "i2c" node. So I don't think is going to be possible to create a generic virtio device node.
Using strcmp() would be good enough here I think.
res = make_virtio_mmio_node_i2c(gc, fdt);if (res) return res;- } else if (!strncmp(type, "virtio,device29", len)) {
res = make_virtio_mmio_node_gpio(gc, fdt);if (res) return res;- } else if (!strncmp(type, "virtio,device", len)) {
The example in in the commit message has "virtio,devices" but that would be rejected by this test due to the extra 's'.
/* Generic Virtio Device */res = fdt_end_node(fdt);
Isn't this an extra end_node() call? As there's already one for the return of the function.
if (res) return res;- } else {
LOG(ERROR, "Invalid type for virtio device: %s", type);return -EINVAL;- }
- return fdt_end_node(fdt);
+}
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c new file mode 100644 index 000000000000..64cec989c674 --- /dev/null +++ b/tools/libs/light/libxl_virtio.c +static int libxl__virtio_from_xenstore(libxl__gc *gc, const char *libxl_path,
libxl_devid devid,libxl_device_virtio *virtio)+{
- const char *be_path, *tmp = NULL;
- int rc;
- virtio->devid = devid;
- rc = libxl__xs_read_mandatory(gc, XBT_NULL,
GCSPRINTF("%s/backend", libxl_path),&be_path);- if (rc) goto out;
- rc = libxl__backendpath_parse_domid(gc, be_path, &virtio->backend_domid);
- if (rc) goto out;
- rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/irq", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
virtio->irq = strtoul(tmp, NULL, 0);- }
- tmp = NULL;
- rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/base", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
virtio->base = strtoul(tmp, NULL, 0);- }
- tmp = NULL;
- rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/transport", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
if (!strncmp(tmp, "mmio", strlen(tmp))) {
Maybe just use strcmp(), otherwise we have "m" going to be match as MMIO for example.
virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;} else if (!strncmp(tmp, "unknown", strlen(tmp))) {virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;
I don't think that value should be allowed in xenstore. If `transport` isn't set to a proper value (only "mmio"), then I think that invalid.
} else {return ERROR_INVAL;}- }
- tmp = NULL;
- rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/type", be_path), &tmp);- if (rc) goto out;
- if (tmp) {
if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
Nit: Something like: const char check[] = "virtio,device"; const size_t checkl = sizeof(check) - 1; ... strncmp(tmp, check, checkl)... (or just strncmp(tmp, check, sizeof(check)-1)) would avoid issue with both string "virtio,device" potentially been different.
virtio->type = strdup(tmp);
Could you use libxl__strdup(NO_GC, tmp) instead? That function is going to check that strdup() doesn't fail the allocation.
Thanks,