On 12/20/2012 05:01 PM, Jiri Denemark wrote:
> The API can be used to check if the model is on the supported models
> list, which needs to be done in several places.
> ---
> src/cpu/cpu.c | 17 +++++++++++++++++
> src/cpu/cpu.h | 5 +++++
> src/cpu/cpu_generic.c | 19 +++++--------------
> src/cpu/cpu_x86.c | 11 +----------
> 4 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index 53c4cc3..b41fb38 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -442,3 +442,20 @@ cpuHasFeature(virArch arch,
>
> return driver->hasFeature(data, feature);
> }
> +
> +bool
> +cpuModelIsAllowed(const char *model,
> + const char **models,
> + unsigned int nmodels)
Is size_t any better than unsigned int for nmodels?
> +{
> + unsigned int i;
> +
> + if (!models || !nmodels)
> + return true;
Should this case be false? Or more specifically, in the old code...
> +
> + for (i = 0; i < nmodels; i++) {
> + if (models[i] && STREQ(models[i], model))
> + return true;
> + }
> + return false;
> +}
> +++ b/src/cpu/cpu_generic.c
> @@ -123,20 +123,11 @@ genericBaseline(virCPUDefPtr *cpus,
> unsigned int count;
> unsigned int i, j;
>
> - if (models) {
!models skipped the error message, but allocated models with nmodels==0
errored out. You have a silent change in behavior by not erroring where
you used to; meanwhile, if you return false instead of true for both
branches of the ||, you would have a change in behavior of erroring
where you previously did not. I think that our current mix of
half-and-half erroring is not useful, but it's probably worth deciding
what we meant, and documenting in the commit message that the change in
error policy for (!models || !nmodels) is intentional.
> - bool found = false;
> - for (i = 0; i < nmodels; i++) {
> - if (STREQ(cpus[0]->model, models[i])) {
> - found = true;
> - break;
> - }
> - }
> - if (!found) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("CPU model '%s' is not support by hypervisor"),
> - cpus[0]->model);
> - goto error;
> - }
> + if (!cpuModelIsAllowed(cpus[0]->model, models, nmodels)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("CPU model %s is not supported by hypervisor"),
> + cpus[0]->model);
> + goto error;
> }
> +++ b/src/cpu/cpu_x86.c
> @@ -1325,16 +1325,7 @@ x86Decode(virCPUDefPtr cpu,
>
> candidate = map->models;
> while (candidate != NULL) {
> - bool allowed = (models == NULL);
> -
> - for (i = 0; i < nmodels; i++) {
> - if (models && models[i] && STREQ(models[i], candidate->name)) {
> - allowed = true;
> - break;
> - }
> - }
Hmm, here the behavior was different for (!models || !nmodels).
> -
> - if (!allowed) {
> + if (!cpuModelIsAllowed(candidate->name, models, nmodels)) {
> if (preferred && STREQ(candidate->name, preferred)) {
> if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list