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>
> ---
> 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.
Michal