Re: [PATCH V7 1/3] libxl: Add support for generic virtio device

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 08-12-22, 18:06, Anthony PERARD wrote:
> 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.

I think that is a generic problem with all the strings I am using. What about
this diff over current patch ?

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index ab3668b3b8a3..292b31881210 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -981,7 +981,7 @@ static int make_virtio_mmio_node_i2c(libxl__gc *gc, void 
*fdt)
     res = fdt_begin_node(fdt, "i2c");
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 1, "virtio,device22");
+    res = fdt_property_compat(gc, fdt, 1, VIRTIO_DEVICE_TYPE_I2C);
     if (res) return res;
 
     return fdt_end_node(fdt);
@@ -999,7 +999,7 @@ static int make_virtio_mmio_node_gpio(libxl__gc *gc, void 
*fdt)
     res = fdt_begin_node(fdt, "gpio");
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 1, "virtio,device29");
+    res = fdt_property_compat(gc, fdt, 1, VIRTIO_DEVICE_TYPE_GPIO);
     if (res) return res;
 
     res = fdt_property(fdt, "gpio-controller", NULL, 0);
@@ -1021,23 +1021,20 @@ 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);
+    int res;
 
     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)) {
+    if (!strcmp(type, VIRTIO_DEVICE_TYPE_I2C)) {
         res = make_virtio_mmio_node_i2c(gc, fdt);
         if (res) return res;
-    } else if (!strncmp(type, "virtio,device29", len)) {
+    } else if (!strcmp(type, VIRTIO_DEVICE_TYPE_GPIO)) {
         res = make_virtio_mmio_node_gpio(gc, fdt);
         if (res) return res;
-    } else if (!strncmp(type, "virtio,device", len)) {
-        /* Generic Virtio Device */
-        res = fdt_end_node(fdt);
-        if (res) return res;
-    } else {
+    } else if (strcmp(type, VIRTIO_DEVICE_TYPE_GENERIC)) {
+        /* Doesn't match generic virtio device */
         LOG(ERROR, "Invalid type for virtio device: %s", type);
         return -EINVAL;
     }
diff --git a/tools/libs/light/libxl_internal.h 
b/tools/libs/light/libxl_internal.h
index cdd155d925c1..a062fca0e2bb 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -166,6 +166,11 @@
 /* Convert pfn to physical address space. */
 #define pfn_to_paddr(x) ((uint64_t)(x) << XC_PAGE_SHIFT)
 
+/* Virtio device types */
+#define VIRTIO_DEVICE_TYPE_GENERIC   "virtio,device"
+#define VIRTIO_DEVICE_TYPE_GPIO      "virtio,device22"
+#define VIRTIO_DEVICE_TYPE_I2C       "virtio,device29"
+
 /* logging */
 _hidden void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int 
errnoval,
              const char *file /* may be 0 */, int line /* ignored if !file */,
diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
index 64cec989c674..183d9c906e27 100644
--- a/tools/libs/light/libxl_virtio.c
+++ b/tools/libs/light/libxl_virtio.c
@@ -62,7 +62,7 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const 
char *libxl_path,
                                        libxl_device_virtio *virtio)
 {
     const char *be_path, *tmp = NULL;
-    int rc;
+    int rc, len = sizeof(VIRTIO_DEVICE_TYPE_GENERIC) - 1;
 
     virtio->devid = devid;
 
@@ -97,10 +97,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const 
char *libxl_path,
     if (rc) goto out;
 
     if (tmp) {
-        if (!strncmp(tmp, "mmio", strlen(tmp))) {
+        if (!strcmp(tmp, "mmio")) {
             virtio->transport = LIBXL_VIRTIO_TRANSPORT_MMIO;
-        } else if (!strncmp(tmp, "unknown", strlen(tmp))) {
-            virtio->transport = LIBXL_VIRTIO_TRANSPORT_UNKNOWN;
         } else {
             return ERROR_INVAL;
         }
@@ -112,8 +110,8 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, const 
char *libxl_path,
     if (rc) goto out;
 
     if (tmp) {
-        if (!strncmp(tmp, "virtio,device", strlen("virtio,device"))) {
-            virtio->type = strdup(tmp);
+        if (!strncmp(tmp, VIRTIO_DEVICE_TYPE_GENERIC, len)) {
+            virtio->type = libxl__strdup(NOGC, tmp);
         } else {
             return ERROR_INVAL;
         }

-- 
viresh