Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

ignition: fix correct device path when randomizing UUID #100

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

tuan-hoang1
Copy link
Contributor

The correct path is /dev/disk/by-label/root.

Signed-off-by: Tuan Hoang [email protected]

The correct path is /dev/disk/by-label/root.

Signed-off-by: Tuan Hoang <[email protected]>
@tuan-hoang1
Copy link
Contributor Author

Confirmed working on s390x.

@ajeddeloh
Copy link
Contributor

They're both correct. partlabel is created using the partition label ans label is created using the filesystem label. Was partlabel not working on s390x?

@tuan-hoang1
Copy link
Contributor Author

This is the output I get from /dev/disk/by-* : https://tpaste.us/DLdo

And output from sgdisk in create_disk.sh: https://tpaste.us/BoZy

@ajeddeloh
Copy link
Contributor

I don't follow. Both should be symlinks created by udev that point to the same place.

@tuan-hoang1
Copy link
Contributor Author

tuan-hoang1 commented Aug 22, 2019

It smells like big endian issue to me, or wrong encoding. @sharkcz: wdyt ?

\200     : "space"
\346\274 : o
\346\210 : b
\347\210 : r
\347\220 : t

@ajeddeloh
Copy link
Contributor

This looks like maybe a systemd-udev bug or udev rules bug? We can switch to label since it doesn't matter which we use.

@ajeddeloh ajeddeloh merged commit 9330616 into coreos:master Aug 22, 2019
@dustymabe
Copy link
Member

They're both correct. partlabel is created using the partition label ans label is created using the filesystem label. Was partlabel not working on s390x?

still would be nice to know if there is a bug there in s390x - can someone else confirm? @sharkcz @jcajka

@sharkcz
Copy link

sharkcz commented Sep 9, 2019

I think partlabel doesn't exist for DASDs.

@dustymabe
Copy link
Member

interesting.. So using partlabel won't work for s390x?

I wonder if we need to change any other code. Looks like mantle uses it in a few places.

@arithx
Copy link
Contributor

arithx commented Sep 9, 2019

@dustymabe The only place that by-partlabel is used in mantle is on CL specific tests.

@dustymabe
Copy link
Member

+1 - thanks @arithx

@tuan-hoang1
Copy link
Contributor Author

tuan-hoang1 commented Sep 10, 2019

@sharkcz : regarding the decoding thing, I think it's an endian bug in gptfdisk.

sfdisk -d metal.disk
...
... name="\xe6\x88\x80\xe6\xbc\x80\xe6\xbc\x80\xe7\x90\x80"
... name="\xe7\x88\x80\xe6\xbc\x80\xe6\xbc\x80\xe7\x90\x80"
...
python -c "print(repr('\xe7\x88\x80\xe6\xbc\x80\xe6\xbc\x80\xe7\x90\x80'.decode('utf8')))"
u'\u7200\u6f00\u6f00\u7400'

Those must be reverse byte order of unicode string '0072 006f 006f 0074' of 'root'.

https://github.com/tuan-hoang1/gptfdisk/blob/master/gptcl.cc#L207

@cgwalters
Copy link
Member

https://github.com/tuan-hoang1/gptfdisk/blob/master/gptpart.cc#L201

OT: I see that kind of C code and am amazed that it doesn't have even more bugs. It's the kind of C code that if I maintained it I'd be rewriting in Rust...

@tuan-hoang1
Copy link
Contributor Author

tuan-hoang1 commented Sep 10, 2019

I think we just need to define USE_UTF16 in .spec file and use #include <unicode/ustream.h> instead. Checking ...

@tuan-hoang1
Copy link
Contributor Author

tuan-hoang1 commented Sep 10, 2019

My very narrow investigation says GPT savse partition name in little endian format (https://en.wikipedia.org/wiki/GUID_Partition_Table : "56 (0x38) 72 bytes Partition name (36 UTF-16LE code units) "). So if the underlying data is decided that way, we could either :

  1. Make the partition name field to be mixed endian format (how? change the spec? lol )
  2. Do the endian conversion when reading from disk (i.e. sfdisk -d metal.raw). But that would mean we have to fix udev and every downstream application that read those LE data without endian conversion.

so tl;dr GPT partition name field (i.e. name= output from sfdisk -d) is not usable on s390x atm. I think it is not a deal breaker because essential fields start=, size=, type=, uuid= do work though. At least we understand the problem now. And gptfdisk code (esp. SetName()) does its job correctly to my understanding.

Prashanth684 added a commit to Prashanth684/mantle that referenced this pull request Feb 26, 2020
…ignition mount disk test

Changed from by-partlabel to by-partuuid by assigning a random uuid. On s390x, the by-partlabel string
is incorrect because GPT partitioning uses little endian by default and s390x is big endian. Refer
to: coreos/ignition-dracut#100.
Prashanth684 added a commit to Prashanth684/mantle that referenced this pull request Feb 26, 2020
… for ignition mount disk test

Changed from by-partlabel to by-partuuid by assigning a random uuid. On s390x, the by-partlabel string
is incorrect because GPT partitioning uses little endian by default and s390x is big endian. Refer
to: coreos/ignition-dracut#100.
Prashanth684 added a commit to Prashanth684/mantle that referenced this pull request Feb 26, 2020
Changed from by-partlabel to by-partuuid by assigning a random uuid. On s390x, the by-partlabel string
is incorrect because GPT partitioning uses little endian by default and s390x is big endian. Refer
to: coreos/ignition-dracut#100.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants