Re: [Xen-devel] [PATCH 2/3] xl: convert vcpu related return codes to EXIT_[SUCCESS|FAILURE]

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2015-10-23 at 13:18 +0530, Harmandeep Kaur wrote:
> turning vcpu manipulation functions xl exit codes toward using the
> EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary
> numbers
> or libxl return codes.
> 
So, this patch is, technically, mostly fine. The observations on the
subject made when reviewing patch 1 applies here too, of course.

Something on the selection of functions the functions. The title says
the patch will address "vcpu related functions". However, there are a
bunch of parse_*() functions in the diff.

I appreciate that, for instance, parse_vcpu_affinity() can be
considered a vcpu related function, but that applies a lot less to
 parse_vnuma_config(), IMO. I'd therefore exclude the latter from this
patch.

Alternatively, get rid of both and do another patch specifically for
dealing with parse_*() items. There are not many of them that are
actual exit paths and/or contain calls to exit() (parse_config_data()
is big enough, and a bit more complex, that it may well deserve its own
patch, and you can leave it alone, just mention that in the changelog).

A few more comments below.

> @@ -5461,7 +5461,7 @@ static int vcpuset(uint32_t domid, const char*
> nr_vcpus, int check_host)
>  
>          rc = libxl_domain_info(ctx, &dominfo, domid);
>          if (rc)
> -            return 1;
> +            return EXIT_FAILURE;
>  
 if (libxl_domain_info(ctx, &dominfo, domid))
     return EXIT_FAILURE;

>          if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu)
> {
>              fprintf(stderr, "You are overcommmitting! You have %d
> physical" \
> @@ -5471,12 +5471,12 @@ static int vcpuset(uint32_t domid, const
> char* nr_vcpus, int check_host)
>          }
>          libxl_dominfo_dispose(&dominfo);
>          if (rc)
> -            return 1;
> +            return EXIT_FAILURE;
>      }
>      rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
>      if (rc) {
>          fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n",
> rc);
> -        return 1;
> +        return EXIT_FAILURE;
>
 if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus))
     return EXIT_FAILURE;

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel