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@redhat.com>
---
src/qemu/qemu_domain.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 88 insertions(+), 2 deletions(-)
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) {
+ return 0;
+ } else if (rc < 0) {
+ return ret;
+ }
+
+ if (VIR_ALLOC(prd) < 0)
+ goto cleanup;
+
+ 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;
+
+ virBufferAddLit(buf, "<prd>\n");
+ virBufferAdjustIndent(buf, 2);
+ virBufferAsprintf(buf, "<alias>%s</alias>\n", prd->alias);
+ virBufferAsprintf(buf, "<path>%s</path>\n", prd->path);
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</prd>\n");
+ return 0;
+}
+
+
+static int
+qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
+ virStorageSourcePtr src)
+{
+ if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
+ return -1;
+
+ if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
+ return -1;
+
+ if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
+ return -1;
+
+ return 0;
+}
+
+
+static int
+qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
+ virBufferPtr buf)
+{
+ if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
+ return -1;
+
+ if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
+ return -1;
+
+ return 0;
+}
+
+
virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
.alloc = qemuDomainObjPrivateAlloc,
.free = qemuDomainObjPrivateFree,
@@ -2548,8 +2634,8 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
.chrSourceNew = qemuDomainChrSourcePrivateNew,
.parse = qemuDomainObjPrivateXMLParse,
.format = qemuDomainObjPrivateXMLFormat,
- .storageParse = virStorageSourcePrivateDataParseRelPath,
- .storageFormat = virStorageSourcePrivateDataFormatRelPath,
+ .storageParse = qemuStorageSourcePrivateDataParse,
+ .storageFormat = qemuStorageSourcePrivateDataFormat,
};
--
2.16.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
> 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.
> +
> + 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.
John
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</prd>\n");
> + return 0;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
> + virStorageSourcePtr src)
> +{
> + if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> + return -1;
> +
> + if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
> + return -1;
> +
> + if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
> + virBufferPtr buf)
> +{
> + if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
> + return -1;
> +
> + if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
> .alloc = qemuDomainObjPrivateAlloc,
> .free = qemuDomainObjPrivateFree,
> @@ -2548,8 +2634,8 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
> .chrSourceNew = qemuDomainChrSourcePrivateNew,
> .parse = qemuDomainObjPrivateXMLParse,
> .format = qemuDomainObjPrivateXMLFormat,
> - .storageParse = virStorageSourcePrivateDataParseRelPath,
> - .storageFormat = virStorageSourcePrivateDataFormatRelPath,
> + .storageParse = qemuStorageSourcePrivateDataParse,
> + .storageFormat = qemuStorageSourcePrivateDataFormat,
> };
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/06/2018 12:31 PM, Michal Privoznik wrote:
> 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@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.
>
There's something subtle that I'm missing with this code... maybe it's
just not fully comprehending the two modes/paths that are being added.
Are we making things too complicated.
>>
>>> +
>>> + 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.
>
IDC either - to some degree it's the author's choice... Trying to stay
as close as possible to other elements. You may be "impacted" w/r/t
Peter's patches changing the status file parse/format tests.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.