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