Am 13.12.2011 04:41, schrieb lan,Tianyu: > On 一, 2011-12-12 at 19:15 +0800, Kevin Wolf wrote: >> Am 12.12.2011 11:58, schrieb Pekka Enberg: >>> On Mon, 12 Dec 2011, Kevin Wolf wrote: >>>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64 >>>>> >>>>> rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT); >>>>> if (rft_idx >= rft->rf_size) >>>>> - return NULL; >>>>> + return (void *)-ENOSPC; >>>> >>>> Is this allowed style in kvm-tool? :-/ >>> >>> It needs to use ERR_PTR() and related macros but otherwise I don't see a >>> big problem with it. >> >> Can you be sure that it never clashes with a valid allocation when you >> use this in userspace? >> >> But yes, at least using appropriate functions should be required. And >> this means that you can't only check for -ENOSPC, but you need to check >> for all possible error codes (IS_ERR_VALUE() I guess). > The qcow_read_refcount_block() is invoiked in the two places > qcow_get_refcount() and update_cluster_refcount() and will only return > NULL or -ENOSPC. > > In the qcow_get_refcount(), when qcow_read_refcount_block() returned > -ENOSPEC(no refcount block is available.), returning zero which means > the cluster is not in use and the refcount block can be grew in the > update_cluster_refcount(). > > In the update_cluster_refcount(), when qcow_read_refcount_block() > returned -ENOSPEC, it is necessary to grow the refcount blocks. > > So the ENOSPEC should be specially dealt with. > > Does this make sense? :) I'm not saying that your code won't work today, just that it's brittle. If someone adds a different error return code somewhere and doesn't check if all callers properly deal with error codes, it will break. Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
