Re: [libvirt] [PATCH v2 05/12] qemu: Store pr runtime data in status XML

On 03/02/2018 01:50 PM, John Ferlan wrote: > > > On 02/21/2018 01:11 PM, Michal Privoznik wrote: >> Now that we generate pr-manager alias and socket path store them >> in status XML so that they are preserved across daemon restarts. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_domain.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 88 insertions(+), 2 deletions(-) >> > > Something that dawned on my this morning (sorry ;-)) - the ->alias could > easily be generated at reconnect time too. Sure, but then we can end up in similar situation like with private paths for domain. I mean, we did not use to store them in status XML so now, whenever we are parsing one we have to see if one is stored there or generate the old one. Luckily there were just two possible options. Just imagine if there were three. We'd have no idea which one to generate. Long story short, I really prefer to store whatever might change in the status XML. > >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index dffc4c30e..45205fd03 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -2540,6 +2540,92 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, >> } >> >> >> +static int >> +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt, >> + virStorageSourcePtr src) >> +{ >> + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); >> + qemuDomainDiskPRDPtr prd = NULL; >> + int rc; >> + int ret = -1; >> + >> + if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) == 0) { > ^^ > Extra space above > >> + return 0; >> + } else if (rc < 0) { >> + return ret; >> + } >> + >> + if (VIR_ALLOC(prd) < 0) >> + goto cleanup; > > return ret works too since prd == NULL > >> + >> + if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) || >> + !(prd->path = virXPathString("string(./prd/path)", ctxt))) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> + _("malformed <prd/>")); >> + goto cleanup; >> + } >> + >> + VIR_STEAL_PTR(srcPriv->prd, prd); >> + ret = 0; >> + cleanup: >> + qemuDomainDiskPRDFree(prd); >> + return ret; >> +} >> + >> + >> +static int >> +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src, >> + virBufferPtr buf) >> +{ >> + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); >> + qemuDomainDiskPRDPtr prd; >> + >> + if (!srcPriv || !srcPriv->prd) >> + return 0; >> + >> + prd = srcPriv->prd; > > Does saving the information really "matter" in whatever case it is that > uses a 'static' alias and path? IOW: Should we save some sort of > boolean to indicate PR was desired and then if path is also provided, we > can use that path; otherwise, use the 'static' path when we try to > reconnect the socket. I don't think we need that. This information is stored in the disk source XML: @managed == yes/no. Because of patch 02/12 we are guaranteed it will not change on migration/restore. Also, I don't see any value in having an managed pr-helper and making it unmanaged all of a sudden. Or vice versa. I expect users to use @managed='yes' prevalently.

> >> + >> + virBufferAddLit(buf, "<prd>\n"); >> + virBufferAdjustIndent(buf, 2); >> + virBufferAsprintf(buf, "<alias>%s</alias>\n", prd->alias); >> + virBufferAsprintf(buf, "<path>%s</path>\n", prd->path); > > alias and path could be attributes of prd too rather than elements on > their own, but that's just your implementation detail... IDC, but > figured I'd note it anyway. Yeah. Unless something is clear yes/no value (like @managed/@enabled) I tend to put knobs like these into separate elements as it is more extendable should we need it in the future IMO. But I don't have much strong opinion on that either.