From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47230) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xifma-0007oX-MV for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:36:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XifmU-0007tj-Qu for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:36:08 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:37263 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XifmU-0007tV-H4 for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:36:02 -0400 Message-ID: <544E03EB.7090207@kamp.de> Date: Mon, 27 Oct 2014 09:35:55 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1414253919-3044-1-git-send-email-pl@kamp.de> <1414253919-3044-6-git-send-email-pl@kamp.de> <544E032F.40509@redhat.com> In-Reply-To: <544E032F.40509@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCHv5 5/6] block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, benoit@irqsave.net, ronniesahlberg@gmail.com, armbru@redhat.com, stefanha@redhat.com On 27.10.2014 09:32, Max Reitz wrote: > On 2014-10-25 at 18:18, Peter Lieven wrote: >> As Max pointed out there is a hidden cast from int64_t to int. >> So use the newly introduced nb_sectors_lun2qemu for all >> limits. >> >> Signed-off-by: Peter Lieven >> --- >> block/iscsi.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/block/iscsi.c b/block/iscsi.c >> index 1ae4add..85131b7 100644 >> --- a/block/iscsi.c >> +++ b/block/iscsi.c >> @@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) >> if (iscsilun->lbp.lbpu) { >> if (iscsilun->bl.max_unmap < 0xffffffff) { >> - bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, >> - iscsilun); >> + bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap, >> + iscsilun); >> } >> - bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >> - iscsilun); >> + bs->bl.discard_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >> + iscsilun); > > This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned INT_MAX. I think it should be capped to something like INT_MAX / 2 + 1 or just simply written out 0x40000000. Good point, I would cap all limits to the highest power of two and use INT_MAX / 2 + 1 directly in nb_sectors_lun2qemu? Peter > >> } >> if (iscsilun->bl.max_ws_len < 0xffffffff) { >> - bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, >> - iscsilun); >> + bs->bl.max_write_zeroes = nb_sectors_lun2qemu(iscsilun->bl.max_ws_len, >> + iscsilun); >> } >> if (iscsilun->lbp.lbpws) { >> - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >> - iscsilun); >> + bs->bl.write_zeroes_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >> + iscsilun); > > Same here. > >> } >> - bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, >> - iscsilun); >> + bs->bl.opt_transfer_length = nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len, >> + iscsilun); >> } >> /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in > > Max -- Mit freundlichen Grüßen Peter Lieven ........................................................... KAMP Netzwerkdienste GmbH Vestische Str. 89-91 | 46117 Oberhausen Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 pl@kamp.de | http://www.kamp.de Geschäftsführer: Heiner Lante | Michael Lante Amtsgericht Duisburg | HRB Nr. 12154 USt-Id-Nr.: DE 120607556 ...........................................................