From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45257) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEYrt-0006Mb-Br for qemu-devel@nongnu.org; Tue, 05 Aug 2014 03:09:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XEYro-0008AQ-Dj for qemu-devel@nongnu.org; Tue, 05 Aug 2014 03:09:09 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:7735) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEYrn-00089S-Ln for qemu-devel@nongnu.org; Tue, 05 Aug 2014 03:09:04 -0400 Message-ID: <53E082D5.2040908@huawei.com> Date: Tue, 5 Aug 2014 15:08:05 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1407140744-11948-1-git-send-email-zhang.zhanghailiang@huawei.com> <1407140744-11948-5-git-send-email-zhang.zhanghailiang@huawei.com> <20140804122625.GC15491@redhat.com> <53E03AC9.2070806@huawei.com> <499586037.10574482.1407221456854.JavaMail.zimbra@redhat.com> In-Reply-To: <499586037.10574482.1407221456854.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Levente Kurusa Cc: kwolf@redhat.com, alex@alex.org.uk, "Michael S. Tsirkin" , luonengjun@huawei.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, peter huangpeng On 2014/8/5 14:50, Levente Kurusa wrote: > ----- Original Message ----- >> Hi Michael, >> Thanks for your review of this patch! >> >>> On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote: >>>> The function fstat() may fail, so check its return value. >>>> >>>> Signed-off-by: zhanghailiang >>>> --- >>>> hw/misc/ivshmem.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >>>> index 768e528..2667e9f 100644 >>>> --- a/hw/misc/ivshmem.c >>>> +++ b/hw/misc/ivshmem.c >>>> @@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) { >>>> >>>> struct stat buf; >>>> >>>> - fstat(fd,&buf); >>>> + if (fstat(fd,&buf)< 0) { >>>> + fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno)); >>>> + return -1; >>>> + } >>>> >>> >>> That's a confusing error message: >>> 1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #. >> I will add this in next version of the patch. >>> 2. Tell the user what action was taken, e.g. IVSHMEM failed to start. >>> >>>> if (s->ivshmem_size> buf.st_size) { >>>> fprintf(stderr, >>>> -- >> I have check the places of calling this function, And found that, if >> this function return -1, qemu will call exit(-1). One of the callers is >> ivshmem_read(), the purpose of check_shm_size() is to forbid guest to >> map more memory than the object has allocated. So here is it suitable to >> return -1 if fstat() failed? Or just give a warning message and return 0? >> what's your opinion? Thanks. > > Yes, please return -1. All the errors fstat(2) can return in this scenario > will be fatal to ivshmem. > > Exiting is probably not the best way to go, but I'm working on a fix for > that at the moment, and for now, let's just exit, like all the other error > paths do. > > Thanks, > Levente Kurusa > OK,thanks! Best regards, zhanghailiang