diff mbox series

[v5,1/6] tools/cpupools: Give a name to unnamed cpupools

Message ID 20220405085741.18336-2-luca.fancellu@arm.com (mailing list archive)
State Superseded
Headers show
Series Boot time cpupools | expand

Commit Message

Luca Fancellu April 5, 2022, 8:57 a.m. UTC 
With the introduction of boot time cpupools, Xen can create many
different cpupools at boot time other than cpupool with id 0.

Since these newly created cpupools can't have an
entry in Xenstore, create the entry using xen-init-dom0
helper with the usual convention: Pool-<cpupool id>.

Given the change, remove the check for poolid == 0 from
libxl_cpupoolid_to_name(...).

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Changes in v5:
- no changes
Changes in v4:
- no changes
Changes in v3:
- no changes, add R-by
Changes in v2:
 - Remove unused variable, moved xc_cpupool_infofree
   ahead to simplify the code, use asprintf (Juergen)
---
 tools/helpers/xen-init-dom0.c  | 35 +++++++++++++++++++++++++++++++++-
 tools/libs/light/libxl_utils.c |  3 +--
 2 files changed, 35 insertions(+), 3 deletions(-)

Comments

Anthony PERARD April 6, 2022, 2:55 p.m. UTC | #1 
Hi Luca,

On Tue, Apr 05, 2022 at 09:57:36AM +0100, Luca Fancellu wrote:
> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
> index c99224a4b607..84286617790f 100644
> --- a/tools/helpers/xen-init-dom0.c
> +++ b/tools/helpers/xen-init-dom0.c
> @@ -43,7 +43,9 @@ int main(int argc, char **argv)
>      int rc;
>      struct xs_handle *xsh = NULL;
>      xc_interface *xch = NULL;
> -    char *domname_string = NULL, *domid_string = NULL;
> +    char *domname_string = NULL, *domid_string = NULL, *pool_path, *pool_name;
> +    xc_cpupoolinfo_t *xcinfo;
> +    unsigned int pool_id = 0;
>      libxl_uuid uuid;
>  
>      /* Accept 0 or 1 argument */
> @@ -114,6 +116,37 @@ int main(int argc, char **argv)
>          goto out;
>      }
>  
> +    /* Create an entry in xenstore for each cpupool on the system */
> +    do {
> +        xcinfo = xc_cpupool_getinfo(xch, pool_id);
> +        if (xcinfo != NULL) {
> +            if (xcinfo->cpupool_id != pool_id)
> +                pool_id = xcinfo->cpupool_id;
> +            xc_cpupool_infofree(xch, xcinfo);
> +            if (asprintf(&pool_path, "/local/pool/%d/name", pool_id) <= 0) {
> +                fprintf(stderr, "cannot allocate memory for pool path\n");
> +                rc = 1;
> +                goto out;
> +            }
> +            if (asprintf(&pool_name, "Pool-%d", pool_id) <= 0) {
> +                fprintf(stderr, "cannot allocate memory for pool name\n");
> +                rc = 1;
> +                goto out_err;

Could you rework this loop so that "goto out" is always the right things
to do in case of error? That is the function would always free()
"pool_path" and "pool_name" regardless of their values, and we only need
to make sure both are NULL when they are already free. This is to avoid
having several path in case of error, as this could result in mistake
later. If there's only one error path, there will be less likely to have
mistakes. This loop, at the moment, is using three different error
paths: "goto out", "goto out_err", and no goto followed by a standalone
goto.

> +            }
> +            pool_id++;
> +            if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
> +                          strlen(pool_name))) {
> +                fprintf(stderr, "cannot set pool name\n");
> +                rc = 1;
> +            }
> +            free(pool_name);
> +out_err:
> +            free(pool_path);
> +            if ( rc )
> +                goto out;
> +        }
> +    } while(xcinfo != NULL);

Thanks,
Luca Fancellu April 7, 2022, 8:26 a.m. UTC | #2 
> Could you rework this loop so that "goto out" is always the right things
> to do in case of error? That is the function would always free()
> "pool_path" and "pool_name" regardless of their values, and we only need
> to make sure both are NULL when they are already free. This is to avoid
> having several path in case of error, as this could result in mistake
> later. If there's only one error path, there will be less likely to have
> mistakes. This loop, at the moment, is using three different error
> paths: "goto out", "goto out_err", and no goto followed by a standalone
> goto.

Hi Anthony,

Yes I will rework it and send v6. Thanks for your review.

Cheers,
Luca

> 
>> +            }
>> +            pool_id++;
>> +            if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
>> +                          strlen(pool_name))) {
>> +                fprintf(stderr, "cannot set pool name\n");
>> +                rc = 1;
>> +            }
>> +            free(pool_name);
>> +out_err:
>> +            free(pool_path);
>> +            if ( rc )
>> +                goto out;
>> +        }
>> +    } while(xcinfo != NULL);
> 
> Thanks,
> 
> -- 
> Anthony PERARD
diff mbox series

Patch

diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index c99224a4b607..84286617790f 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -43,7 +43,9 @@  int main(int argc, char **argv)
     int rc;
     struct xs_handle *xsh = NULL;
     xc_interface *xch = NULL;
-    char *domname_string = NULL, *domid_string = NULL;
+    char *domname_string = NULL, *domid_string = NULL, *pool_path, *pool_name;
+    xc_cpupoolinfo_t *xcinfo;
+    unsigned int pool_id = 0;
     libxl_uuid uuid;
 
     /* Accept 0 or 1 argument */
@@ -114,6 +116,37 @@  int main(int argc, char **argv)
         goto out;
     }
 
+    /* Create an entry in xenstore for each cpupool on the system */
+    do {
+        xcinfo = xc_cpupool_getinfo(xch, pool_id);
+        if (xcinfo != NULL) {
+            if (xcinfo->cpupool_id != pool_id)
+                pool_id = xcinfo->cpupool_id;
+            xc_cpupool_infofree(xch, xcinfo);
+            if (asprintf(&pool_path, "/local/pool/%d/name", pool_id) <= 0) {
+                fprintf(stderr, "cannot allocate memory for pool path\n");
+                rc = 1;
+                goto out;
+            }
+            if (asprintf(&pool_name, "Pool-%d", pool_id) <= 0) {
+                fprintf(stderr, "cannot allocate memory for pool name\n");
+                rc = 1;
+                goto out_err;
+            }
+            pool_id++;
+            if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
+                          strlen(pool_name))) {
+                fprintf(stderr, "cannot set pool name\n");
+                rc = 1;
+            }
+            free(pool_name);
+out_err:
+            free(pool_path);
+            if ( rc )
+                goto out;
+        }
+    } while(xcinfo != NULL);
+
     printf("Done setting up Dom0\n");
 
 out:
diff --git a/tools/libs/light/libxl_utils.c b/tools/libs/light/libxl_utils.c
index b91c2cafa223..81780da3ff40 100644
--- a/tools/libs/light/libxl_utils.c
+++ b/tools/libs/light/libxl_utils.c
@@ -151,8 +151,7 @@  char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid)
 
     snprintf(path, sizeof(path), "/local/pool/%d/name", poolid);
     s = xs_read(ctx->xsh, XBT_NULL, path, &len);
-    if (!s && (poolid == 0))
-        return strdup("Pool-0");
+
     return s;
 }