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

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.