From: Benjamin Marzinski <bmarzins@redhat.com>
To: lixiaokeng <lixiaokeng@huawei.com>
Cc: linfeilong <linfeilong@huawei.com>,
dm-devel mailing list <dm-devel@redhat.com>,
Martin Wilck <mwilck@suse.com>,
"liuzhiqiang (I)" <liuzhiqiang26@huawei.com>
Subject: Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map
Date: Tue, 8 Sep 2020 10:45:41 -0500 [thread overview]
Message-ID: <20200908154540.GK11108@octiron.msp.redhat.com> (raw)
In-Reply-To: <8dd72a70-991a-cb7d-3279-3d5036df1b9d@huawei.com>
On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
>
> >> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char * params, int len)
> >> get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
> >> add_feature(&mp->features, retain_hwhandler);
> >>
> >> - f = STRDUP(mp->features);
> >
> > clearly strdup()ing without checking if mp->features NULL is incorrect.
> > However, I'm not sure that we need to fail if mp->features is NULL.
> > instead, int the APPEND call, we could use the gcc ternary operator
> > extension
> >
> > (mp->features)?: "0"
> >
> > to use "0" if mp->features is NULL.
> >
> > Also, have you seen this actually occur? Or is this just a theoretical
> > issue that you've found from reading the code. It seems like
> > setup_map() will always call select_features() before calling
> > assemble_map(), which should mean that mp->features will always be set
> > in this case. Perhaps I'm missing something here.
> >
> > -Ben
> >
> Hi Ben,
> This just a theoretical issue and I did not see it. But it's not necessary
> to call strdup. In your opinion, need multipath be checked? I will make new
> patch with your suggestion.
Since we don't believe it's possible for mp->features (or mp->hwhandler)
to be set to NULL here, it makes sense to print an error if it is NULL.
So, I guess my suggestion would be to print an error message if
mp->features or mp->hwhandler are NULL, but to assemble the map anyway,
using the default value of "0" if they are NULL. That's how
assemble_map() currently handles failures in add_feature().
add_feature() will print an error, but assemble_map() will go ahead with
assembling the map.
I'm willing to be convinced that there is a better solution, however.
-Ben
> -Lixiaokeng
> >> + if (!mp->features) {
> >> + condlog(0, "mp->features is still NULL.");
> >> + goto err;
> >> + }
>
>
next prev parent reply other threads:[~2020-09-08 15:45 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-02 6:40 [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool lixiaokeng
2020-09-02 7:15 ` [PATCH 01/14] multipathd: return if dm_get_major_minor failed in, cli_add_map lixiaokeng
2020-09-03 17:26 ` Martin Wilck
2020-09-04 2:48 ` lixiaokeng
2020-09-04 13:13 ` Martin Wilck
2020-09-02 7:16 ` [PATCH 02/14] libmultipath: check malloc return value in, print_foreign_topology lixiaokeng
2020-09-03 17:29 ` Martin Wilck
2020-09-02 7:17 ` [PATCH 03/14] libmultipath: use map instead of dm_task_get_name lixiaokeng
2020-09-03 17:35 ` Martin Wilck
2020-09-02 7:17 ` [PATCH 04/14] multipathd: check MALLOC return value in, mpath_pr_event_handler_fn lixiaokeng
2020-09-03 18:57 ` Martin Wilck
2020-09-02 7:18 ` [PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func lixiaokeng
2020-09-03 19:13 ` Martin Wilck
2020-09-04 18:25 ` Benjamin Marzinski
2020-09-04 21:24 ` Martin Wilck
2020-09-02 7:19 ` [PATCH 06/14] kpartx: check return value of malloc in main func lixiaokeng
2020-09-03 19:23 ` Martin Wilck
2020-09-02 7:19 ` [PATCH 07/14] libmultipath: check return value of dm_mapname in, sysfs_check_holders lixiaokeng
2020-09-04 20:28 ` Benjamin Marzinski
2020-09-02 7:20 ` [PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words lixiaokeng
2020-09-04 21:11 ` Benjamin Marzinski
2020-09-07 11:58 ` lixiaokeng
2020-09-02 7:21 ` [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map lixiaokeng
2020-09-04 21:30 ` Benjamin Marzinski
2020-09-07 12:21 ` lixiaokeng
2020-09-08 15:45 ` Benjamin Marzinski [this message]
2020-09-08 16:35 ` Martin Wilck
2020-09-08 16:44 ` Benjamin Marzinski
2020-09-09 3:18 ` lixiaokeng
2020-09-09 10:11 ` Martin Wilck
2020-09-02 7:22 ` [PATCH 10/14] util/tests: use assert_non_null to ensure malloc, returns non-null pointer lixiaokeng
2020-09-04 21:31 ` Benjamin Marzinski
2020-09-02 7:23 ` [PATCH 11/14] mpathpersist: check whether malloc paramp->trnptid_list, fails in handle_args func lixiaokeng
2020-09-04 23:52 ` Benjamin Marzinski
2020-09-07 12:26 ` lixiaokeng
2020-09-02 7:24 ` [PATCH 12/14] libmultipathpersist: use update_multipath_table/status, in get_mpvec lixiaokeng
2020-09-05 0:05 ` Benjamin Marzinski
2020-09-07 13:26 ` lixiaokeng
2020-09-07 14:33 ` Martin Wilck
2020-09-02 7:25 ` [PATCH 13/14] multipath: use update_multipath_table/status in, check_useable_paths lixiaokeng
2020-09-05 0:10 ` Benjamin Marzinski
2020-09-02 7:26 ` [PATCH 14/14] multipathpersist: delete unused variable in handle_args lixiaokeng
2020-09-05 0:14 ` Benjamin Marzinski
2020-09-03 20:08 ` [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool Martin Wilck
2020-09-04 2:24 ` lixiaokeng
2020-09-04 20:00 ` Benjamin Marzinski
2020-09-04 21:28 ` Martin Wilck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200908154540.GK11108@octiron.msp.redhat.com \
--to=bmarzins@redhat.com \
--cc=dm-devel@redhat.com \
--cc=linfeilong@huawei.com \
--cc=liuzhiqiang26@huawei.com \
--cc=lixiaokeng@huawei.com \
--cc=mwilck@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.