[parted-devel] [PATCH v2] libparted: Fixed bug in initializing FBA devices

[Brian C. Lane]

On Thu, Oct 31, 2013 at 03:48:29PM +0530, Nageswara R Sastry wrote:
> Subject: [PATCH v2] libparted: Fixed bug in initializing FBA devices
> 
> From: Nageswara R Sastry <rnsastry at linux.vnet.ibm.com>
> 
> Fixed Block Access (FBA) DASDs are mainframe-specific disk devices
> that are layed out as a sequence of 512-byte sectors. In contrast
> to ECKD DASDs, these disks do not require formatting and resemble
> the LBA layout of non-mainframe disks. Despite this resemblance,
> the Linux kernel applies special handling during partition detection
> for FBA DASDs, resulting in a single, immutable partition being
> reported.
> While actual FBA DASD hardware is no longer available, the z/VM
> hypervisor can simulate FBA DASD disks, backed by either ECKD or
> SCSI devices.
> 
> * libparted/labels/dasd.c (dasd_alloc): For FBA devices corrected the
> initilization and calling dasd_read in the case of FBA devices to
> set the implicit partition.
> 
> 
> Signed-off-by: Nageswara R Sastry <rnsastry at linux.vnet.ibm.com>
> ---
>  libparted/labels/dasd.c |   23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> --- a/libparted/labels/dasd.c
> +++ b/libparted/labels/dasd.c
> @@ -150,9 +150,19 @@ dasd_alloc (const PedDevice* dev)
>  		return NULL;
>  	}
> 
> -	/* CDL format, newer */
> -	disk_specific->format_type = 2;
> -	disk_specific->label_block = 2;
> +	/* Correct assignment is required to pass the correct values
> +	   according to the disk type. For FBA disk values are '1'
> +           and for CDL formatted disk values are '2'*/
> +	struct fdasd_anchor anchor;
> +	fdasd_initialize_anchor(&anchor);
> +	fdasd_get_geometry(disk->dev, &anchor, arch_specific->fd);
> +	if (anchor.FBA_layout == 1) {
> +		disk_specific->format_type = 1;
> +		disk_specific->label_block = 1;
> +	}else{
> +		disk_specific->label_block = 2;
> +		disk_specific->format_type = 2;
> +	}
> 
>  	/* Setup volume label (for fresh disks) */
>  	snprintf(volser, sizeof(volser), "0X%04X", arch_specific->devno);
> @@ -163,6 +173,13 @@ dasd_alloc (const PedDevice* dev)
>  	vtoc_set_cchhb(&disk_specific->vlabel.vtoc,
>  		       VTOC_START_CC, VTOC_START_HH, 0x01);
> 
> +	/* Calling dasd_read is required for FBA disks to set
> +	   the implicit partition */
> +	if (anchor.FBA_layout == 1) {
> +		if (!dasd_read(disk))
> +			dasd_free(disk);

There should be a return NULL after the dasd_free, inside brackets of course :)


> +	}
> +
>  	return disk;
>  }

Other than that, this looks ok to me.

Phillip, does this take are of your concerns about the DASD patchset?
I've been carrying the other 5 patches in Fedora for a while now, not
that they get much exposure there, but there is a s390 secondary arch
project.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)