oh，yeah. Thanks for your reply. It's a good lesson for me. :) I will update soon. On 二, 2011-12-13 at 16:57 +0800, Kevin Wolf wrote: > 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 Thanks Tianyu Lan -- 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
