| Message ID | 20210513101233.87119-1-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] diskpart: improve partition diff logging | expand |
Hi James, On 13.05.21 12:12, James Hilliard wrote: > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > handlers/diskpart_handler.c | 43 +++++++++++++++++++++++++++++++------ > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > index 65010c1..3013b80 100644 > --- a/handlers/diskpart_handler.c > +++ b/handlers/diskpart_handler.c > @@ -131,10 +131,29 @@ static int diskpart_set_partition(struct fdisk_partition *pa, > return ret; > } > > +static void diskpart_partition_info(const char *name, struct fdisk_partition *pa, struct fdisk_context *cxt, > + size_t nids, int *ids, struct fdisk_label *lb) > +{ > + size_t i; > + TRACE("%s:", name); > + for (i = 0; i < nids; i++) { > + const struct fdisk_field *field = > + fdisk_label_get_field(lb, ids[i]); > + char *data = NULL; > + if (!field) > + continue; > + if (fdisk_partition_to_string(pa, cxt, ids[i], &data)) > + continue; > + TRACE("\t%s: %s", fdisk_field_get_name(field), data); > + free(data); > + } > +} > + > /* > * 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(const char *lbtype, struct fdisk_context *cxt, > + struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > { > if (!firstpa || !secondpa) > return true; > @@ -151,11 +170,21 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f > fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) != > fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) || > fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) { > - TRACE("Partition differ : %s(%llu) <--> %s(%llu)", > - fdisk_partition_get_name (firstpa) ? fdisk_partition_get_name(firstpa) : "", > - (long long unsigned)fdisk_partition_get_size(firstpa), > - fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : "", > - (long long unsigned)fdisk_partition_get_size(secondpa)); > + struct fdisk_label *lb; > + int *ids = NULL; > + size_t nids = 0; > + lb = fdisk_get_label(cxt, NULL); Label and ids array are allocated by fdisk, but they are not freed. This patch generates memory leaks. > + fdisk_label_get_fields_ids(lb, cxt, &ids, &nids); > + TRACE("Partition differ:"); > + if (ids && lb) { > + diskpart_partition_info("Original", firstpa, cxt, nids, ids, lb); > + diskpart_partition_info("New", secondpa, cxt, nids, ids, lb); > + } else { > + if (!ids) > + ERROR("Failed to load field ids"); > + if (!lb) > + ERROR("Failed to load label"); > + } > return true; > } > return false; > @@ -376,7 +405,7 @@ static int diskpart(struct img_type *img, > ret = -EFAULT; > goto handler_exit; > } > - if (diskpart_partition_cmp(lbtype, pa, newpa)) { > + if (diskpart_partition_cmp(lbtype, cxt, pa, newpa)) { > createtable = true; > } > > Best regards, Stefano Babic
On Sun, May 16, 2021 at 5:46 AM Stefano Babic <stefano.babic@denx.de> wrote: > > Hi James, > > On 13.05.21 12:12, James Hilliard wrote: > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > handlers/diskpart_handler.c | 43 +++++++++++++++++++++++++++++++------ > > 1 file changed, 36 insertions(+), 7 deletions(-) > > > > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > > index 65010c1..3013b80 100644 > > --- a/handlers/diskpart_handler.c > > +++ b/handlers/diskpart_handler.c > > @@ -131,10 +131,29 @@ static int diskpart_set_partition(struct fdisk_partition *pa, > > return ret; > > } > > > > +static void diskpart_partition_info(const char *name, struct fdisk_partition *pa, struct fdisk_context *cxt, > > + size_t nids, int *ids, struct fdisk_label *lb) > > +{ > > + size_t i; > > + TRACE("%s:", name); > > + for (i = 0; i < nids; i++) { > > + const struct fdisk_field *field = > > + fdisk_label_get_field(lb, ids[i]); > > + char *data = NULL; > > + if (!field) > > + continue; > > + if (fdisk_partition_to_string(pa, cxt, ids[i], &data)) > > + continue; > > + TRACE("\t%s: %s", fdisk_field_get_name(field), data); > > + free(data); > > + } > > +} > > + > > /* > > * 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(const char *lbtype, struct fdisk_context *cxt, > > + struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) > > { > > if (!firstpa || !secondpa) > > return true; > > @@ -151,11 +170,21 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f > > fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) != > > fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) || > > fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) { > > - TRACE("Partition differ : %s(%llu) <--> %s(%llu)", > > - fdisk_partition_get_name (firstpa) ? fdisk_partition_get_name(firstpa) : "", > > - (long long unsigned)fdisk_partition_get_size(firstpa), > > - fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : "", > > - (long long unsigned)fdisk_partition_get_size(secondpa)); > > + struct fdisk_label *lb; > > + int *ids = NULL; > > + size_t nids = 0; > > + lb = fdisk_get_label(cxt, NULL); > > Label and ids array are allocated by fdisk, but they are not freed. This > patch generates memory leaks. Labels seem to be attached to the context and can't be freed separately: https://github.com/karelzak/util-linux/blob/1c75a85101e36ebc193183733821546f0fa430fc/libfdisk/src/context.c#L215-L217 Added a free for the ids and the partition iterators in my v2. > > > + fdisk_label_get_fields_ids(lb, cxt, &ids, &nids); > > + TRACE("Partition differ:"); > > + if (ids && lb) { > > + diskpart_partition_info("Original", firstpa, cxt, nids, ids, lb); > > + diskpart_partition_info("New", secondpa, cxt, nids, ids, lb); > > + } else { > > + if (!ids) > > + ERROR("Failed to load field ids"); > > + if (!lb) > > + ERROR("Failed to load label"); > > + } > > return true; > > } > > return false; > > @@ -376,7 +405,7 @@ static int diskpart(struct img_type *img, > > ret = -EFAULT; > > goto handler_exit; > > } > > - if (diskpart_partition_cmp(lbtype, pa, newpa)) { > > + if (diskpart_partition_cmp(lbtype, cxt, pa, newpa)) { > > createtable = true; > > } > > > > > > 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 > =====================================================================
diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c index 65010c1..3013b80 100644 --- a/handlers/diskpart_handler.c +++ b/handlers/diskpart_handler.c @@ -131,10 +131,29 @@ static int diskpart_set_partition(struct fdisk_partition *pa, return ret; } +static void diskpart_partition_info(const char *name, struct fdisk_partition *pa, struct fdisk_context *cxt, + size_t nids, int *ids, struct fdisk_label *lb) +{ + size_t i; + TRACE("%s:", name); + for (i = 0; i < nids; i++) { + const struct fdisk_field *field = + fdisk_label_get_field(lb, ids[i]); + char *data = NULL; + if (!field) + continue; + if (fdisk_partition_to_string(pa, cxt, ids[i], &data)) + continue; + TRACE("\t%s: %s", fdisk_field_get_name(field), data); + free(data); + } +} + /* * 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(const char *lbtype, struct fdisk_context *cxt, + struct fdisk_partition *firstpa, struct fdisk_partition *secondpa) { if (!firstpa || !secondpa) return true; @@ -151,11 +170,21 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) != fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) || fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) { - TRACE("Partition differ : %s(%llu) <--> %s(%llu)", - fdisk_partition_get_name (firstpa) ? fdisk_partition_get_name(firstpa) : "", - (long long unsigned)fdisk_partition_get_size(firstpa), - fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : "", - (long long unsigned)fdisk_partition_get_size(secondpa)); + struct fdisk_label *lb; + int *ids = NULL; + size_t nids = 0; + lb = fdisk_get_label(cxt, NULL); + fdisk_label_get_fields_ids(lb, cxt, &ids, &nids); + TRACE("Partition differ:"); + if (ids && lb) { + diskpart_partition_info("Original", firstpa, cxt, nids, ids, lb); + diskpart_partition_info("New", secondpa, cxt, nids, ids, lb); + } else { + if (!ids) + ERROR("Failed to load field ids"); + if (!lb) + ERROR("Failed to load label"); + } return true; } return false; @@ -376,7 +405,7 @@ static int diskpart(struct img_type *img, ret = -EFAULT; goto handler_exit; } - if (diskpart_partition_cmp(lbtype, pa, newpa)) { + if (diskpart_partition_cmp(lbtype, cxt, pa, newpa)) { createtable = true; }
Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- handlers/diskpart_handler.c | 43 +++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-)