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

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.