[libvirt] [PATCHv2 11/16] storage: make it easier to find file within chain

[Laine Stump]

On 10/16/2012 12:06 PM, Eric Blake wrote:
> On 10/16/2012 09:55 AM, Laine Stump wrote:
>> On 10/13/2012 06:00 PM, Eric Blake wrote:
>>> In order to temporarily label files read/write during a commit
>>> operation, we need to crawl the backing chain and find the absolute
>>> file name that needs labeling in the first place, as well as the
>>> name of the file that owns the backing file.
>>>
>>> +    *parent = NULL;
>>> +    if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) :
>>> +        !chain->backingStore) {
>> Hmm. First time I've ever seen ?: used inside an if statement (since for
>> me the entire purpose of ?: is to eliminate the need for an if). Maybe
>> this would be more quickly parseable as (the admittedly longer):
>>
>>      if ((name & (STREQ(start, name) || virFileLinkPointsTo(start,
>> name))) ||
>>          (!name && !chain->backingStore)) {
>>
>> Not mandatory though.
> I've done it before in libvirt, but yeah, it is kind of rare to see.  I
> think I'll leave it, if only because the long form is so much longer.
>
>>> +        if (meta)
>>> +            *meta = chain;
>>> +        return start;
>> Don't you need to make sure start is an absolute path?
> start was passed in by the caller, and should already be absolute
> (although maybe not canonical), since domain XML always wants an
> absolute name in the <source> element of each <disk>.  Also, this
> function is easier to use if I guarantee that I either return a pointer
> to somewhere within the chain, or to the actual start pointer given by
> the user (patch 16/16 relies on pointer equality rather than STREQ,
> because of this guarantee) - that is, the pointer I return must NOT be
> freed, because it is merely an alias to something that the user passed
> in.  If I canonicalize start, then I would also have to strdup() all
> other names, and require the user to free the result of this function.
>
>>> +        } else if (owner->backingStoreIsFile) {
>>> +            char *abs = absolutePathFromBaseFile(*parent, name);
>>> +            if (abs && STREQ(abs, owner->backingStore)) {
>>> +                VIR_FREE(abs);
>>> +                break;
>>> +            }
>>> +            VIR_FREE(abs);
>>> +        }
>>> +        *parent = owner->backingStore;
>>> +        owner = owner->backingMeta;
>>> +    }
>>> +    if (!owner)
>>> +        goto error;
>>> +    if (meta)
>>> +        *meta = owner->backingMeta;
>>> +    return owner->backingStore;
>> and I'm recalling from the previous patch that owner->backingStore is
>> already stored in its canonical form, right?
> Correct, which is why I got away with STREQ(abs, owner->backingStore) in
> order to exit the loop.
>> ACK as-is if start doesn't need to be canonicalized (i.e. if I didn't
>> understand :-), otherwise ACK with that change.
> I think it is more important to return start as-is, even if it is not
> absolute, than to canonicalize in that corner case; if anything, that
> may mean a slight tweak to my documentation.
>

Okay. You've obviously thought about the implications, which was the
most important reason for my mentioning it.