Re: [PATCH] xl cpupool-list: add option to list domains

[Juergen Gross <juergen.gross@ts.fujitsu.com>]

On 10.03.2014 12:58, Ian Jackson wrote:
> Juergen Gross writes ("[Xen-devel] [PATCH] xl cpupool-list: add option to list domains"):
>> It is rather complicated to obtain the cpupool a domain lives in. Add an
>> option -d (or --domains) to list all domains running in a cpupool.
>
> Thanks for this.  I have some comments:
>
>>   List CPU pools on the host.
>>   If I<-c> is specified, B<xl> prints a list of CPUs used by I<cpu-pool>.
>> +If I<-d> is specified, B<xl> prints a list of domains in I<cpu-pool> instead
>> +of the domain count.
>> +I<-c> and I<-d> are mutually exclusive.
>
> Couldn't we come up with an (unambiguous) output syntax that made them
> nonexclusive ?

I have no idea how to print this information in a table without producing
unaligned columns.

It would be possible, however, to add something like "xl cpupool-list --long"
to avoid this problem completely. We wouldn't need the --domains option then.
At the same time your comment regarding the comma-separated list could be
addressed.

What do you think?


Juergen

>
>> @@ -6808,15 +6826,30 @@ int main_cpupoollist(int argc, char **argv)
>>                           n++;
>>                       }
>>                   if (!opt_cpus) {
>> -                    printf("%3d %9s       y       %4d", n,
>> -                           libxl_scheduler_to_string(poolinfo[p].sched),
>> -                           poolinfo[p].n_dom);
>> +                    printf("%3d %9s       y     ", n,
>> +                           libxl_scheduler_to_string(poolinfo[p].sched));
>> +                    if (opt_domains) {
>> +                        c = 0;
>> +                        for (n = 0; n < n_domains; n++) {
>> +                            if (poolinfo[p].poolid == dominfo[n].cpupool) {
>> +                                name = libxl_domid_to_name(ctx, dominfo[n].domid);
>
> This long source line needs wrapping.
>
>> +                                printf("%s%s", c ? ", " : "", name);
>
> I'm not a huge fan of this comma-separated list.  If the list were
> space separated it could be cut-and-pasted into
>    for f in dom1 dom2; do xl somethingorother; done
> etc.
>
> Also, I think domain names aren't guaranteed not to contain commas.
> So I think you need to quote and/or escape them somehow.  I suggest
> using "-quotes iff the domain name contains whitespace.
>
> (People who put " in their domain names can send patches to \-escape
> them or something.)
>
>> +                                free(name);
>> +                                c++;
>
> And if you are going to do this delimiter checking, calling the
> variable for "what delimiter to use" "c" is a bit odd.  Presumably you
> mean count, but it's actually used as a boolean.
>
>> +                            }
>> +                        }
>> +                    }
>> +                    else
>
> } and else should be on the same line.  (Also I'm not a huge fan of
> "} else" and "else {" but we have other examples in the tree...)
>
> Thanks,
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>


-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html