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 <mpriv...@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

Reply via email to