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.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)