Current use of err() has the following issues:
- without setting errno, on error it results in printing e.g.:
 "init-dom0less: writing to xenstore: Success"
 This is very misleading and difficult to deduct that there was a
 failure.
- does not propagate error codes to the caller.
- skips "init_domain failed" message by exiting early.

Replace err() with more informative messages propagating rc when
possible.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 tools/helpers/init-dom0less.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index a182dce56353..3dd2d74886eb 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -288,24 +288,33 @@ static int init_domain(struct xs_handle *xsh,
 
         rc = xc_dom_gnttab_seed(xch, info->domid, true,
                                 (xen_pfn_t)-1, xenstore_pfn, 0, 0);
-        if (rc)
-               err(1, "xc_dom_gnttab_seed");
+        if (rc) {
+            printf("Failed to seed gnttab entries\n");
+            return rc;
+        }
     }
 
     libxl_uuid_generate(&uuid);
     xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
 
     rc = gen_stub_json_config(info->domid, &uuid);
-    if (rc)
-        err(1, "gen_stub_json_config");
+    if (rc) {
+        printf("Failed to create stub json config\n");
+        return rc;
+    }
 
     rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
-    if (rc)
-        err(1, "writing to xenstore");
+    if (rc) {
+        printf("Failed to write to xenstore\n");
+        return rc;
+    }
 
     rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
-    if (!rc)
-        err(1, "xs_introduce_domain");
+    if (!rc) {
+        printf("Failed to introduce a domain\n");
+        return 1;
+    }
+
     return 0;
 }
 
-- 
2.43.0

On Wed Oct 1, 2025 at 9:51 AM CEST, Michal Orzel wrote:
> Current use of err() has the following issues:
> - without setting errno, on error it results in printing e.g.:
>  "init-dom0less: writing to xenstore: Success"
>  This is very misleading and difficult to deduct that there was a
>  failure.
> - does not propagate error codes to the caller.
> - skips "init_domain failed" message by exiting early.
>
> Replace err() with more informative messages propagating rc when
> possible.

Sounds good to me. Only suggestion I'd make is to also print relevant arguments
where needed, like...

>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  tools/helpers/init-dom0less.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> index a182dce56353..3dd2d74886eb 100644
> --- a/tools/helpers/init-dom0less.c
> +++ b/tools/helpers/init-dom0less.c
> @@ -288,24 +288,33 @@ static int init_domain(struct xs_handle *xsh,
>  
>          rc = xc_dom_gnttab_seed(xch, info->domid, true,
>                                  (xen_pfn_t)-1, xenstore_pfn, 0, 0);
> -        if (rc)
> -               err(1, "xc_dom_gnttab_seed");
> +        if (rc) {
> +            printf("Failed to seed gnttab entries\n");

... also printing the domid and the xenstore pfn here. Or...

> +            return rc;
> +        }
>      }
>  
>      libxl_uuid_generate(&uuid);
>      xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
>  
>      rc = gen_stub_json_config(info->domid, &uuid);
> -    if (rc)
> -        err(1, "gen_stub_json_config");
> +    if (rc) {
> +        printf("Failed to create stub json config\n");

... the domid and uuid here.

Similar suggestions for the other printfs.

> +        return rc;
> +    }
>  
>      rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
> -    if (rc)
> -        err(1, "writing to xenstore");
> +    if (rc) {
> +        printf("Failed to write to xenstore\n");
> +        return rc;
> +    }
>  
>      rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
> -    if (!rc)
> -        err(1, "xs_introduce_domain");
> +    if (!rc) {
> +        printf("Failed to introduce a domain\n");
> +        return 1;

nit: Maybe -EBUSY so it's -errno like the others?

> +    }
> +
>      return 0;
>  }
>  

Cheers,
Alejandro

On 01/10/2025 12:09, Alejandro Vallejo wrote:
> On Wed Oct 1, 2025 at 9:51 AM CEST, Michal Orzel wrote:
>> Current use of err() has the following issues:
>> - without setting errno, on error it results in printing e.g.:
>>  "init-dom0less: writing to xenstore: Success"
>>  This is very misleading and difficult to deduct that there was a
>>  failure.
>> - does not propagate error codes to the caller.
>> - skips "init_domain failed" message by exiting early.
>>
>> Replace err() with more informative messages propagating rc when
>> possible.
> 
> Sounds good to me. Only suggestion I'd make is to also print relevant arguments
> where needed, like...
> 
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  tools/helpers/init-dom0less.c | 25 +++++++++++++++++--------
>>  1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
>> index a182dce56353..3dd2d74886eb 100644
>> --- a/tools/helpers/init-dom0less.c
>> +++ b/tools/helpers/init-dom0less.c
>> @@ -288,24 +288,33 @@ static int init_domain(struct xs_handle *xsh,
>>  
>>          rc = xc_dom_gnttab_seed(xch, info->domid, true,
>>                                  (xen_pfn_t)-1, xenstore_pfn, 0, 0);
>> -        if (rc)
>> -               err(1, "xc_dom_gnttab_seed");
>> +        if (rc) {
>> +            printf("Failed to seed gnttab entries\n");
> 
> ... also printing the domid and the xenstore pfn here. Or...
Domid is printed from main() before everything else, so no need to print it
again. Not sure why we need to print pfn but I can add it here if it's a must.

> 
>> +            return rc;
>> +        }
>>      }
>>  
>>      libxl_uuid_generate(&uuid);
>>      xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
>>  
>>      rc = gen_stub_json_config(info->domid, &uuid);
>> -    if (rc)
>> -        err(1, "gen_stub_json_config");
>> +    if (rc) {
>> +        printf("Failed to create stub json config\n");
> 
> ... the domid and uuid here.
> 
> Similar suggestions for the other printfs.
> 
>> +        return rc;
>> +    }
>>  
>>      rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
>> -    if (rc)
>> -        err(1, "writing to xenstore");
>> +    if (rc) {
>> +        printf("Failed to write to xenstore\n");
>> +        return rc;
>> +    }
>>  
>>      rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
>> -    if (!rc)
>> -        err(1, "xs_introduce_domain");
>> +    if (!rc) {
>> +        printf("Failed to introduce a domain\n");
>> +        return 1;
> 
> nit: Maybe -EBUSY so it's -errno like the others?
There are other places in this script where we return 1.

~Michal