8:57 a.m.
On Tue, Aug 30, 2016 at 17:53:56 -0400, John Ferlan wrote:
On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> The reworked API is now called virCPUUpdate and it should change the
> provided CPU definition into a one which can be consumed by QEMU command
s/by/by the/
Fixed.
...
> /**
> - * cpuUpdate:
> + * virCPUUpdate:
In a way I'd expect a "virCPUUpdate" API to go in src/util/...
somewhere. Not a problem to keep it here, but I guess when I see vir
prefixed functions I think src/util/...
Not really, I think the vir prefix is just used when someone refactors
an old code. AFAIK the goal (not enforced in any way) was to eventually
use the prefix for all functions. Anyway, see virQEMUCaps* is good
example of such functions/types that are not in src/util.
...
> +virCPUUpdate(virArch arch,
> + virCPUDefPtr guest,
> + const virCPUDef *host)
Still not 100% clear whether if eventually (patch 40) it's possible to
enter here with 'host == NULL'... Keeping in mind from patch 26 that
virQEMUCapsInitHostCPUModel could have qemuCaps->cpuModel = NULL, thus
the virQEMUCapsGetHostModel could then return a NULL from
qemuProcessUpdateGuestCPU which calls this function. However, that
possibility is gated by whether virQEMUCapsIsCPUModeSupported succeeds
or not. Since you've added NULLSTR and host ? checks, I'm partially
assuming it's possible to get here with host == NULL. The intro comments
here don't help me determine that though.
Yes, it is possible to enter virCPUUpdate with host == NULL.
virQEMUCapsIsCPUModeSupported checks hostCPUModel only for
VIR_CPU_MODE_HOST_MODEL, but virCPUUpdate is called for all CPU
definitions. It may seem we do not call virCPUUpdate if hostCPUModel is
NULL since the code is still not in its final shape after this patch
(until patch 40 is applied).
I'm lost in the terminology between old/new that's described
in patch
40. If your belief is things are going to be OK, then I'm fine with
that, but I figured I'd at least point out what I saw and what got
confusing eventually. Hopefully it's understandable.
> {
> struct cpuArchDriver *driver;
>
> - VIR_DEBUG("guest=%p, host=%p", guest, host);
> + VIR_DEBUG("arch=%s, guest=%p mode=%s model=%s, host=%p model=%s",
> + virArchToString(arch), guest,
virCPUModeTypeToString(guest->mode),
> + NULLSTR(guest->model), host, NULLSTR(host ? host->model :
NULL));
The Coverity build throws up here... According to cpu.h, arg 3 is
NONNULL; however, as I pointed out above that could change.
So at the very least cpu.h needs to change to remove the NONNULL(3).
Yes, fixed.
...
> + if (!driver->update) {
> virReportError(VIR_ERR_NO_SUPPORT,
> - _("cannot update guest CPU data for %s
architecture"),
> + _("cannot update guest CPU for %s architecture"),
> virArchToString(host->arch));'
Should this just be 'arch' ? if not, if host == NULL, then this will core.
Oops, yes, I forgot to change this one when adding an explicit arch
parameter.
> return -1;
> }
>
> - return driver->update(guest, host);
> + if (driver->update(guest, host) < 0)
Seems as though all implemented ->update functions will return an error
if 'host' is NULL, so that's good I guess. Whether it's expected is
something you can answer!
It depends on the guest CPU definition, sometimes host == NULL is OK,
sometimes it is not. And the update functions detect whether they need
host CPU model for updating the CPU and report an error.
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 422818e..f4bb51d 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -87,8 +87,8 @@ typedef virCPUDefPtr
> unsigned int flags);
>
> typedef int
> -(*cpuArchUpdate) (virCPUDefPtr guest,
> - const virCPUDef *host);
> +(*virCPUArchUpdate)(virCPUDefPtr guest,
> + const virCPUDef *host);
Why the name change? I see upcoming patches also use the new
nomenclature, although I'll assume not all driver functions will change.
Is that the goal for the future?
I think I mentioned it somewhere in the cover letter or maybe I just
wanted to do that, but the goal is to completely rework all APIs
provided by the cpu driver. Some of them will just change their names to
virCPU*, some of them will be completely removed, and several new APIs
will be added. So the new name also serves as a way to distinguish
reworked/new APIs from those that still needs to be rewritten or
removed.
> @@ -182,9 +182,10 @@ cpuBaseline (virCPUDefPtr *cpus,
> ATTRIBUTE_NONNULL(1);
>
> int
> -cpuUpdate (virCPUDefPtr guest,
> +virCPUUpdate(virArch arch,
> + virCPUDefPtr guest,
> const virCPUDef *host)
> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
Remove the ATTRIBUTE_NONNULL(3) as noted above or even go back to the
Done.
Jirka