[parted-devel] [PATCH 1/4] libparted/dasd: unify vtoc handling for cdl/ldl

[Brian C. Lane]

On Fri, Jul 08, 2016 at 10:04:06AM +0200, Hendrik Brueckner wrote:

Sorry it's taken so long to get to reviewing this. I have a few
questions inline with each patch.

> +/*
> + * The following structure is a merger of the cdl and ldl volume label.
> + * On an ldl disk there is no key information, so when reading an
> + * ldl label from disk, the data should be copied at the address of vollbl.
> + * On the other side, the field ldl_version is reserved in a cdl record
> + * and the field formatted_cyl exists only for ldl labels. So when
> + * reading a cdl label from disk, the formatted_cyl field will contain
> + * arbitrary data.

I think you mean formatted_blocks here, not formatted_cyl

> -        u_int64_t formatted_blocks;  /* valid when ldl_version >= "2" (in
> -                                        EBCDIC)                              */
> +	char res3[28];	        /* reserved                                  */
> +	char ldl_version;       /* version number, valid for ldl format      */
> +	unsigned long long formatted_blocks; /* valid when ldl_version >= f2 */

Is there any reason not to use u_int64_t for this? I think it's clearer
than unsigned long long.

> @@ -360,8 +359,8 @@ dasd_read (PedDisk* disk)
>  				* (long long) cms_ptr->disk_offset;
>  
>  		if (is_ldl)
> -		   if (strncmp(ldl_ptr->ldl_version,
> -			       vtoc_ebcdic_enc("2", str, 1), 1) >= 0)
> +			/* (ASCII) 0xf2 <===> "2"(in EBCDIC) */
> +		    if (ldl_ptr->ldl_version >= 0xf2)

Extra indentation here ^^

-- 
Brian C. Lane | Port Orchard, WA (PST8PDT)