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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
