Re: [PATCH parted] dasd: Fix ped_disk_new_fresh() to not read old data from disk (rh533808)

[Jim Meyering]

Jim Meyering wrote:
> Hans de Goede wrote:
>> dasd_write(), was reading the volume label from the disk (trough
>> fdasd_check_volume()) and later writing it back again, this is fine for
>> existing dasd labels, but when creating a fresh label, this would
>> also cause the old volume label to be re-used, and if the old label
>> was corrupt, it would cause fdasd_check_volume() and thus dasd_write()
>> to fail.
>>
>> * libparted/arch/linux.c (toplevel): include fdasd.h.
>> (init_dasd): Do BIODASDINFO ioctl, and store the dasd devno in
>> arch_specific.
>> (init_dasd): Remove dead (never reached) code.
>> * libparted/arch/linux.h (struct _LinuxSpecific): Add devno member.
>> * libparted/labels/dasd.c (DasdDiskSpecific): add vlabel member.
>> (dasd_alloc): Init DasdDiskSpecific.vlabel for fresh disks
>> (dasd_read): Store read vlabel in DasdDiskSpecific.vlabel.
>> (dasd_write): Write DasdDiskSpecific.vlabel instead of on disk vlabel.
>
> Thanks!
> I'll do a thorough review tomorrow,
> but in the mean time, would you please write a sentence about
> this in NEWS?
>
> Also, I want to be able to exercise this fix.
> Assuming I have an s390 VM, I suppose I could
> create a dasd partition table from a dd-created file
> full of zeros, corrupt part of it, and then what?
> Create another on top of that?  Is it the mklabel that fails?
> Can you give enough detail so I can write a reproducer?

Thanks.
This has taken more time than I expected.
First, the code didn't even compile with --enable-gcc-warnings on an s390.
There were dasd const-correctness problems as well as something that
at first looked serious (buffer overrun), but was really just poor code.

Then, once the code compiled there, I found that many tests were failing.
Most failures turned into passes once I fixed a bug in my recent GPT-
related changes.  The sole remaining test failure is due to a fundamental
design assumption in the dasd code: it doesn't work at all when the backing
"device" is a plain file, unlike all other partition types.

I'm in the process of evaluating whether we can drop that restriction,
and have a few change sets that take us down that road.  I'll write
more when/if it looks like the result will be worthwhile.  I'd really
like to be able to test dasd partition tables in normal files, but if
the required code changes are too numerous or too deep, I'll punt instead.