[libvirt] [PATCHv2 08/16] storage: get entire metadata chain in one call

[Eric Blake]

On 10/16/2012 08:43 AM, Laine Stump wrote:
> On 10/13/2012 06:00 PM, Eric Blake wrote:
>> Previously, no one was using virStorageFileGetMetadata, and for good
>> reason - it couldn't support root-squash NFS.  Change the signature
>> and make it useful to future patches, including enhancing the metadata
>> to recursively track the entire chain.
>>

>> +
>> +    if (VIR_CLOSE(fd) < 0)
>> +        virReportSystemError(errno, _("could not close file %s"), path);
> 
> If this isn't fatal to the operation, shouldn't it be a warning instead
> of an error? (And if it is fatal, it should free the remaining resources
> and return NULL)

Later in the series, I convert virDomainDiskDefForeachPath, which had an
argument that said whether to ignore open failures while recursing
through a chain.  That implies that there are some callers that care
about being able to find every file in a chain, while other callers want
to know as much as possible but don't care once they can't get further.
 I debated about how best to represent this in the new recursive data
structure, and ended up with: if meta->backingStoreIsFile is set but
meta->backingStoreMeta is clear, the recursion failed partway through,
and it is up to the caller to decide if an aborted recursion is fatal.

I also debated about whether I should have the helper function return an
int instead of a pointer; returning a pointer or NULL is a boolean
operation, and does not allow for as much flexibility.  Returning an int
would let me do things like returning 0 if recursion ended normally,
returning 1 if recursion ended because a file could not be opened, and
returning -1 if recursion ended due to OOM or other fatal problem.  If
you want me to revise along those lines I can, but for now, I think I'll
just change this to be a VIR_WARN instead of a virReportSystemError().

> 
>> +
>> +    if (ret->backingStoreIsFile) {
>> +        if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe)
>> +            ret->backingStoreFormat = VIR_STORAGE_FILE_RAW;
>> +        else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE)
>> +            ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
>> +        format = ret->backingStoreFormat;
> 
> Why are you bothering to set this, instead of just using
> ret->backingStoreFormat in the args to the following function call?
> (It's harmless, though, so no big deal)

I did that solely to fit within 80 columns.

> 
> ACK, assuming acceptable response to my above questions.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121016/9d37ec76/attachment-0001.sig>