Re: [dm-devel] [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks
> @@ -687,12 +688,8 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned > char *buf) >* Check zone size: only devices with a constant zone size (except >* an eventual last runt zone) that is a power of 2 are supported. >*/ > - zone_blocks = sd_zbc_check_zones(sdkp); > - ret = -EFBIG; > - if (zone_blocks != (u32)zone_blocks) > - goto err; > - ret = zone_blocks; > - if (ret < 0) > + ret = sd_zbc_check_zones(sdkp, _blocks); > + if (ret != 0) This coud be simplified to if (ret) or just keep the old if (ret < 0) which would still work. Otherwise looks fine: Reviewed-by: Christoph Hellwig -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks
Damien, > The unsigned 32 bits overflow check for the zone size value is already > done within sd_zbc_check_zones() with the test: > > } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { > > so there is no need to check again for an out of range value in > sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() > error return to -EFBIG instead of -ENODEV if the zone size is too large. > Change the return type of sd_zbc_check_zones() to an int for the error > code and return the zone size (zone_blocks) through a u32 pointer to > avoid overflowing the signed 32 return value. Acked-by: Martin K. Petersen -- Martin K. Petersen Oracle Linux Engineering -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks
On 2018/10/12 19:23, Hannes Reinecke wrote: > On 10/12/18 12:08 PM, Damien Le Moal wrote: >> The unsigned 32 bits overflow check for the zone size value is already >> done within sd_zbc_check_zones() with the test: >> >> } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { >> >> so there is no need to check again for an out of range value in >> sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() >> error return to -EFBIG instead of -ENODEV if the zone size is too large. >> Change the return type of sd_zbc_check_zones() to an int for the error >> code and return the zone size (zone_blocks) through a u32 pointer to >> avoid overflowing the signed 32 return value. >> >> Signed-off-by: Damien Le Moal >> --- >> drivers/scsi/sd_zbc.c | 19 --- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >> index ca73c46931c0..0678e1e108b0 100644 >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct >> scsi_disk *sdkp, >>* Returns the zone size in number of blocks upon success or an error code >>* upon failure. >>*/ >> -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) >> +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) >> { >> u64 zone_blocks = 0; >> sector_t max_lba, block = 0; > > Actually I thought to just change the 's32' to 'int', and not adding > another parameter; but anyway. Yes, I understood that. But since chunk_sectors is unsigned int, zone_blocks has to be too and so returning that through an int would be asking for troubles. If we ever have see a drive with a 2G LBA zone size that is :) I thought it was cleaner this way. > > Reviewed-by: Hannes Reinecke Thanks ! > > Hannes > -- Damien Le Moal Western Digital Research -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks
On 10/12/18 12:08 PM, Damien Le Moal wrote: The unsigned 32 bits overflow check for the zone size value is already done within sd_zbc_check_zones() with the test: } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { so there is no need to check again for an out of range value in sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() error return to -EFBIG instead of -ENODEV if the zone size is too large. Change the return type of sd_zbc_check_zones() to an int for the error code and return the zone size (zone_blocks) through a u32 pointer to avoid overflowing the signed 32 return value. Signed-off-by: Damien Le Moal --- drivers/scsi/sd_zbc.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index ca73c46931c0..0678e1e108b0 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp, * Returns the zone size in number of blocks upon success or an error code * upon failure. */ -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) { u64 zone_blocks = 0; sector_t max_lba, block = 0; Actually I thought to just change the 's32' to 'int', and not adding another parameter; but anyway. Reviewed-by: Hannes Reinecke Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel