| Message ID | 20211130091025.93495-1-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] diskpart_handler: use existing uuid if not explicitely set | expand |
Hi James, sorry if this caused any trouble for you and thanks for submitting a patch! > To ensure this doesn't result in a comparison of the partition table being marked as changed due to a different random > uuid getting generated Just to clarify: There is a scenario in which there is no partuuid set in sw-description and this results in a new partuuid whenever swupdate's disk partitioner runs (or in other words: secondpa_uuid is not null although partuuid is missing from sw-description)? Kind regards, Michael
On Tue, Nov 30, 2021 at 5:29 AM Michael Adler <michael.adler@siemens.com> wrote: > > Hi James, > > sorry if this caused any trouble for you and thanks for submitting a patch! > > > To ensure this doesn't result in a comparison of the partition table being marked as changed due to a different random > > uuid getting generated > > Just to clarify: There is a scenario in which there is no partuuid set in sw-description and this results in a new > partuuid whenever swupdate's disk partitioner runs (or in other words: secondpa_uuid is not null although partuuid is > missing from sw-description)? Correct, secondpa_uuid should AFAIU will never actually be null by the time you hit the comparison in is_diskpart_different(). From my understanding this is because diskpart_reload_table() is always called before is_diskpart_different(), which automatically fills in any missing values in the table, in the case of the uuid if not explicitly set it will generate a random one. > > Kind regards, > Michael > > -- > Michael Adler > > Siemens AG > T RDA IOT SES-DE > Otto-Hahn-Ring 6 > 81739 München, Deutschland > > Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322
Hi James, > /* > * Fill the new in-memory partition table from the partition list. > */ > - ret = diskpart_fill_table(cxt, tb, part, priv); > + ret = diskpart_fill_table(cxt, tb, oldtb, part, priv); How about passing in oldtb->parent instead? In diskpart_fill_table, you only access the `parent` field which you forward to diskpart_set_partition. > + } else { > + struct fdisk_partition *oldpart = fdisk_table_get_partition_by_partno(oldtb, part->partno); > + if (oldpart) { > + const char *uuid = fdisk_partition_get_uuid(oldpart); > + if (uuid) > + ret |= fdisk_partition_set_uuid(pa, uuid); > + } > + } How about adding a comment here explaining why this is necessary? The code is simple, but the reason why it's necessary maybe not so much (at least I have missed it when I submitted the patch).
On Wed, Dec 1, 2021 at 1:32 AM Michael Adler <michael.adler@siemens.com> wrote: > > Hi James, > > > /* > > * Fill the new in-memory partition table from the partition list. > > */ > > - ret = diskpart_fill_table(cxt, tb, part, priv); > > + ret = diskpart_fill_table(cxt, tb, oldtb, part, priv); > > How about passing in oldtb->parent instead? In diskpart_fill_table, you only access the `parent` field which you forward > to diskpart_set_partition. Well it's mostly for consistency with tb since we're passing in tb, and not tb->parent here. > > > + } else { > > + struct fdisk_partition *oldpart = fdisk_table_get_partition_by_partno(oldtb, part->partno); > > + if (oldpart) { > > + const char *uuid = fdisk_partition_get_uuid(oldpart); > > + if (uuid) > > + ret |= fdisk_partition_set_uuid(pa, uuid); > > + } > > + } > > How about adding a comment here explaining why this is necessary? The code is simple, but the reason why it's necessary > maybe not so much (at least I have missed it when I submitted the patch). True, I'll add one. > > -- > Michael Adler > > Siemens AG > T RDA IOT SES-DE > Otto-Hahn-Ring 6 > 81739 München, Deutschland > > Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322
diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c index a48f4d2..0e2affb 100644 --- a/handlers/diskpart_handler.c +++ b/handlers/diskpart_handler.c @@ -287,7 +287,8 @@ static int diskpart_get_partitions(struct fdisk_context *cxt, struct diskpart_ta static int diskpart_set_partition(struct fdisk_partition *pa, struct partition_data *part, unsigned long sector_size, - struct fdisk_parttype *parttype) + struct fdisk_parttype *parttype, + struct fdisk_table *oldtb) { int ret; @@ -312,8 +313,16 @@ static int diskpart_set_partition(struct fdisk_partition *pa, if (parttype) ret |= fdisk_partition_set_type(pa, parttype); - if (strlen(part->partuuid)) - ret |= fdisk_partition_set_uuid(pa, part->partuuid); + if (strlen(part->partuuid)) { + ret |= fdisk_partition_set_uuid(pa, part->partuuid); + } else { + struct fdisk_partition *oldpart = fdisk_table_get_partition_by_partno(oldtb, part->partno); + if (oldpart) { + const char *uuid = fdisk_partition_get_uuid(oldpart); + if (uuid) + ret |= fdisk_partition_set_uuid(pa, uuid); + } + } return ret; } @@ -496,7 +505,7 @@ static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table * } static int diskpart_fill_table(struct fdisk_context *cxt, struct diskpart_table *tb, - struct partition_data *part, struct hnd_priv priv) + struct diskpart_table *oldtb, struct partition_data *part, struct hnd_priv priv) { struct fdisk_parttype *parttype; struct fdisk_label *lb; @@ -527,7 +536,7 @@ static int diskpart_fill_table(struct fdisk_context *cxt, struct diskpart_table } else { parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16)); } - ret = diskpart_set_partition(newpa, part, sector_size, parttype); + ret = diskpart_set_partition(newpa, part, sector_size, parttype, oldtb->parent); if (ret) { WARN("I cannot set all partition's parameters"); } @@ -949,7 +958,7 @@ static int diskpart(struct img_type *img, /* * Fill the new in-memory partition table from the partition list. */ - ret = diskpart_fill_table(cxt, tb, part, priv); + ret = diskpart_fill_table(cxt, tb, oldtb, part, priv); if (ret) goto handler_exit;
When the ability to specifiy a partition UUID was added it also starting enforcing the uuid matches in is_diskpart_different, however in cases where the uuid is not set a new one will be randomly generated. To ensure this doesn't result in a comparison of the partition table being marked as changed due to a different random uuid getting generated, set the UUID based on the existing partition if it exists in cases where one is not specified in the sw-description. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- handlers/diskpart_handler.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)