bug#15299: [PATCH 1/9] parted: fix EOF and ctrl-c handling

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/11/2013 05:51 PM, Brian C. Lane wrote:
> I generally like them to be in a separate commit so that they can
> be applied by themselves so I can see it fail before applying the
> fix.

I suppose it does make it easier to verify they fail first, but on the
other hand, it clutters the history a bit.  I'm not sure permanently
cluttering the history is worth saving a few keystrokes to verify test
failure before applying, but I do see your point so if you feel this
is important I'll change it.

> See commit 4 from the patches I just sent, it preserves the UUID if
> it exists, but doesn't make up a new one (which may break things
> expecting a certain form).

Ahh yes, your version does look better.

> Let me see if I understand the problem. The issue is that the disk
> isn't right, the gpt backup is in the wrong location and currently
> we will sorta fix that by writing to the correct location if
> changes are made even if told to ignore it.

There are two related, but distinct errors.  One problem is that the
apparent size of the disk does not agree with what the GPT header
says.  The other is the location of the backup table.  If a disk
grows, then the size will be wrong, but the backup location will be
correct, relative to the old size, and incorrect relative to the new
size of the disk.

Because parted checks for the backup location first, and the size
second, after growing a disk, it would complain about the backup
location, and then also complain about the size.  It didn't make much
sense to have to fix both and the first message is a bit confusing, so
now it will not complain about the backup location since it does agree
with the previous size of the disk, and if you choose to fix the size,
the backup will then be moved to the correct location as part of the
size fix.

If you chose not to fix the location ( whether it is wrong entirely or
only because of a change in size ), parted would go ahead and write it
to the correct location anyhow, without zeroing the previous copy.
This is clearly wrong.

> I'd lean towards not allowing any operations at all until the
> location is fixed. If it isn't at the end it is in the wrong place
> and we really have no way to tell why it isn't right.

If the apparent disk size is wrong and the user chooses to let it be (
maybe another OS can't see the full disk size and they want to
maintain compatibility with it ), then the backup should also be left
where it is, at least if it is in the correct location from the
perspective of the other OS.  As long as the backup lies outside of
the usable portion of the disk, not being at the end ( as we see it )
should not cause any harm, and complaining that it isn't at the end as
we see it, after they have already said to accept the other OS's view
of where the end of the disk is, is counter productive.

In the event that the backup is at neither the current or previous end
of the disk, I lean towards allowing the user to ignore that and
continue unless we have a compelling reason not to.

> Unrelated to that, I also notice that the gpt-head-move.py script
> isn't taking into account endianness. I just ran into a similar
> problem with the gpt-header-munge script. You can't just
> pack/unpack from the header without converting from little-endian.

I saw that in your patch set today, and yes, since I adapted from
gpt-head-move.py, it probably needs the same fix.

> I don't have any better ideas yet. I'm just worried all those
> changes are going to break something subtle.

That's why we have a regression test suite ;)

I'm ok with tabling it for the next release, then apply it after and
give it some more testing time.

> Ends up I just had to do something similar to get filesystem
> probing working all the time. See patch 18 in my set. What I did
> was keep the kernel 2.6 check and skip the sync on the individual
> partitions. I think Hans' point about udev, etc. churn while doing
> this is a good one. parted only operates on offsets inside the
> parent device anyway so I think it is better to only sync that.

I don't think that is enough.  Flushing the disk device will clear out
any stale data in that cache and force it to be re-read from disk when
we probe the fs type, but if the partition cache still has dirty write
buffers, the disk is still out of date so re-reading it will still
return stale data.

As far as udev churn goes, I have another patch that is now in Debian
and Ubuntu that I still need to rebase onto upstream parted that
refactors the BLKPG code to avoid removing and recreating unmodified
partitions.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSMTGqAAoJEJrBOlT6nu75m7IIALmSTGaMIwwUxzqIgHsHH2Gj
aVGeHOvGOFUFTuaSfi8eVnKxV933CkaoDtvMTNpJFXYnziAKUUxV+lB0mYCHMBUo
muiVu0cSC+w4AIlbQBf5XhK1sZgiKSpJrgGkrjWeesVeQRVxvbBpo+opai5cYxEv
WQDOYju62D9utEzfZdbvaAC/oNlKwLXlekTFwJuYGgwJum07zbox64iK7CqODBUv
fablJSIFAYHYjnL9Qz/MnfFkhFxOrQDS+M8QrZZL/FQmTPVnrKTl4mYW0WfORZrn
cYfq0DEtaN2XodqzhsV26Z+zsIjrZ0iEt1x1laI912sVbYqAyqe5GSd928O8HHY=
=SsuT
-----END PGP SIGNATURE-----