linux-kernel - RE: [PATCH v3] block: ublk: enable zoned storage support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BN8PR04MB6417B762EA748C0117D6EACCF124A@BN8PR04MB6417.namprd04.prod.outlook.com>
Date:   Wed, 28 Jun 2023 12:06:59 +0000
From:   Matias Bjørling <Matias.Bjorling@....com>
To:     "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>,
        Niklas Cassel <Niklas.Cassel@....com>
CC:     "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        Hans Holmberg <Hans.Holmberg@....com>,
        Minwoo Im <minwoo.im.dev@...il.com>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Jens Axboe <axboe@...nel.dk>, Ming Lei <ming.lei@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3] block: ublk: enable zoned storage support

> -----Original Message-----
> From: Andreas Hindborg (Samsung) <nmi@...aspace.dk>
> Sent: Wednesday, 28 June 2023 13.52
> To: Niklas Cassel <Niklas.Cassel@....com>
> Cc: linux-block@...r.kernel.org; Hans Holmberg <Hans.Holmberg@....com>;
> Matias Bjørling <Matias.Bjorling@....com>; Minwoo Im
> <minwoo.im.dev@...il.com>; Damien Le Moal
> <damien.lemoal@...nsource.wdc.com>; Jens Axboe <axboe@...nel.dk>;
> Ming Lei <ming.lei@...hat.com>; open list <linux-kernel@...r.kernel.org>
> Subject: Re: [PATCH v3] block: ublk: enable zoned storage support
> 
> 
> Niklas Cassel <Niklas.Cassel@....com> writes:
> 
> > On Thu, Mar 16, 2023 at 03:55:38PM +0100, Andreas Hindborg wrote:
> >> From: Andreas Hindborg <a.hindborg@...sung.com>
> >
> > Hello Andreas,
> >
> >
> > I think that this patch is starting to look very nice!
> 
> Thanks!
> 
> >
> >
> <snip>
> >> +
> >> +int ublk_report_zones(struct gendisk *disk, sector_t sector,
> >> +		      unsigned int nr_zones, report_zones_cb cb, void *data) {
> >> +	unsigned int done_zones = 0;
> >> +	struct ublk_device *ub = disk->private_data;
> >> +	unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors;
> >> +	unsigned int first_zone = sector >> ilog2(zone_size_sectors);
> >> +	struct blk_zone *buffer;
> >> +	size_t buffer_length;
> >> +	unsigned int max_zones_per_request;
> >
> > Nit: I would sort the variables differently.
> >
> > Perhaps:
> >> +	struct ublk_device *ub = disk->private_data;
> >> +	unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors;
> >> +	unsigned int first_zone = sector >> ilog2(zone_size_sectors);
> >> +	unsigned int done_zones = 0;
> >> +	unsigned int max_zones_per_request;
> >> +	struct blk_zone *buffer;
> >> +	size_t buffer_length;
> >
> 
> Can I ask what is the reasoning behind this? I think they way you propose looks
> better, but are there any rules one can follow for this?

There aren't many hard rules to my knowledge. One rule of thumb, which I use, is to group variables of the same data type. Another one, I personally prefer having the complex data structures first, followed by the simple data types at the end. However, it isn't that easy, as I usually also take the code flow of the function into account, such that the data structures that are used together are also declared together and follows the flow of the code (if it is getting too complex, its probably because the function needs to be split). Finally, it always helps keeping things as simple as possible. 

Other suggestions might be to look at existing code and get a feel for how other structures it. I've found Code Complete by Steve McConnell a classic on maintaining good code style. Obviously, each advice in the book should be weighted towards how its typically done in the kernel.

> 
> Best regards
> Andreas