On Thu, May 31, 2018 at 09:45:04AM -0400, John Ferlan wrote:
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.
I feel the same way about most of those things. I have some unfinished patches
here and there for adding these to the CG so that we don't have that many
subjective discussions. Having said that I don't mind having the discussions.
I just don't like having some of the discussions multiple times =) I will learn
a new way if there is a clearly written consensus, it's just that I don't want
to do that just based on some reviews. Until it is written in stone^Wthe repo,
I'm not that convinced. But what is a great plus for me, here and now, is that
I understand your review more. I wasn't really sure whether that was optional
feedback or a requirement before pushing =) Thanks for the explanation. That's
why it is very mandatory to have a healthy communication.
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.
See:
- commit 8dc88aeed612aeea1ae0d249ec760938cecce5aa
- commit bf20251048534022efe21785f98949c772ce7a71
- function qemuDomainGetSCSIControllerModel
- Andrea's issues with default GIC version (many commits)
- commit f55eaccb0c570767d8245f57deae188255ee995e
and I'm sure there's more. Even ones that don't only differentiate on the
architecture.
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.
I'll see how the discussion goes with Pavel in the other thread, if someone
chimes in. This patch is in no rush since it's too close to release anyway, so
we'll see. I'm okay with setting it to whatever was the default during the first
start (getting the value from QEM) for example. Or maybe leaving the code
altogether (even though I must admit it's partially because with this mail
history I will be able to say "told you so" in case the day comes like 5 years
from now).
>>> +
>>> + 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.
Yeah, most of the stuff is kept in KiB, I keep it in Bytes. Reason?
Extensibility in the future just in case some other hypervisor adds similar
option. Also because I imagined that if I kept it in MiB someone would tell me
the opposite :)
>>> +
>>> + 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.
They are not doing the same thing. genericxml2xml is testing XML
formatting/parsing in domain_conf. qemuxml2xml is testing the driver-specific
postparse bits and qemuxml2argv is testing the command-line generation.
>>> + </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.
Yeah, we can do that. The only advantage, though, would be that we keep less
files around. The number of tests would be the same. And we can do that with
symlinks nowadays. So probably that's why nobody bothered.
[...]
>>> 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.
That's perfectly fine, I just wasn't ever sure whether I understand it corectly.
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.
I'm different in this, I guess. I think the review should be readable for all
people the same way, no matter whether you know the person, what they are used
to say or how they mean things. That way it's unambiguous. OTOH I'm not saying
that I'm doing everything the best way and that my opinions are the best, so
it's time for me to learn some new ways! ;)
John
[...]
Have a nice day,
Martin