Re: [PATCH 1/9] parted: fix EOF and ctrl-c handling

[Phillip Susi]

On 08/08/2013 08:45 PM, Brian C. Lane wrote:
> Commit messages need to follow the normal parted form. I'd also like to
> see tests as separate commits.

Can you be a bit more specific about the form?  As far as I know this is 
the usual form that Jim had me use.  He also stressed that new feature 
and test for it go hand in hand, and logically they are two parts of the 
same conceptual change, so it makes sense to keep them in the same commit.

> 4/9 - Nak. I have a more general patch for this -- not everything
>             inserts DMRAID in there.

Could you be a bit more specific?

> 5/9 - Shouldn't it use disk->dev->sector_size instead of hard-coded
>        512/1024? The logic seems a bit messy, using an if block instead
>        of the == 1 ? 512 : 1024 may clean it up a little. If it is
>        already set to 512 (or really ->sector_size) we don't need to walk
>        the other partitions. One place where a tab snuck in. Looks pretty
>        ok other than that.

I'm not sure what you mean about walking the other partitions.  I 
suppose it should use ->sector size.  I'll have to check the kernel 
sources again to see how they behave with non 512 byte sectors.

> 6/9 - Nak. I'm not sure if this is the right way to handle things.

Could you be more specific?

> 7/9 - Nak. I really don't like the giant pile of changes for a problem
>        that is only in the gpt code.

Do you have another suggestion?  I don't see another way of fixing the 
bug, and the constant re-reading of the partition table seems a 
senseless waste of time to begin with, so it kills two birds with one stone.

> 8/9 - Nak. It looks to me like removing some of these checks is going to
>        allow user (ui or library) checks that should fail on new data to
>        pass. I'm ok with letting us run with these errors so we can fix
>        them, but you shouldn't be able to enter bad numbers after the
>        warnings.

I thought that the ui did its own checks and stops you before it gets to 
the library but I'll take a look at that again.

> 9/9 - Nak. I need more details here. The reasoning in the original
>        commit seems sound to me, and I'd be more worried about busy
>        devices than the filesystem not matching (I'm not sure we should
>        even be in the fs detection business anymore).

The reasoning was sound, it was just based on a falsehood: the kernel 
does indeed still maintain two separate caches for the two devices, so 
when you write a fs superblock to the partition device then try to read 
it via the whole disk device, you don't see the superblock unless you 
flush the caches.  See:

http://lists.alioth.debian.org/pipermail/parted-devel/2012-December/004289.html

I never heard back from Hans about why he put that in, but removing it 
fixed the problem, and I couldn't find any attempt to resolve the cache 
aliasing problem in the code, or heard of anyone talking about it on 
linux-mm.