From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35896) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYcyf-0008KL-00 for qemu-devel@nongnu.org; Mon, 29 Sep 2014 11:35:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYcyY-0002vH-Eh for qemu-devel@nongnu.org; Mon, 29 Sep 2014 11:35:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12648) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYcyY-0002uY-6W for qemu-devel@nongnu.org; Mon, 29 Sep 2014 11:34:58 -0400 Date: Mon, 29 Sep 2014 17:34:45 +0200 From: Kevin Wolf Message-ID: <20140929153445.GI3910@noname.str.redhat.com> References: <1410891148-28849-1-git-send-email-armbru@redhat.com> <1410891148-28849-19-git-send-email-armbru@redhat.com> <20140929122429.GF3910@noname.str.redhat.com> <87fvfalgpo.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87fvfalgpo.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3 18/23] blockdev: Fix blockdev-add not to create IDE drive (0, 0) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: benoit.canet@nodalink.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 29.09.2014 um 15:05 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) > >> } > >> > >> /* Set legacy DriveInfo fields */ > >> - dinfo = blk_legacy_dinfo(blk); > >> + dinfo = g_malloc0(sizeof(*dinfo)); > >> dinfo->enable_auto_del = true; > >> dinfo->opts = all_opts; > >> - > >> dinfo->cyls = cyls; > >> dinfo->heads = heads; > >> dinfo->secs = secs; > >> dinfo->trans = translation; > >> - > >> dinfo->type = type; > >> dinfo->bus = bus_id; > >> dinfo->unit = unit_id; > >> dinfo->devaddr = devaddr; > >> - > >> dinfo->serial = g_strdup(serial); > >> + blk_set_legacy_dinfo(blk, dinfo); > > > > You don't like the grouping? > > No, because the comment appears as if it applied only to the first > group. > > This is how this spot looks at the end of the series: > > /* Set legacy DriveInfo fields */ > dinfo = g_malloc0(sizeof(*dinfo)); > dinfo->opts = all_opts; > dinfo->cyls = cyls; > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > dinfo->type = type; > dinfo->bus = bus_id; > dinfo->unit = unit_id; > dinfo->devaddr = devaddr; > dinfo->serial = g_strdup(serial); > blk_set_legacy_dinfo(blk, dinfo); > > Could do this instead: > > dinfo = g_malloc0(sizeof(*dinfo)); > dinfo->opts = all_opts; > > dinfo->cyls = cyls; > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > > dinfo->type = type; > dinfo->bus = bus_id; > dinfo->unit = unit_id; > dinfo->devaddr = devaddr; > dinfo->serial = g_strdup(serial); > > blk_set_legacy_dinfo(blk, dinfo); > > The comment is next to useless anyway. Got a preference? The reason why it's there is that I like to use comments to give "headlines" to the blocks of code. In drive_new(), I can only read the comments without looking at the code and understand what functionality is implemented at a high level. So for me the "headline" is valid until the next one appears (except that this is the last one) and it's good as it is today. Your taste differs, as it seems. If I have to choose between your two alternatives, I'll reluctantly vote for removing the empty lines, because making it part of the "Actual block device init" block by removing the comment makes even less sense. Kevin