8:45 a.m.
On 05/31/2018 03:24 AM, Martin Kletzander wrote:
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>
> This is way too sparse.
>
I can't really think of what to say here that is not already mentioned
in the
documentation or self-explanatory in the code. But maybe only I see
that as
self-explanatory. I'll write something up here.
The whole default thing is what really drew my attention back to the
top... It's where I expected to see an explanation why the default was
being set. I think that's something that should be described - you may
disagree, but that's why it's a review.
[...]
See, I know how to cut - I just forgot in my response to patch 4 ;-)
I'll try not to overcut as I've seen done in for reviews I've received
which means I have to find the context of the feedback (we all have our
favorite things to gripe about).
>> +
>> +static void
>> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
>> + const virDomainDef *def)
>> +{
>> + if (!def->tseg_size)
>
> Since it's not bool or pointer, prefer the tseg_size == 0
>
I don't know if you mean that as indicative or imperative. It is very
subjective and for such scenarios I would like to execute my right to
mention
that it is not part of Contributor Guidelines. I know it might seem
rude, but
this is one of the things that's very subjective and for such things I
like to
first reach a consensus before I change what I'm used to writing. I
hope you
understand that.
There's a lot of things not specifically listed in the CG that are
mentioned in many reviews or that are just now done because they have
been mentioned (2 blank lines between functions, formatting of function
headers, 1 variable per line, etc.). When I see this style and remember
to note it, I do... And I have see others mention it too. Many times
it's for enum comparisons.
In the long run, whatever. It's a style preference that denotes variable
usage for those reading code "in the future". We don't have a rule for
it, so go with your style if that's what you prefer.
FWIW: back in the dark ages when I first started doing this... Something
like the above for an integer, long, etc. value would be converted by
the compiler to checking *only* if the low bit was set/cleared (think
little endian) because that instruction was really quick. Try to find
that needle in a haystack for each even value that could be used ;-). I
got very used to the explicit comparison to 0 just to be 100% clear.
>> + return;
>> +
>> + virCommandAddArg(cmd, "-global");
>> +
[...]
>> + /* QEMU started defaulting to 16MiB after 2.9 */
>> + if (major > 2 || (major == 2 && minor > 9))
>> + def->tseg_size = 16 * 1024 * 1024;
>
> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>
TL;DR because not setting the default value when it is not explicitly
requested has already bitten us in the back multiple times.
Long story short this way we are safe. No matter what happens (QEMU
changing the default, the user changing the config somewhere else and
not expecting this to change, us wanting to change the default in the
future for some reason, migration to newer libvirt where who-knows-what
is checked there, etc.) it is way easier to keep the guest ABI stable.
It is visible what the value actually is and we're not hiding it
somewhere in the code of the hypervisor. I know we don't do that for
many things. I could've gone with the (often alibistic [1]) "the
default is left for hypervisor to decide", but since we have the
opportunity tomake things right (thanks to Laszlo's explanations), why
shouldn't we?
> This seems to me we are setting policy which based on history of many
> patches before is a no go. I'd say NAK to this, unless there is some
> convincing argument made in the commit message and followup responses to
> the series (or you can take Jan's R-By and ignore me - your call.
>
What patches are you referring to? I can add that to the commit
message, sure.
In my mind, for starters what I mentioned in my response to patch 3 for
formatdomain.html.in changes.
Beyond that, I don't really don't have the patience to go through
hundreds of patches of history in order to pick out ones that were
NACK'd or requested to be changed because the patch made some policy
decision or a let's set a default value type decision. I know they've
happened though.
TL;DR: I'm still of the opinion we don't set (or even save) a default.
I would think someone that failed to boot their guest because the memory
size or vCPU count was too large for smm/tseg would then be pointed to
this value which they would then optionally add and then "manage" going
forward.
I still don't see the point of setting a default if the default for QEMU
has been good enough up to now up to and including a certain memory size
of number of vCPU's, then how does us setting that default change
anything? Especially if it's the same value that QEMU uses. How do we
know that the value in the XML is one we provided or one the user
provided? Without providing a default and if QEMU changed the default
would a migration fail? If we set a default and then the migration
failed, where does the fickle finger of fate fall? Yes, Laszlo's
explanations are great, but they're lost to the mailing list and the
fading memories of those reading them if not captured in some way in
comments to code.
If there's a consensus to allow a default to be listed, then I'm not
going to even try to block. However, not mentioning it or calling it out
as part of the review would to me would be wrong. I believe by doing
this you're setting a precedent although I could be wrong as I don't
have the entire code base memorized.
>> +
>> + return 0;
>> +
>> + error:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Cannot parse QEMU machine type version
'%s'"),
>> + def->os.machine);
>> + return -1;
>> +}
>> +
>> +
>> +static int
>> +qemuDomainDefTsegPostParse(virDomainDefPtr def,
>
> While TSEG is what you're adjusting, this is more a 'features' or in
> particular SMM features post parse callback.
>
I'm getting the hint of a hidden suggestion from the sentence. Are you
suggesting I should rename the function or create few more layers like
this?
qemuDomainDefSMMFeaturesPostParse() {
qemuDomainDefTsegPostParse();
}
qemuDomainDefFeaturesPostParse() {
qemuDomainDefSMMFeaturesPostParse();
}
qemuDomainDefPostParse() {
...
qemuDomainDefFeaturesPostParse();
...
}
While the above is the progression I had in mind, I cannot make my mind
up if we should go through the insanity of all that or just go with the
qemuDomainDefSMMFeaturesPostParse which "allows" someone with a future
need to create a qemuDomainDefXXXFeaturesPostParse that is then either
directly called from qemuDomainDefPostParse or indirectly from a new
qemuDomainDefFeaturesPostParse.
This another one of those things not explicitly stated in the CG that I
know I've received feedback on in the past or have seen done by other
patches with respect to the naming of functions and/or creation of shell
methods to drill down into the path of the details. Function naming can
be so subjective...
>> + virQEMUCapsPtr qemuCaps)
>> +{
>> + if (def->features[VIR_DOMAIN_FEATURE_SMM] !=
>> VIR_TRISTATE_SWITCH_ON)
>> + return 0;
>> +
>> + if (!def->tseg_size)
>
> Similar... prefer tseg_size == 0
>
Again, are you directing me to do that or saying what version you like
more?
Pointing out a similar construct that IMO should be changed, but you
seem to dislike that way of comparison, so whatever. Yes, I know it's
functionally equivalent.
>> + return qemuDomainSetDefaultTsegSize(def, qemuCaps);
>> +
>> + if (!qemuDomainIsQ35(def)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("SMM TSEG is only supported with q35
>> machine type"));
>> + return -1;
>> + }
>> +
>> + if (!virQEMUCapsGet(qemuCaps,
>> QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Setting TSEG size is not supported with
>> this QEMU binary"));
>> + return -1;
>> + }
>> +
>> + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("SMM TSEG size must be divisible by 1
MiB"));
>> + return -1;
>> + }
>
> Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother.
>
> Curious that this differs from qemuDomainMemoryDeviceAlignSize which
> claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which
> returns 1024 unless it's PPC64 which returns 256MiB alignments.
It does. The extended TSEG explicitly allows 1 MiB increments. Not
only because of the naming (*_mbytes), but also because bigger
granularity would just be pointless probably. Also this is not going to
exist on PPC64, it only makes sense with q35.
I was partially commenting on why even bother - just force the rounding
and indicate that libvirt will do so - that I think has been done
numerous times before. Failing just because someone (tester) used
12345KiB or 1025KiB just seems pointless especially if we can and say we
will round if you don't know how to make things 1MiB aligned. A value
below 1 MiB would be useless - in fact it seems from the default
discussion that anything below 16 MiB would be useless, wouldn't it?
As for the rest - I went and checked a few other VIR_ROUND_UP consumers
and they were claiming the 1MiB rounding, but using 1024 instead of
1024*1024 like is done above which I just found "curious" - I didn't
bother to check the basis of the value being stored, but I think for
memory it is KB.
>> +
>> + return 0;
>> +}
>> +
>> +
[...]
>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml
>> @@ -0,0 +1,23 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>219100</memory>
>> + <currentMemory unit='KiB'>219100</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64'
machine='pc-q35-2.10'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <features>
>> + <smm state='on'>
>> + <tseg>48</tseg>
>
> The only difference here from genericxml2xmlindata/tseg.xml is that this
> one doesn't have "unit='MiB'"
>
Yes, I wanted to check for the correct parsing of both. Maybe I should
put 1 GiB in the other one so that I check that it parses the unit
properly? Actually I have KiB somewhere, so probably not.
Considering you commented recently on another series regarding whether
the code should use genericxml2xml - I'm just pointing out the IMO
excess of having both generic and qemu tests doing pretty much the same
thing... You can do what you want though - if both are left there,
that's fine.
>> + </smm>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + </devices>
>> +</domain>
[...]
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tseg.xml
>
> and yet another one similr to genericxml2xmlindata/tseg.xml - same name,
> different directory
>
Are you suggesting I create this as a symlink? Or that I should add
info to the commit message that this patch adds checks for formatting
the command-line (the functionality the patch is adding) and that the
genericxml2xmltest is checking the parsing even for developers who build
with QEMU driver disabled?
IMO: More duplication of the same test... I'm not the first person to
ever point out duplication of tests in commits. "Some day" maybe
qemuxml2xmltest will be able to source from genericxml2xml - then it's
up to someone to decide which test to use.
[...]
>> diff --git a/tests/qemuxml2argvtest.c
b/tests/qemuxml2argvtest.c
>> index 78454acb1a41..633dfaaee9a4 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2827,6 +2827,54 @@ mymain(void)
>>
[...]
>> + DO_TEST_PARSE_ERROR("tseg-i440fx",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI,
>> + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);
> Failure wrong machine type...
>
>> + DO_TEST_PARSE_ERROR("tseg-explicit-size",
>> + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
>> + QEMU_CAPS_DEVICE_IOH3420,
>> + QEMU_CAPS_ICH9_AHCI,
>> + QEMU_CAPS_MACHINE_SMM_OPT,
>> + QEMU_CAPS_VIRTIO_SCSI);
>
> Failure because TSEG_MBYTES not provided.
>
I'm sorry, don't take this personally, but sometimes I'm not sure
whether you are trying to convey some information or you are just adding
comments for yourself. If you are asking me to do something, then
please say so. If you want these messages to be there as comments, let
me know, I'll do that (although the only time that would be useful is if
someone makes a change and the tests start failing (by the tested code
not failing) and that person wants to know what was supposed to be the
error. In which case they can re-run them without the modifications.
Or if you are suggesting that it should be DO_TEST_FAILURE instead of
DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the
code doesn't throw an error during starting the domain, but right away
during parsing. And yes, that can be done because I'm not erroring out
for something that existed before, but for a new feature so it will
never be read from the disk for a domain that existed before this
series. Or maybe I ran the tests wrong and it is supposed to be
DO_TEST_FAILURE and I made a mistake somewhere else as well.
The reason for me not really being sure what you mean by that is that I
take all of what's in the previous paragraph as something obvious and
known by default for non-newbie libvirt developers (which you most
certainly are one since you have years of experience in this codebase),
so I'm probably just missing something. It very well might be a
language barrier, because I'm nowhere near understanding most of the
nuances in English language and various dialects thereof. So with no
hidden meaning between the lines, please enlighten me.
Nothing personal... and valid question/points. There's no hidden meaning
- sometimes it's just notes for myself that I forget to go back and
remove and other times it's just validation of what's being tested. Part
of my review processing is going from the test to the code and
considering the error being tested. Just like some people forget to
remove debugging logic from their patches before posting, I just forgot
to remove my review debugging notes - mea culpa and apologies for any
added stress/anxiety ;-). Since Jan had already provided an R-By for
the patch - I figured anything I'm providing becomes feedback for you to
pick and choose which you find particularly necessary to incorporate and
which to ignore.
After a few years of getting reviews I've learned the styles of various
reviewers and what feedback can be ignored, heeded, and challenged.
You've been working on libvirt longer than me, so I'm sure you have
certain filters set already too.
John
[...]