From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47642) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XifpL-00021A-EM for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:39:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XifpG-0000HX-F4 for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:38:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58384) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XifpG-0000HT-5O for qemu-devel@nongnu.org; Mon, 27 Oct 2014 04:38:54 -0400 Message-ID: <544E0493.7060403@redhat.com> Date: Mon, 27 Oct 2014 09:38:43 +0100 From: Max Reitz 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> <544E03EB.7090207@kamp.de> In-Reply-To: <544E03EB.7090207@kamp.de> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit 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: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, benoit@irqsave.net, ronniesahlberg@gmail.com, armbru@redhat.com, stefanha@redhat.com On 2014-10-27 at 09:35, Peter Lieven wrote: > 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? Seems good enough. I'd just like a comment in that function why you're doing that (in order to limit both alignments and "normal" lengths properly). I'm counting on nobody noticing that INT_MAX may be something different than 2^x - 1. *g* Max > 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 > >