Re: [libvirt] [PATCH v5 03/10] conf: introduce launch-security element in domain

On 04/02/2018 10:18 AM, Brijesh Singh wrote: Again - I see the R-by from Daniel, but I have more comments for the patches... Also since this patch is where we introduce the XML, the xml2xml changes that are in the last patch should move into here so that we keep everything together. We're probably getting beyond the point of me doing the movement, so a v6 will be needed. s/see/see the/ I'll be 4.3.0 now that 4.2.0 is released. Your example should provide some more realistic values. The above won't be necessary as long as you note required below, e.g. s/The/The required/ s/from domaincapabilities/from the domain capabilities/ s/The/The required/ s/reducation/reduction/ s/from domaincapabilities/from the domain capabilities/ There's some long lines above and below - while not a policy - for those using 80 character wide displays, we try to keep lines in the 70-75 char range. How about 4 unsigned byte I don't see this in the qemu policies: #define SEV_POLICY_NODBG 0x1 #define SEV_POLICY_NOKS 0x2 #define SEV_POLICY_ES 0x4 #define SEV_POLICY_NOSEND 0x8 #define SEV_POLICY_DOMAIN 0x10 #define SEV_POLICY_SEV 0x20 Sois there just one bit for the lower firmware thing or is this a word field with some value expected? So this is the QEMU default currently; however, that could change in the future. I think we could just not say what the default is. It'll make some changes a bit later on easier too. s/owners/owners base64 encoded/ s/SEV/the SEV/ and the last sentence wouldn't be needed any more... s/session/base64 encoded session/ s/in/in the/ and the last sentence wouldn't be needed any more... s/for/for the/ Hopefully hexuint will suffice over time... On the other hand, this patch uses virXPathULongHex in order to parse. Similar to patch 2 questions... Should this be "launchSecurity" "reducedPhysBits" "DHCert" Changing has fairly major implications, but I'd rather not have it baked in and then have someone throw a complaint and have to redo things. Since many people are off today, let's give them at least until tomorrow to provide feedback before spinning a v6... again 2 blank lines between functions Rather than making callers know... if (!def) return; Meaning this one won't be necessary 2 blank lines Two lines: virDomainSevDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt) Prefer 2 lines here too: char *tmp = NULL; char *type = NULL; probably should initialize = 0; Assuming 'sev' for domain_conf isn't "normal" - that is it's not future proof... This produces a libvirt failed for some reason type error message... So, as ugly as this will look, using switch/case logic: def->sectype = virDomainLaunchSecurityTypeFromString(type); switch((virDomainLaunchSecurity) def->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: default: virReportError(VIR_ERR_XML_ERROR, _("unsupported launch-security type '%s'"), type); goto error; } NB: I've *added* ->sectype to @def [see below] s/get/get launch-security/ > + > + if (virXPathUInt("string(./reduced-phys-bits)", ctxt, > + &def->reduced_phys_bits) < 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("failed to get reduced-phys-bits")); s/get/get launch-security/ > + > + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0)> + policy = 0x1; Hmmm... This one is optional which makes things a bit interesting. How do you know it was provided? Today the default could be 0x1, but sometime in the future if you decide upon a different default, then things get really complicated. Or for some other chip and/or hypervisor the default won't be 1. Also virXPathULongHex returns -1 or -2 w/ different meanings - if it's not there, You get a -1 when not provided and -2 when you have invalid value in field, which should be an error. Finally, ULongHex returns 64 bits and your field is 32 bits leading to possible overflow So, either you have to have a "policy_provided" boolean of some sort or I think preferably leave it as 0 (zero) indicating not provided and then when generating a command line check against 0 and provide the hypervisor dependent value on the command line *OR* just don't provided it an let the hypervisor choose it's default value because the value wasn't provided (that's a later patch though) Also see [1] below... So my suggestion is: if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 || policy > UINT_MAX) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid launch-security policy value")); goto error; } def->policy = policy; If -1 is returned, the def->policy = 0; otherwise, it's set to something. 2 blank lines 2 blank lines Two lines e.g. virDomainSevDefFormat(virBufferPtr buf, virDomainSevDefPtr sev) if (!sev) return; Since we shouldn't be assuming 'sev' at this point virBufferAsprintf(buf, "<launch-security type='%s'>\n", virDomainLaunchSecurityTypeToString(def->sectype)); This has been defined as a "0x" number, but you're printing in decimal format. Let's print in 0x value. [1] Also if you leave this a 0 (zero), then you can compare sev->policy if (sev->policy) virBufferAsprintf(buf, "<policy>0x%x</policy>\n", sev->policy); you may also want to do a "0x%04x" Use virBufferEscapeString Use virBufferEscapeString See above, no need for the if (def->sev) int sectype; /* enum virDomainLaunchSecurity */ John