Allow the user to specify the partition type #328

squimrel · 2017-06-26T22:07:46Z

Not sure if I like this implementation since theoretically the partition type can always be guessed correctly except if the partition type should be extended. (Note that doing so would alter existing behavior and therefore break applications that rely on the current behavior.)
Deriving "extended" from the filesystem partition seems legit and is already done by udisks.

Note that on my end the test_fill_with_primary_partitions fails after adding the second partition due to this error:

Error waiting for partition to appear: Timed out waiting for object

I've got no clue why that happens. Help wanted.

Implement feature requested in #318.

Allow the user to specify the "partition-type" option in
"CreatePartition" to explicitly specify whether the partition should be
a primary, extended or logical partition.

vpodzime

Looks good to me. Let's see what our CI thinks about this. The test failure is likely to be cause by some udev-related issues. We have been seeing something similar recently.

vpodzime · 2017-06-27T10:06:20Z

data/org.freedesktop.UDisks2.xml

@@ -1406,7 +1406,7 @@
        @size: The desired size of the partition, in bytes.
        @type: The type of partition to create (cf. the #org.freedesktop.UDisks2.Partition:Type property) or blank to use the default for the partition table type and OS.
        @name: The name for the new partition or blank if the partition table do not support names.
-        @options: Options (currently unused except for <link linkend="udisks-std-options">standard options</link>).
+        @options: Options - known options (in addition to <link linkend="udisks-std-options">standard options</link>) includes <parameter>partition-type</parameter> (of type 's').


Instead of "includes"? That's what I thought but everywhere else even if there's only one option it also says "includes".

Either this is a consistency thing everyone went with or the writers all somehow read: "includes parameter partition-type" even though the tag is obviously not read.

known options...include... makes sense to me. Don't really care about consistency here. :)

vpodzime · 2017-06-27T10:08:32Z

data/org.freedesktop.UDisks2.xml

@@ -1439,7 +1443,7 @@
        @size: The desired size of the partition, in bytes.
        @type: The type of partition to create (cf. the #org.freedesktop.UDisks2.Partition:Type property) or blank to use the default for the partition table type and OS.
        @name: The name for the new partition or blank if the partition table do not support names.
-        @options: Options (currently unused except for <link linkend="udisks-std-options">standard options</link>).
+        @options: Options - known options (in addition to <link linkend="udisks-std-options">standard options</link>) includes <parameter>partition-type</parameter> (of type 's').


same here with include

vpodzime · 2017-06-27T10:10:36Z

src/tests/dbus-tests/test_60_partitioning.py

@@ -95,7 +95,7 @@ def test_create_mbr_partition(self):
        sys_num = int(self.read_file('%s/partition' % part_syspath))
        dbus_num.assertEqual(sys_num)

-    def test_create_extended_partition(self):
+    def create_extended_partition(self, ext_options, log_options, fstype=''):


It's not fstype, it is the partition type (see the docs for details)

vpodzime · 2017-06-27T10:13:38Z

Jenkins, this is ok to test.

vpodzime · 2017-06-27T12:14:34Z

The test failure is likely to be cause by some udev-related issues.

I can confirm I'm seeing very weird issues even without this patch when trying to create 3 partitions on a single disk. The extended tests in this PR probably just it.

squimrel · 2017-06-27T12:45:43Z

So you say I should remove that test? Honestly it's not very good either since it would probably pass without this patch.

vpodzime · 2017-06-27T12:47:17Z

So you say I should remove that test? Honestly it's not very good either since it would probably pass without this patch.

Could be a good idea for now. We need to deal with the issues when creating partitions in general and it wouldn't make much sense to add tests that would always fail at this point.

vpodzime · 2017-06-27T13:56:39Z

@squimrel, please take a look at #329. We may need to merge that first to give your changes a chance to pass the existing and newly added tests.

squimrel · 2017-06-27T15:27:42Z

@vpodzime All tests fail with #329 but you already know that since the CI told you. Not sure what exactly I should look at.

vpodzime · 2017-06-28T06:30:02Z

@vpodzime All tests fail with #329 but you already know that since the CI told you. Not sure what exactly I should look at.

I meant the issues it fixes. It obviously needs more work. :)

vpodzime · 2017-06-28T13:02:34Z

I meant the issues it fixes. It obviously needs more work. :)

Done. Once merged, we'll see how that affects your newly added tests from here, @squimrel

squimrel · 2017-06-28T13:21:14Z

@vpodzime Ohh wow in fact this fixes the test I just removed! Well done.

vpodzime · 2017-06-29T07:03:23Z

@vpodzime Ohh wow in fact this fixes the test I just removed! Well done.

Thanks. :) Let's see if it really makes things work better. Please resolve the conflict that was introduced by me extending the tests a bit.

Implement feature requested in #318. Allow the user to specify the "partition-type" option in "CreatePartition" to explicitly specify whether the partition should be a primary, extended or logical partition.

vpodzime approved these changes Jun 27, 2017

View reviewed changes

vpodzime added this to the udisks 2.7.1 milestone Jun 27, 2017

Allow the user to specify the partition type

0680231

Implement feature requested in #318. Allow the user to specify the "partition-type" option in "CreatePartition" to explicitly specify whether the partition should be a primary, extended or logical partition.

vpodzime merged commit 6c422f0 into storaged-project:master Jun 30, 2017

vpodzime mentioned this pull request Jul 10, 2017

Create primary partition as last partition on dos label #318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the user to specify the partition type #328

Allow the user to specify the partition type #328

squimrel commented Jun 26, 2017 •

edited

Loading

vpodzime left a comment

vpodzime Jun 27, 2017

squimrel Jun 27, 2017 •

edited

Loading

vpodzime Jun 27, 2017

vpodzime Jun 27, 2017

vpodzime Jun 27, 2017

vpodzime commented Jun 27, 2017

vpodzime commented Jun 27, 2017

squimrel commented Jun 27, 2017

vpodzime commented Jun 27, 2017

vpodzime commented Jun 27, 2017 •

edited

Loading

squimrel commented Jun 27, 2017 •

edited

Loading

vpodzime commented Jun 28, 2017

vpodzime commented Jun 28, 2017

squimrel commented Jun 28, 2017

vpodzime commented Jun 29, 2017

Allow the user to specify the partition type #328

Allow the user to specify the partition type #328

Conversation

squimrel commented Jun 26, 2017 • edited Loading

vpodzime left a comment

Choose a reason for hiding this comment

vpodzime Jun 27, 2017

Choose a reason for hiding this comment

squimrel Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

vpodzime Jun 27, 2017

Choose a reason for hiding this comment

vpodzime Jun 27, 2017

Choose a reason for hiding this comment

vpodzime Jun 27, 2017

Choose a reason for hiding this comment

vpodzime commented Jun 27, 2017

vpodzime commented Jun 27, 2017

squimrel commented Jun 27, 2017

vpodzime commented Jun 27, 2017

vpodzime commented Jun 27, 2017 • edited Loading

squimrel commented Jun 27, 2017 • edited Loading

vpodzime commented Jun 28, 2017

vpodzime commented Jun 28, 2017

squimrel commented Jun 28, 2017

vpodzime commented Jun 29, 2017

squimrel commented Jun 26, 2017 •

edited

Loading

squimrel Jun 27, 2017 •

edited

Loading

vpodzime commented Jun 27, 2017 •

edited

Loading

squimrel commented Jun 27, 2017 •

edited

Loading