Re: [libvirt] [PATCH v2 1/5] domain: Fix unknown flags diagnosis in virDomainGetXMLDesc

On 2/14/19 4:29 PM, Eric Blake wrote: Since this is limited to the GetXMLDesc processing wouldn't this limit the exposure in such a way that some new flag expecting some default action would not return expected or "new" results on the newer libvirt vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that list, the get wouldn't fail because it doesn't know the flag, but it also wouldn't return any XML related to the new flag, right? Secondary to that knowing this would require someone to read this specific commit message to garnish at least a small understanding of the issue. Perhaps some comments in domain_conf.h near the new VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand those without understanding the impact. Similarly, near the flags definition either a similar noteworthy comment or a comment to go read the domain_conf.h comment. There's a quick comment added before virDomainDefFormatConvertXMLFlags in this patch that could be expandable instead. Doesn't mean someone will read and/or understand it, but at least leaves a trail. There is one extra I'd question, qemuDomainDefFormatBufInternal that perhaps could add the virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU). Chasing current callers seems to eventually ensure no more than those flags are used. That may mean an extra check for a couple of paths, but it does ensure others don't need to check. Here's my lookup/tree of callers and what could be passed originally. * qemuDomainDefFormatBuf * qemuMigrationCookieXMLFormat * Uses INACTIVE | SECURE | MIGRATABLE * qemuDomainDefFormatXMLInternal * qemuDomainDefFormatXML * qemuDomainDefCopy * qemuDomainDefCheckABIStability * qemuDomainCheckABIStability * Each uses SECURE | MIGRATABLE (via COPY_FLAGS) * qemuDomainSaveImageUpdateDef * Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE * qemuMigrationSrcRun * Uses SECURE | MIGRATABLE * qemuDomainSaveImageGetXMLDesc * qemuDomainManagedSaveGetXMLDesc * Each checks virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); * qemuDomainSaveImageDefineXML * Uses INACTIVE | SECURE | MIGRATABLE * qemuConnectDomainXMLFromNative * Checks virCheckFlags(0, NULL); * qemuMigrationDstPrepareAny * Uses SECURE | MIGRATABLE * qemuProcessStartHook * qemuProcessStop * qemuProcessAttach * qemuProcessReconnect * Each passes 0 * qemuDomainFormatXML * qemuDomainCheckABIStability * Same as above using COPY_FLAGS * qemuDomainGetXMLDesc * Newly repatched (and why I added UPDATE_CPU flag check for the virCheckFlags in my qemuDomainDefFormatBufInternal query). * qemuMigrationSrcPerformPeer2Peer2 * Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE * qemuDomainDefFormatLive * Will pass only SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) and possibly INACTIVE or MIGRATABLE BTW: For the two callers that pass all 3 maybe it'd be good to change those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently: * qemuMigrationCookieXMLFormat * qemuDomainSaveImageDefineXML What's here so far is fine. The qemuDomainDefFormatBufInternal processing is something that perhaps is "extra protection" considering all the paths that could get there. I'll let you decide whether it needs to be part of this, not part of this, or another patch. Beyond that, I assume you can add the correct attribution/comments... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John FWIW: The following were my notes during review - which I think covers the history that I found... Whether any of it is useful or not is up to you. Commit 461e0f1a2 (0.9.4) is where you first added code to virDomainDefFormat in order to check (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_UPDATE_CPU). Commit 20135c70 (0.9.4) introduces virDomainDefFormatInternal and DUMPXML_FLAGS to mean the same thing as well as adding VIR_DOMAIN_XML_INTERNAL_STATUS to the virCheckFlags so that virDomainObjFormat will work properly. It adds the verify macro to ensure no overlap. Commit 07f413699 (0.9.4) adds VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET to the list of flags that virDomainDefFormatInternal cares about. The verify gets adjusted as well. Commit d84b3626 (0.9.7) adds VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES to the list of flags that virDomainDefFormatInternal cares about. The verify gets adjusted as well. Commit c01ba1a48 (0.9.10) adds VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM and VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT, but does not update the verify macro. Commit 28f8dfdcc (1.0.0) adds VIR_DOMAIN_XML_MIGRATABLE to the DUMPXML_FLAGS list (includes virDomainObjFormat caller too). There is no need to change the verify. Commit e31b5cf39 (1.1.0) adds VIR_DOMAIN_XML_INTERNAL_BASEDATE to the list of flags that virDomainDefFormatInternal cares about. The verify is updated; however, still missing the ALLOW_ROM and ALLOW_BOOT. Commit b8efa6f2e (1.2.5 reverts the VIR_DOMAIN_XML_INTERNAL_BASEDATE from the virDomainDefFormatInternal list of flags. Commit b62d67da3 (1.2.5) adds VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST to the list of flags that virDomainDefFormatInternal cares about. The verify macro gets adjusted, but still not with ALLOW_ROM or ALLOW_BOOT. Commit 79f4c4e69 (1.2.8) moves where the the verify macro closer to where the internal flags are defined and adds ALLOW_ROM and ALLOW_BOOT. (a task that would require a couple of patches in today's world). Commit 37588b25 (1.2.8) adds VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE to the list of flags, but doesn't change virDomainDefFormatInternal since all it cares about is <disk> processing and adds that flag to the list of flags processed in virDomainDiskDefSourceParse. Commit 0ecd6851 (1.2.12) as you note comes along and alters things. It too would probably require more steps in today's world ;-). Commit 43a90eb7 (3.8.0) drops VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU