From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=36399 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1ON4wv-0003Kn-Iq
	for qemu-devel@nongnu.org; Fri, 11 Jun 2010 10:11:13 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <hare@suse.de>) id 1ON4wr-0002aq-L0
	for qemu-devel@nongnu.org; Fri, 11 Jun 2010 10:11:06 -0400
Received: from cantor2.suse.de ([195.135.220.15]:55879 helo=mx2.suse.de)
	by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from <hare@suse.de>)
	id 1ON4wr-0002a4-CW
	for qemu-devel@nongnu.org; Fri, 11 Jun 2010 10:11:05 -0400
Message-ID: <4C1243F5.90103@suse.de>
Date: Fri, 11 Jun 2010 16:11:01 +0200
From: Hannes Reinecke <hare@suse.de>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] megasas: Update to version 1.01
References: <20100608141506.DC0A52AC5B@ochil.suse.de>
	<AANLkTimKsaKGtTlWRICzqZdndRL5SFAH7alvRyN1vnFm@mail.gmail.com>
In-Reply-To: <AANLkTimKsaKGtTlWRICzqZdndRL5SFAH7alvRyN1vnFm@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: qemu-devel@nongnu.org, "Nicholas A.Bellinger" <nab@linux-iscsi.org>

Blue Swirl wrote:
> On Tue, Jun 8, 2010 at 2:15 PM, Hannes Reinecke <hare@suse.de> wrote:
>> This patch updates the megasas HBA emulation to version 1.01.
>> It fixes the following issues:
>>
>> - Remove hand-crafted inquiry command
>> - Remove bounce-buffer for direct commands
>> - Implements qdev properties to set 'max_sge', 'max_cmds'.
>> - Implement JBOD mode
>> - Improve direct command handling
>> - Minor cleanups
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
[ .. ]
>> +static uint64_t megasas_gen_sas_addr(unsigned long id)
>> +{
>> +    uint64_t addr;
>> +
>> +    addr =3D ((uint64_t)0x5001a4a << 36);
>=20
> With 0x5001a4aULL the cast could be avoided.
>=20
>> +    addr |=3D ((uint64_t)id & 0xfffffffff);
>=20
> This cast could be avoided by making id uint64_t.
>=20
Ok. Fixed.

[ .. ]
>> +    memcpy(info->product_name,"MegaRAID SAS 8708EM2", 20);
>> +    sprintf(info->serial_number,"QEMU%08lx",(unsigned long)s & 0xFFFF=
FFFF);
>=20
> Please use snprintf(), OpenBSD linker issues warnings for all uses of s=
printf().
>=20
>> +    sprintf(info->package_version,"%s-QEMU", QEMU_VERSION);
>> +    strcpy(info->image_component[0].name, "APP");
>=20
> Same problem with strcpy(), please use pstrcpy(), snprintf() or memcpy(=
).
>=20
Ah. Ok, Fixed.

>> -    return offset;
>> +    if (info->vpd_page83[3] =3D=3D 0) {
>> +       req =3D scsi_req_get(sdev, (uint32_t) -1, lun);
>> +       if (!req)
>=20
> This would be against CODING_STYLE but nobody seems to care.
>=20
... And quite some other places, notably the tabs vs. spaces issue.
Fixed now.

[ .. ]
>> +
>> +struct dcmd_cmd_tbl_t {
>=20
> static const?
>=20
If you feel like it, okay.

[ .. ]
>> +    DPRINTF("Using %d sges, %d cmds, %s mode\n",
>> +           s->fw_sge, s->fw_cmds, s->is_jbod?"jbod":"raid");
>=20
> Please add spaces around '?' and ':'.
>=20
Ok, fixed.

Thanks for the feedback. I'll be including the fixes with
the next patchset.

Cheers,

Hannes
--=20
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg
GF: Markus Rex, HRB 16746 (AG N=C3=BCrnberg)