On 8/26/21 5:40 AM, Mateusz Grzonka wrote:
> map allocated through map_by_uuid() is not freed if mdfd is invalid.
> It is also not freed at the end of Incremental_container().
> In addition mdfd is not closed, and mdinfo list is not
> freed too.
> Fix it.
>
> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@xxxxxxxxx>
Hi Mateusz,
Sorry for the late feedback.
Looking through this change, I would prefer to have had this done in
multiple patches that are easier to review individually. Like the first
close is obviously correct and the last change too, but they are
independent of the middle changes.
I also feel the second set of changes relying on doclose could be less
convoluted if we just check the return value from open_dev() and
create_mddev() immediately instead of trying to handle it in a catch all
case. This would help make the code easier to read.
Please see comments below.
Thoughts?
Thanks,
Jes
> Incremental.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/Incremental.c b/Incremental.c
> index cd9cc0fc..6678739b 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -1416,6 +1416,7 @@ restart:
> }
> sysfs_free(sra);
> }
> + close(mdfd);
> }
> map_free(mapl);
> return rv;
> @@ -1499,6 +1500,7 @@ static int Incremental_container(struct supertype *st, char *devname,
> }
> for (ra = list ; ra ; ra = ra->next) {
> int mdfd;
> + int doclose = 0;
> char chosen_name[1024];
> struct map_ent *mp;
> struct mddev_ident *match = NULL;
> @@ -1513,6 +1515,7 @@ static int Incremental_container(struct supertype *st, char *devname,
>
> if (mp) {
> mdfd = open_dev(mp->devnm);
> + doclose = 1;
Check mdfd >= 0 here and jump to release.
> if (mp->path)
> strcpy(chosen_name, mp->path);
> else
> @@ -1572,22 +1575,30 @@ static int Incremental_container(struct supertype *st, char *devname,
> c->autof,
> trustworthy,
> chosen_name, 0);
> + doclose = 1;
Check mdfd >= 0 here and jump to release.
> }
> - if (only && (!mp || strcmp(mp->devnm, only) != 0))
> - continue;
>
> if (mdfd < 0) {
> pr_err("failed to open %s: %s.\n",
> chosen_name, strerror(errno));
> - return 2;
> + rv = 2;
> + goto release;
> + }
We can then get rid of this block completely.
> + if (only && (!mp || strcmp(mp->devnm, only) != 0)) {
> + if (doclose)
> + close(mdfd);
> + continue;
And call close() here unconditionally.
> }
>
> assemble_container_content(st, mdfd, ra, c,
> chosen_name, &result);
> map_free(map);
> map = NULL;
> - close(mdfd);
> + if (doclose)
> + close(mdfd);
> }
and get rid of the if() here
> +
> if (c->export && result) {
> char sep = '=';
> printf("MD_STARTED");
> @@ -1609,7 +1620,11 @@ static int Incremental_container(struct supertype *st, char *devname,
> }
> printf("\n");
> }
> - return 0;
> +
> +release:
> + map_free(map);
> + sysfs_free(list);
> + return rv;
> }
>
> static void run_udisks(char *arg1, char *arg2)
> @@ -1701,7 +1716,7 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
> return 1;
> }
> mdfd = open_dev_excl(ent->devnm);
> - if (mdfd > 0) {
> + if (mdfd >= 0) {
This part is also independent and fine by itself.
> close(mdfd);
> if (sysfs_get_str(&mdi, NULL, "array_state",
> buf, sizeof(buf)) > 0) {
>
