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 == 0I 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 == 0Again, 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.xmland yet another one similr to genericxml2xmlindata/tseg.xml - same name, different directoryAre 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
