Message ID | 20210518032002.45742-1-james.hilliard1@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [1/1] diskpart: refactor partition table comparison into separate function | expand |
On 18.05.21 05:20, James Hilliard wrote: > This should reduce the amount of duplicate code in cases where we > need to do multiple partition table comparisions, such as would be > required for hybrid MBR tables. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > handlers/diskpart_handler.c | 90 ++++++++++++++++++++++++------------- > 1 file changed, 60 insertions(+), 30 deletions(-) > > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > index 2cb524a..e50b54f 100644 > --- a/handlers/diskpart_handler.c > +++ b/handlers/diskpart_handler.c > @@ -134,11 +134,23 @@ static int diskpart_set_partition(struct fdisk_partition *pa, > /* > * Return true if partition differs > */ > -static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > { > + struct fdisk_parttype *type; > + const char *lbtype; > + > if (!firstpa || !secondpa) > return true; > > + type = fdisk_partition_get_type(firstpa); It looks like this is the functiuon I missed when I implemented the function, thanks ! > + if (!type) > + return true; > + > + if (fdisk_parttype_get_string(type)) > + lbtype = "gpt"; > + else > + lbtype = "dos"; > + > if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) || > (!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) && > fdisk_partition_cmp_start(firstpa, secondpa)) || > @@ -161,6 +173,47 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f > return false; > } > > +/* > + * Return 1 if table differs, 0 if table is the same, negative on error > + */ > +static int diskpart_table_cmp(struct fdisk_table *tb, struct fdisk_table *oldtb) > +{ > + size_t numnewparts = fdisk_table_get_nents(tb); > + size_t numpartondisk = fdisk_table_get_nents(oldtb); > + unsigned long i; > + int ret = 0; > + > + if (numpartondisk != numnewparts) { > + TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > + (long unsigned int)numpartondisk, (long unsigned int)numnewparts); > + ret = 1; > + } else { > + struct fdisk_partition *pa, *newpa; > + struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > + struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > + > + i = 0; > + while (i < numpartondisk && !ret) { > + newpa=NULL; > + pa = NULL; > + if (fdisk_table_next_partition (tb, itr, &newpa) || > + fdisk_table_next_partition (oldtb, olditr, &pa)) { > + TRACE("Partition not found, something went wrong %lu !", i); > + ret = -EFAULT; > + } else if (diskpart_partition_cmp(pa, newpa)) { > + ret = 1; > + } > + > + fdisk_unref_partition(newpa); > + fdisk_unref_partition(pa); > + i++; > + } > + fdisk_free_iter(itr); > + fdisk_free_iter(olditr); > + } > + return ret; > +} > + > static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table *tb) > { > int ret = 0; > @@ -404,36 +457,13 @@ static int diskpart(struct img_type *img, > * to check if they differ. > */ > if (!createtable) { > - size_t numpartondisk = fdisk_table_get_nents(oldtb); > - > - if (numpartondisk != i) { > - TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > - (long unsigned int)numpartondisk, i); > + ret = diskpart_table_cmp(tb, oldtb); > + if (ret < 0) > + goto handler_exit; > + else if (ret) > createtable = true; > - } else { > - struct fdisk_partition *pa, *newpa; > - struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > - struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > - > - i = 0; > - while (i < numpartondisk && !createtable) { > - newpa=NULL; > - pa = NULL; > - if (fdisk_table_next_partition (tb, itr, &newpa) || > - fdisk_table_next_partition (oldtb, olditr, &pa)) { > - TRACE("Partition not found, something went wrong %lu !", i); > - ret = -EFAULT; > - goto handler_exit; > - } > - if (diskpart_partition_cmp(lbtype, pa, newpa)) { > - createtable = true; > - } > - > - fdisk_unref_partition(newpa); > - fdisk_unref_partition(pa); > - i++; > - } > - } > + else > + createtable = false; > } > > if (createtable) { > Code looks much more cleaner. Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
On Tue, May 18, 2021 at 7:29 AM Stefano Babic <sbabic@denx.de> wrote: > > On 18.05.21 05:20, James Hilliard wrote: > > This should reduce the amount of duplicate code in cases where we > > need to do multiple partition table comparisions, such as would be > > required for hybrid MBR tables. > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > handlers/diskpart_handler.c | 90 ++++++++++++++++++++++++------------- > > 1 file changed, 60 insertions(+), 30 deletions(-) > > > > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > > index 2cb524a..e50b54f 100644 > > --- a/handlers/diskpart_handler.c > > +++ b/handlers/diskpart_handler.c > > @@ -134,11 +134,23 @@ static int diskpart_set_partition(struct fdisk_partition *pa, > > /* > > * Return true if partition differs > > */ > > -static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > > +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > > { > > + struct fdisk_parttype *type; > > + const char *lbtype; > > + > > if (!firstpa || !secondpa) > > return true; > > > > + type = fdisk_partition_get_type(firstpa); > > It looks like this is the functiuon I missed when I implemented the > function, thanks ! > > > + if (!type) > > + return true; > > + > > + if (fdisk_parttype_get_string(type)) > > + lbtype = "gpt"; > > + else > > + lbtype = "dos"; > > + > > if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) || > > (!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) && > > fdisk_partition_cmp_start(firstpa, secondpa)) || > > @@ -161,6 +173,47 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f > > return false; > > } > > > > +/* > > + * Return 1 if table differs, 0 if table is the same, negative on error > > + */ > > +static int diskpart_table_cmp(struct fdisk_table *tb, struct fdisk_table *oldtb) > > +{ > > + size_t numnewparts = fdisk_table_get_nents(tb); > > + size_t numpartondisk = fdisk_table_get_nents(oldtb); > > + unsigned long i; > > + int ret = 0; > > + > > + if (numpartondisk != numnewparts) { > > + TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > > + (long unsigned int)numpartondisk, (long unsigned int)numnewparts); > > + ret = 1; > > + } else { > > + struct fdisk_partition *pa, *newpa; > > + struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > > + struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > > + > > + i = 0; > > + while (i < numpartondisk && !ret) { > > + newpa=NULL; > > + pa = NULL; > > + if (fdisk_table_next_partition (tb, itr, &newpa) || > > + fdisk_table_next_partition (oldtb, olditr, &pa)) { > > + TRACE("Partition not found, something went wrong %lu !", i); > > + ret = -EFAULT; > > + } else if (diskpart_partition_cmp(pa, newpa)) { > > + ret = 1; > > + } > > + > > + fdisk_unref_partition(newpa); > > + fdisk_unref_partition(pa); > > + i++; > > + } > > + fdisk_free_iter(itr); > > + fdisk_free_iter(olditr); > > + } > > + return ret; > > +} > > + > > static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table *tb) > > { > > int ret = 0; > > @@ -404,36 +457,13 @@ static int diskpart(struct img_type *img, > > * to check if they differ. > > */ > > if (!createtable) { > > - size_t numpartondisk = fdisk_table_get_nents(oldtb); > > - > > - if (numpartondisk != i) { > > - TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > > - (long unsigned int)numpartondisk, i); > > + ret = diskpart_table_cmp(tb, oldtb); > > + if (ret < 0) > > + goto handler_exit; > > + else if (ret) > > createtable = true; > > - } else { > > - struct fdisk_partition *pa, *newpa; > > - struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > > - struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > > - > > - i = 0; > > - while (i < numpartondisk && !createtable) { > > - newpa=NULL; > > - pa = NULL; > > - if (fdisk_table_next_partition (tb, itr, &newpa) || > > - fdisk_table_next_partition (oldtb, olditr, &pa)) { > > - TRACE("Partition not found, something went wrong %lu !", i); > > - ret = -EFAULT; > > - goto handler_exit; > > - } > > - if (diskpart_partition_cmp(lbtype, pa, newpa)) { > > - createtable = true; > > - } > > - > > - fdisk_unref_partition(newpa); > > - fdisk_unref_partition(pa); > > - i++; > > - } > > - } > > + else > > + createtable = false; > > } > > > > if (createtable) { > > > > Code looks much more cleaner. Yeah, should be especially so for implementing the hybrid MBR feature as we'll need to diff two tables essentially. I was getting ready to rebase my hybrid MBR patch but noticed I had a fairly trivial merge conflict between this and https://groups.google.com/g/swupdate/c/pI7ohMYWykY do you want to just handle that resolution yourself? Once these two patches are merged I should be able clean up the hybrid MBR patch significantly. > > Acked-by: Stefano Babic <sbabic@denx.de> > > Best regards, > Stefano Babic > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
On 19.05.21 07:29, James Hilliard wrote: > On Tue, May 18, 2021 at 7:29 AM Stefano Babic <sbabic@denx.de> wrote: >> >> On 18.05.21 05:20, James Hilliard wrote: >>> This should reduce the amount of duplicate code in cases where we >>> need to do multiple partition table comparisions, such as would be >>> required for hybrid MBR tables. >>> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>> --- >>> handlers/diskpart_handler.c | 90 ++++++++++++++++++++++++------------- >>> 1 file changed, 60 insertions(+), 30 deletions(-) >>> >>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c >>> index 2cb524a..e50b54f 100644 >>> --- a/handlers/diskpart_handler.c >>> +++ b/handlers/diskpart_handler.c >>> @@ -134,11 +134,23 @@ static int diskpart_set_partition(struct fdisk_partition *pa, >>> /* >>> * Return true if partition differs >>> */ >>> -static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) >>> +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) >>> { >>> + struct fdisk_parttype *type; >>> + const char *lbtype; >>> + >>> if (!firstpa || !secondpa) >>> return true; >>> >>> + type = fdisk_partition_get_type(firstpa); >> >> It looks like this is the functiuon I missed when I implemented the >> function, thanks ! >> >>> + if (!type) >>> + return true; >>> + >>> + if (fdisk_parttype_get_string(type)) >>> + lbtype = "gpt"; >>> + else >>> + lbtype = "dos"; >>> + >>> if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) || >>> (!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) && >>> fdisk_partition_cmp_start(firstpa, secondpa)) || >>> @@ -161,6 +173,47 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f >>> return false; >>> } >>> >>> +/* >>> + * Return 1 if table differs, 0 if table is the same, negative on error >>> + */ >>> +static int diskpart_table_cmp(struct fdisk_table *tb, struct fdisk_table *oldtb) >>> +{ >>> + size_t numnewparts = fdisk_table_get_nents(tb); >>> + size_t numpartondisk = fdisk_table_get_nents(oldtb); >>> + unsigned long i; >>> + int ret = 0; >>> + >>> + if (numpartondisk != numnewparts) { >>> + TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", >>> + (long unsigned int)numpartondisk, (long unsigned int)numnewparts); >>> + ret = 1; >>> + } else { >>> + struct fdisk_partition *pa, *newpa; >>> + struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); >>> + struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); >>> + >>> + i = 0; >>> + while (i < numpartondisk && !ret) { >>> + newpa=NULL; >>> + pa = NULL; >>> + if (fdisk_table_next_partition (tb, itr, &newpa) || >>> + fdisk_table_next_partition (oldtb, olditr, &pa)) { >>> + TRACE("Partition not found, something went wrong %lu !", i); >>> + ret = -EFAULT; >>> + } else if (diskpart_partition_cmp(pa, newpa)) { >>> + ret = 1; >>> + } >>> + >>> + fdisk_unref_partition(newpa); >>> + fdisk_unref_partition(pa); >>> + i++; >>> + } >>> + fdisk_free_iter(itr); >>> + fdisk_free_iter(olditr); >>> + } >>> + return ret; >>> +} >>> + >>> static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table *tb) >>> { >>> int ret = 0; >>> @@ -404,36 +457,13 @@ static int diskpart(struct img_type *img, >>> * to check if they differ. >>> */ >>> if (!createtable) { >>> - size_t numpartondisk = fdisk_table_get_nents(oldtb); >>> - >>> - if (numpartondisk != i) { >>> - TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", >>> - (long unsigned int)numpartondisk, i); >>> + ret = diskpart_table_cmp(tb, oldtb); >>> + if (ret < 0) >>> + goto handler_exit; >>> + else if (ret) >>> createtable = true; >>> - } else { >>> - struct fdisk_partition *pa, *newpa; >>> - struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); >>> - struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); >>> - >>> - i = 0; >>> - while (i < numpartondisk && !createtable) { >>> - newpa=NULL; >>> - pa = NULL; >>> - if (fdisk_table_next_partition (tb, itr, &newpa) || >>> - fdisk_table_next_partition (oldtb, olditr, &pa)) { >>> - TRACE("Partition not found, something went wrong %lu !", i); >>> - ret = -EFAULT; >>> - goto handler_exit; >>> - } >>> - if (diskpart_partition_cmp(lbtype, pa, newpa)) { >>> - createtable = true; >>> - } >>> - >>> - fdisk_unref_partition(newpa); >>> - fdisk_unref_partition(pa); >>> - i++; >>> - } >>> - } >>> + else >>> + createtable = false; >>> } >>> >>> if (createtable) { >>> >> >> Code looks much more cleaner. > Yeah, should be especially so for implementing the hybrid MBR feature > as we'll need to > diff two tables essentially. > > I was getting ready to rebase my hybrid MBR patch but noticed I had a > fairly trivial > merge conflict between this and > https://groups.google.com/g/swupdate/c/pI7ohMYWykY > do you want to just handle that resolution yourself? This is also redundant: + else + createtable = false; It runs in a branch where creatable was laready tested as false. I fixed the conflict and merge into the coverity_scan for testing - take a look, then I'll cherry pick the commits into -master. Best regards, Stefano Babic > Once these two > patches are merged > I should be able clean up the hybrid MBR patch significantly. >> >> Acked-by: Stefano Babic <sbabic@denx.de> >> >> Best regards, >> Stefano Babic >> >> >> -- >> ===================================================================== >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de >> ===================================================================== >
On Wed, May 19, 2021 at 12:29 AM Stefano Babic <sbabic@denx.de> wrote: > > On 19.05.21 07:29, James Hilliard wrote: > > On Tue, May 18, 2021 at 7:29 AM Stefano Babic <sbabic@denx.de> wrote: > >> > >> On 18.05.21 05:20, James Hilliard wrote: > >>> This should reduce the amount of duplicate code in cases where we > >>> need to do multiple partition table comparisions, such as would be > >>> required for hybrid MBR tables. > >>> > >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>> --- > >>> handlers/diskpart_handler.c | 90 ++++++++++++++++++++++++------------- > >>> 1 file changed, 60 insertions(+), 30 deletions(-) > >>> > >>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > >>> index 2cb524a..e50b54f 100644 > >>> --- a/handlers/diskpart_handler.c > >>> +++ b/handlers/diskpart_handler.c > >>> @@ -134,11 +134,23 @@ static int diskpart_set_partition(struct fdisk_partition *pa, > >>> /* > >>> * Return true if partition differs > >>> */ > >>> -static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > >>> +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > >>> { > >>> + struct fdisk_parttype *type; > >>> + const char *lbtype; > >>> + > >>> if (!firstpa || !secondpa) > >>> return true; > >>> > >>> + type = fdisk_partition_get_type(firstpa); > >> > >> It looks like this is the functiuon I missed when I implemented the > >> function, thanks ! > >> > >>> + if (!type) > >>> + return true; > >>> + > >>> + if (fdisk_parttype_get_string(type)) > >>> + lbtype = "gpt"; > >>> + else > >>> + lbtype = "dos"; > >>> + > >>> if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) || > >>> (!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) && > >>> fdisk_partition_cmp_start(firstpa, secondpa)) || > >>> @@ -161,6 +173,47 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f > >>> return false; > >>> } > >>> > >>> +/* > >>> + * Return 1 if table differs, 0 if table is the same, negative on error > >>> + */ > >>> +static int diskpart_table_cmp(struct fdisk_table *tb, struct fdisk_table *oldtb) > >>> +{ > >>> + size_t numnewparts = fdisk_table_get_nents(tb); > >>> + size_t numpartondisk = fdisk_table_get_nents(oldtb); > >>> + unsigned long i; > >>> + int ret = 0; > >>> + > >>> + if (numpartondisk != numnewparts) { > >>> + TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > >>> + (long unsigned int)numpartondisk, (long unsigned int)numnewparts); > >>> + ret = 1; > >>> + } else { > >>> + struct fdisk_partition *pa, *newpa; > >>> + struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > >>> + struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > >>> + > >>> + i = 0; > >>> + while (i < numpartondisk && !ret) { > >>> + newpa=NULL; > >>> + pa = NULL; > >>> + if (fdisk_table_next_partition (tb, itr, &newpa) || > >>> + fdisk_table_next_partition (oldtb, olditr, &pa)) { > >>> + TRACE("Partition not found, something went wrong %lu !", i); > >>> + ret = -EFAULT; > >>> + } else if (diskpart_partition_cmp(pa, newpa)) { > >>> + ret = 1; > >>> + } > >>> + > >>> + fdisk_unref_partition(newpa); > >>> + fdisk_unref_partition(pa); > >>> + i++; > >>> + } > >>> + fdisk_free_iter(itr); > >>> + fdisk_free_iter(olditr); > >>> + } > >>> + return ret; > >>> +} > >>> + > >>> static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table *tb) > >>> { > >>> int ret = 0; > >>> @@ -404,36 +457,13 @@ static int diskpart(struct img_type *img, > >>> * to check if they differ. > >>> */ > >>> if (!createtable) { > >>> - size_t numpartondisk = fdisk_table_get_nents(oldtb); > >>> - > >>> - if (numpartondisk != i) { > >>> - TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", > >>> - (long unsigned int)numpartondisk, i); > >>> + ret = diskpart_table_cmp(tb, oldtb); > >>> + if (ret < 0) > >>> + goto handler_exit; > >>> + else if (ret) > >>> createtable = true; > >>> - } else { > >>> - struct fdisk_partition *pa, *newpa; > >>> - struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); > >>> - struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); > >>> - > >>> - i = 0; > >>> - while (i < numpartondisk && !createtable) { > >>> - newpa=NULL; > >>> - pa = NULL; > >>> - if (fdisk_table_next_partition (tb, itr, &newpa) || > >>> - fdisk_table_next_partition (oldtb, olditr, &pa)) { > >>> - TRACE("Partition not found, something went wrong %lu !", i); > >>> - ret = -EFAULT; > >>> - goto handler_exit; > >>> - } > >>> - if (diskpart_partition_cmp(lbtype, pa, newpa)) { > >>> - createtable = true; > >>> - } > >>> - > >>> - fdisk_unref_partition(newpa); > >>> - fdisk_unref_partition(pa); > >>> - i++; > >>> - } > >>> - } > >>> + else > >>> + createtable = false; > >>> } > >>> > >>> if (createtable) { > >>> > >> > >> Code looks much more cleaner. > > Yeah, should be especially so for implementing the hybrid MBR feature > > as we'll need to > > diff two tables essentially. > > > > I was getting ready to rebase my hybrid MBR patch but noticed I had a > > fairly trivial > > merge conflict between this and > > https://groups.google.com/g/swupdate/c/pI7ohMYWykY > > do you want to just handle that resolution yourself? > > This is also redundant: > > + else > + createtable = false; > > It runs in a branch where creatable was laready tested as false. > > I fixed the conflict and merge into the coverity_scan for testing - take > a look, then I'll cherry pick the commits into -master. Ran a few quick tests, looks good to me. > > Best regards, > Stefano Babic > > > Once these two > > patches are merged > > I should be able clean up the hybrid MBR patch significantly. > >> > >> Acked-by: Stefano Babic <sbabic@denx.de> > >> > >> Best regards, > >> Stefano Babic > >> > >> > >> -- > >> ===================================================================== > >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > >> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > >> ===================================================================== > > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
Hi James, On 19.05.21 08:39, James Hilliard wrote: [snip] >> I fixed the conflict and merge into the coverity_scan for testing - take >> a look, then I'll cherry pick the commits into -master. > Ran a few quick tests, looks good to me. coverity ran, too, I merge both on -master. Best regards, Stefano Babic
diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c index 2cb524a..e50b54f 100644 --- a/handlers/diskpart_handler.c +++ b/handlers/diskpart_handler.c @@ -134,11 +134,23 @@ static int diskpart_set_partition(struct fdisk_partition *pa, /* * Return true if partition differs */ -static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) { + struct fdisk_parttype *type; + const char *lbtype; + if (!firstpa || !secondpa) return true; + type = fdisk_partition_get_type(firstpa); + if (!type) + return true; + + if (fdisk_parttype_get_string(type)) + lbtype = "gpt"; + else + lbtype = "dos"; + if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) || (!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) && fdisk_partition_cmp_start(firstpa, secondpa)) || @@ -161,6 +173,47 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f return false; } +/* + * Return 1 if table differs, 0 if table is the same, negative on error + */ +static int diskpart_table_cmp(struct fdisk_table *tb, struct fdisk_table *oldtb) +{ + size_t numnewparts = fdisk_table_get_nents(tb); + size_t numpartondisk = fdisk_table_get_nents(oldtb); + unsigned long i; + int ret = 0; + + if (numpartondisk != numnewparts) { + TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", + (long unsigned int)numpartondisk, (long unsigned int)numnewparts); + ret = 1; + } else { + struct fdisk_partition *pa, *newpa; + struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); + struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); + + i = 0; + while (i < numpartondisk && !ret) { + newpa=NULL; + pa = NULL; + if (fdisk_table_next_partition (tb, itr, &newpa) || + fdisk_table_next_partition (oldtb, olditr, &pa)) { + TRACE("Partition not found, something went wrong %lu !", i); + ret = -EFAULT; + } else if (diskpart_partition_cmp(pa, newpa)) { + ret = 1; + } + + fdisk_unref_partition(newpa); + fdisk_unref_partition(pa); + i++; + } + fdisk_free_iter(itr); + fdisk_free_iter(olditr); + } + return ret; +} + static int diskpart_reload_table(struct fdisk_context *cxt, struct fdisk_table *tb) { int ret = 0; @@ -404,36 +457,13 @@ static int diskpart(struct img_type *img, * to check if they differ. */ if (!createtable) { - size_t numpartondisk = fdisk_table_get_nents(oldtb); - - if (numpartondisk != i) { - TRACE("Number of partitions differs on disk: %lu <--> requested: %lu", - (long unsigned int)numpartondisk, i); + ret = diskpart_table_cmp(tb, oldtb); + if (ret < 0) + goto handler_exit; + else if (ret) createtable = true; - } else { - struct fdisk_partition *pa, *newpa; - struct fdisk_iter *itr = fdisk_new_iter(FDISK_ITER_FORWARD); - struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD); - - i = 0; - while (i < numpartondisk && !createtable) { - newpa=NULL; - pa = NULL; - if (fdisk_table_next_partition (tb, itr, &newpa) || - fdisk_table_next_partition (oldtb, olditr, &pa)) { - TRACE("Partition not found, something went wrong %lu !", i); - ret = -EFAULT; - goto handler_exit; - } - if (diskpart_partition_cmp(lbtype, pa, newpa)) { - createtable = true; - } - - fdisk_unref_partition(newpa); - fdisk_unref_partition(pa); - i++; - } - } + else + createtable = false; } if (createtable) {
This should reduce the amount of duplicate code in cases where we need to do multiple partition table comparisions, such as would be required for hybrid MBR tables. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- handlers/diskpart_handler.c | 90 ++++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 30 deletions(-)