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

[Jim Meyering]

Jim Meyering wrote:
> 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've pushed your patch, along with the others mentioned above.
Here are two of them:


>From bd368af10871344e11bd9fd75554261fde08edf2 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 10 Nov 2009 12:19:26 +0100
Subject: [PATCH 1/5] maint: fix s390-specific const correctness problems

* libparted/labels/vtoc.c (vtoc_error, vtoc_ebcdic_enc):
(vtoc_ebcdic_dec): Declare parameters to be const, as required.
* libparted/labels/fdasd.c (fdasd_error): Likewise.
* include/parted/vtoc.h (vtoc_ebcdic_enc, vtoc_ebcdic_enc): Update
prototypes.
---
 include/parted/vtoc.h    |    6 ++----
 libparted/labels/fdasd.c |    2 +-
 libparted/labels/vtoc.c  |   10 +++-------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/parted/vtoc.h b/include/parted/vtoc.h
index 93ee162..613b706 100644
--- a/include/parted/vtoc.h
+++ b/include/parted/vtoc.h
@@ -207,10 +207,8 @@ struct __attribute__ ((packed)) format7_label {
        cchhb_t DS7PTRDS;       /* pointer to next FMT7 DSCB               */
 };

-char * vtoc_ebcdic_enc (char source[LINE_LENGTH], char target[LINE_LENGTH],
-                        int l);
-char * vtoc_ebcdic_dec (char source[LINE_LENGTH], char target[LINE_LENGTH],
-                        int l);
+char *vtoc_ebcdic_enc (char const *source, char *target, int l);
+char *vtoc_ebcdic_dec (char const *source, char *target, int l);
 void vtoc_set_extent (extent_t * ext, u_int8_t typeind, u_int8_t seqno,
                       cchh_t * lower, cchh_t * upper);
 void vtoc_set_cchh (cchh_t * addr, u_int16_t cc, u_int16_t hh);
diff --git a/libparted/labels/fdasd.c b/libparted/labels/fdasd.c
index cb9404e..b116a69 100644
--- a/libparted/labels/fdasd.c
+++ b/libparted/labels/fdasd.c
@@ -87,7 +87,7 @@ fdasd_cleanup (fdasd_anchor_t *anchor)
 }

 static void
-fdasd_error (fdasd_anchor_t *anc, enum fdasd_failure why, char * str)
+fdasd_error (fdasd_anchor_t *anc, enum fdasd_failure why, char const *str)
 {
        PDEBUG
        char error[2*LINE_LENGTH], *message = error;
diff --git a/libparted/labels/vtoc.c b/libparted/labels/vtoc.c
index 35b26e1..b54dc3f 100644
--- a/libparted/labels/vtoc.c
+++ b/libparted/labels/vtoc.c
@@ -153,7 +153,7 @@ enum failure {
 static char buffer[85];

 static void
-vtoc_error (enum failure why, char *s1, char *s2)
+vtoc_error (enum failure why, char const *s1, char const *s2)
 {
        PDEBUG
        char error[8192];
@@ -183,9 +183,7 @@ vtoc_error (enum failure why, char *s1, char *s2)
 }

 char *
-vtoc_ebcdic_enc (char source[LINE_LENGTH],
-                 char target[LINE_LENGTH],
-                 int l)
+vtoc_ebcdic_enc (char const *source, char *target, int l)
 {
        PDEBUG
        int i;
@@ -197,9 +195,7 @@ vtoc_ebcdic_enc (char source[LINE_LENGTH],
 }

 char *
-vtoc_ebcdic_dec (char source[LINE_LENGTH],
-                 char target[LINE_LENGTH],
-                 int l)
+vtoc_ebcdic_dec (char const *source, char *target, int l)
 {
        PDEBUG
        int i;
--
1.6.5.2.372.gc0502


>From 574a13671f8659f5ba4217a1748f6ccd893b2feb Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 10 Nov 2009 13:06:39 +0100
Subject: [PATCH 2/5] build: avoid s390-specific compilation failiure

* libparted/labels/vtoc.c (vtoc_volume_label_init): Don't use
strncpy to copy 84 bytes into a 4-byte field.  Instead, use
memcpy to copy "sizeof *vlabel" bytes into the "vlabel".
---
 libparted/labels/vtoc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libparted/labels/vtoc.c b/libparted/labels/vtoc.c
index b54dc3f..41fb22a 100644
--- a/libparted/labels/vtoc.c
+++ b/libparted/labels/vtoc.c
@@ -258,8 +258,8 @@ vtoc_volume_label_init (volume_label_t *vlabel)
 {
        PDEBUG
        sprintf(buffer, "%84s", " ");
-       vtoc_ebcdic_enc(buffer, buffer, 84);
-       strncpy(vlabel->volkey, buffer, 84);
+       vtoc_ebcdic_enc(buffer, buffer, sizeof *vlabel);
+       memcpy(vlabel, buffer, sizeof *vlabel);
 }

 /*
--
1.6.5.2.372.gc0502