From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHxXy-0002Z8-5E for qemu-devel@nongnu.org; Thu, 14 Aug 2014 12:06:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XHxXs-0001Hd-03 for qemu-devel@nongnu.org; Thu, 14 Aug 2014 12:06:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHxXr-0001HY-HD for qemu-devel@nongnu.org; Thu, 14 Aug 2014 12:06:31 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7EG6U9L014557 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 14 Aug 2014 12:06:31 -0400 Message-ID: <53ECDE81.7070401@redhat.com> Date: Thu, 14 Aug 2014 12:06:25 -0400 From: John Snow MIME-Version: 1.0 References: <1407950470-26183-1-git-send-email-jsnow@redhat.com> <87ha1fmto6.fsf@blackfin.pond.sub.org> In-Reply-To: <87ha1fmto6.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] ide: Add resize callback to ide/core List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 08/14/2014 03:12 AM, Markus Armbruster wrote: > > I'd prefer if (s->drive_kind == IDE_CFATA) ... else ..., because it > avoids the double negative. OK. This is how cmd_identify delegates. For matters of style I usually try to defer to nearby code. > > Your code duplicates the parts of the functions initializing > s->identify_data dealing with nb_sectors. These are: > > * ide_identify() for IDE_HD > > Writes nb_sectors to slots 60..61 and 100..103. Matches your code. > > * ide_atapi_identify() for IDE_CD > > Doesn't use it at all. Your code clobbers slots 60..61 and 100.103. > Oops. > ide_resize_cb does not get called for ATAPI drives, so I think this is not relevant. I tried to note this in a comment. See ide_init_drive: if (kind == IDE_CD) { bdrv_set_dev_ops(bs, &ide_cd_block_ops, s); ... } else { ... bdrv_set_dev_ops(bs, &ide_hd_block_ops, s); } > * ide_cfata_identify() for IDE_CFATA > > Writes nb_sectors to slots 7..8 and and 60..61. Your code only writes > the former. ...Sorry. I appreciate you taking your time to review these patches. I will submit a V3. > > I guess code duplication is tolerable, because the format of > s->identify_data is unlikely to change. Comments linking the copies > together wouldn't hurt, though. > > If we don't want the duplication, we need to factor the length code out > of the identify functions, so we can use it in both places. > The length and complexity didn't feel like it needed to be factored out into two new functions, and I liked the idea of keeping the writes to the buffer sequential inside the initialization functions, because it made it easier for me to read along with the spec that way. Factoring it out seemed more of a fruitless hassle than anything. Thanks, --John