From: Martin Wilck <mwilck@suse.com>
To: lixiaokeng <lixiaokeng@huawei.com>,
Benjamin Marzinski <bmarzins@redhat.com>
Cc: dm-devel, list <dm-devel@redhat.com>,
linfeilong <linfeilong@huawei.com>,
"liuzhiqiang (I)" <liuzhiqiang26@huawei.com>
Subject: Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map
Date: Wed, 09 Sep 2020 12:11:12 +0200 [thread overview]
Message-ID: <3ae3e9fdc5692b5d5244e58e777a18c14bda5abd.camel@suse.com> (raw)
In-Reply-To: <3154dc82-e5cf-5d34-6955-04a0ad6e01f0@huawei.com>
On Wed, 2020-09-09 at 11:18 +0800, lixiaokeng wrote:
>
> On 2020/9/9 0:35, Martin Wilck wrote:
> >
> > What about this:
> >
> > assemble_map() is only called from setup_map(), which sets mp-
> > >features
> > in select_features(). So what we should do is check for NULL after
> > select_features(), or have that function return an error code if
> > strdup
> > fails, and bail out early in setup_map() in that case.
> >
> > Then we simply need to add a comment in assemble_map() saying that
> > mp->features must be non-null when this function is called.
> >
> > As I said earlier, I'm of course not against checking function
> > parameters, but here we should fail to setup a "struct multipath"
> > in
> > the first place in setup map(), rather than returning an
> > incompletely
> > initialized one. If we handle it this way, we don't need to check
> > the
> > fields of struct multipath over and over again. Similar arguments
> > hold
> > for other structs.
> >
> > Of course this kind of assumption needs to be better documented in
> > the
> > code.
> >
> > Regards
> > Martin
> >
> >
> Hi Martin,
> If I don't misunderstand, we check feature after select_features
> and
> return 1 if feature is NULL in setup_map. We delete strdup and add
> message "mp->features must be non-null" in assemble_map. Like this:
> ---
> select_features(conf, mpp);
> if (!mpp->featrues)
> return 1;
>
> ---
> /* mp->feature must not be NULL */
> APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler
> ---
Yes. I guess error handling in setup_map() needs a closer look, too.
Martin
next prev parent reply other threads:[~2020-09-09 10:11 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
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 [this message]
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=3ae3e9fdc5692b5d5244e58e777a18c14bda5abd.camel@suse.com \
--to=mwilck@suse.com \
--cc=bmarzins@redhat.com \
--cc=dm-devel@redhat.com \
--cc=linfeilong@huawei.com \
--cc=liuzhiqiang26@huawei.com \
--cc=lixiaokeng@huawei.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.