From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Wolf Subject: Re: [RFC PATCH] kvm tools, qcow: Add support for growing refcount blocks Date: Tue, 13 Dec 2011 09:57:56 +0100 Message-ID: <4EE71394.6070906@redhat.com> References: <1323655410-32000-1-git-send-email-tianyu.lan@intel.com> <4EE5CFA0.5000203@redhat.com> <4EE5E24D.20300@redhat.com> <1323747673.2585.96.camel@lantianyu-ws> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Pekka Enberg , "kvm@vger.kernel.org" , "asias.hejun@gmail.com" , "levinsasha928@gmail.com" , "prasadjoshi124@gmail.com" To: "lan,Tianyu" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:65145 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752803Ab1LMIyw (ORCPT ); Tue, 13 Dec 2011 03:54:52 -0500 In-Reply-To: <1323747673.2585.96.camel@lantianyu-ws> Sender: kvm-owner@vger.kernel.org List-ID: Am 13.12.2011 04:41, schrieb lan,Tianyu: > On =E4=B8=80, 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_rea= d_refcount_block(struct qcow *q, u64 >>>>> >>>>> rft_idx =3D clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_= BLOCK_SHIFT); >>>>> if (rft_idx >=3D 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=20 >>> big problem with it. >> >> Can you be sure that it never clashes with a valid allocation when y= ou >> use this in userspace? >> >> But yes, at least using appropriate functions should be required. An= d >> this means that you can't only check for -ENOSPC, but you need to ch= eck >> 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 retur= n > NULL or -ENOSPC. >=20 > 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(). =20 >=20 > In the update_cluster_refcount(), when qcow_read_refcount_block() > returned -ENOSPEC, it is necessary to grow the refcount blocks. >=20 > So the ENOSPEC should be specially dealt with. =20 >=20 > 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