From: "ira.weiny" <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
To: Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Cc: Vladimir Koushnir
<vladimirk-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>,
"linux-rdma
(linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org)"
<linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH infiniband-diags] vendstat: mad_rpc_close_port not called in corner cases
Date: Wed, 22 Apr 2015 11:26:50 -0400 [thread overview]
Message-ID: <20150422152649.GA7601@phlsvsds.ph.intel.com> (raw)
In-Reply-To: <5536584C.8070505-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
On Tue, Apr 21, 2015 at 10:01:48AM -0400, Hal Rosenstock wrote:
> From: Vladimir Koushnir <vladimirk-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Date: Thu, 16 Apr 2015 18:27:39 +0300
>
> Call mad_rpc_close_port in all corner cases.
> This is needed so fds are not leaked with ibsim.
>
> Signed-off-by: Vladimir Koushnir <vladimirk-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
> src/vendstat.c | 82 ++++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 53 insertions(+), 29 deletions(-)
>
> diff --git a/src/vendstat.c b/src/vendstat.c
> index 11ee73e..baade3e 100644
> --- a/src/vendstat.c
> +++ b/src/vendstat.c
> @@ -172,13 +172,14 @@ static int do_vendor(ib_portid_t *portid, struct ibmad_port *srcport,
> call.attrid = attr_id;
> call.mod = attr_mod;
>
> - if (!ib_vendor_call_via(data, portid, &call, srcport))
> - IBEXIT("vendstat: method %u, attribute %u", method, attr_id);
> -
> + if (!ib_vendor_call_via(data, portid, &call, srcport)) {
> + fprintf(stderr,"vendstat: method %u, attribute %u failure\n", method, attr_id);
We lose the function information when not calling IBEXIT.
Can you create a new MACRO in ibdiag_comon.h called IBERROR? And preserve this
information.
Same comment throughout,
Ira
> + return -1;
> + }
> return 0;
> }
>
> -static void do_config_space_records(ib_portid_t *portid, unsigned set,
> +static int do_config_space_records(ib_portid_t *portid, unsigned set,
> is3_config_space_t *cs, unsigned records)
> {
> unsigned i;
> @@ -194,17 +195,19 @@ static void do_config_space_records(ib_portid_t *portid, unsigned set,
> if (do_vendor(portid, srcport, IB_MLX_VENDOR_CLASS,
> set ? IB_MAD_METHOD_SET : IB_MAD_METHOD_GET,
> IB_MLX_IS3_CONFIG_SPACE_ACCESS, 2 << 22 | records << 16,
> - cs))
> - IBEXIT("cannot %s config space records", set ? "set" : "get");
> -
> + cs)) {
> + fprintf(stderr,"cannot %s config space records\n", set ? "set" : "get");
> + return -1;
> + }
> for (i = 0; i < records; i++) {
> printf("Config space record at 0x%x: 0x%x\n",
> ntohl(cs->record[i].address),
> ntohl(cs->record[i].data & cs->record[i].mask));
> }
> + return 0;
> }
>
> -static void counter_groups_info(ib_portid_t * portid, int port)
> +static int counter_groups_info(ib_portid_t * portid, int port)
> {
> char buf[1024];
> is4_counter_group_info_t *cg_info;
> @@ -213,15 +216,17 @@ static void counter_groups_info(ib_portid_t * portid, int port)
> /* Counter Group Info */
> memset(&buf, 0, sizeof(buf));
> if (do_vendor(portid, srcport, IB_MLX_VENDOR_CLASS, IB_MAD_METHOD_GET,
> - IB_MLX_IS4_COUNTER_GROUP_INFO, port, buf))
> - IBEXIT("counter group info query");
> -
> + IB_MLX_IS4_COUNTER_GROUP_INFO, port, buf)) {
> + fprintf(stderr,"counter group info query failure\n");
> + return -1;
> + }
> cg_info = (is4_counter_group_info_t *) & buf;
> num_cg = cg_info->num_of_counter_groups;
> printf("counter_group_info:\n");
> printf("%d counter groups\n", num_cg);
> for (i = 0; i < num_cg; i++)
> printf("group%d mask %#x\n", i, ntohl(cg_info->group_masks[i]));
> + return 0;
> }
>
> /* Group0 counter config values */
> @@ -236,7 +241,7 @@ static void counter_groups_info(ib_portid_t * portid, int port)
>
> static int cg0, cg1;
>
> -static void config_counter_groups(ib_portid_t * portid, int port)
> +static int config_counter_groups(ib_portid_t * portid, int port)
> {
> char buf[1024];
> is4_config_counter_groups_t *cg_config;
> @@ -251,15 +256,19 @@ static void config_counter_groups(ib_portid_t * portid, int port)
> cg_config->group_selects[1].group_select = (uint8_t) cg1;
>
> if (do_vendor(portid, srcport, IB_MLX_VENDOR_CLASS, IB_MAD_METHOD_SET,
> - IB_MLX_IS4_CONFIG_COUNTER_GROUP, port, buf))
> - IBEXIT("config counter group set");
> -
> + IB_MLX_IS4_CONFIG_COUNTER_GROUP, port, buf)) {
> + fprintf(stderr, "config counter group set failure\n");
> + return -1;
> + }
> /* get config counter groups */
> memset(&buf, 0, sizeof(buf));
>
> if (do_vendor(portid, srcport, IB_MLX_VENDOR_CLASS, IB_MAD_METHOD_GET,
> - IB_MLX_IS4_CONFIG_COUNTER_GROUP, port, buf))
> - IBEXIT("config counter group query");
> + IB_MLX_IS4_CONFIG_COUNTER_GROUP, port, buf)) {
> + fprintf(stderr, "config counter group query failure\n");
> + return -1;
> + }
> + return 0;
> }
>
> static int general_info, xmit_wait, counter_group_info, config_counter_group;
> @@ -364,20 +373,26 @@ int main(int argc, char **argv)
>
> if (argc) {
> if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
> - ibd_dest_type, ibd_sm_id, srcport) < 0)
> + ibd_dest_type, ibd_sm_id, srcport) < 0) {
> + mad_rpc_close_port(srcport);
> IBEXIT("can't resolve destination port %s", argv[0]);
> + }
> } else {
> - if (resolve_self(ibd_ca, ibd_ca_port, &portid, &port, 0) < 0)
> + if (resolve_self(ibd_ca, ibd_ca_port, &portid, &port, 0) < 0) {
> + mad_rpc_close_port(srcport);
> IBEXIT("can't resolve self port %s", argv[0]);
> + }
> }
>
> if (counter_group_info) {
> counter_groups_info(&portid, port);
> + mad_rpc_close_port(srcport);
> exit(0);
> }
>
> if (config_counter_group) {
> config_counter_groups(&portid, port);
> + mad_rpc_close_port(srcport);
> exit(0);
> }
>
> @@ -388,6 +403,7 @@ int main(int argc, char **argv)
> if (write_cs_records)
> do_config_space_records(&portid, 1, &write_cs,
> write_cs_records);
> + mad_rpc_close_port(srcport);
> exit(0);
> }
>
> @@ -395,23 +411,27 @@ int main(int argc, char **argv)
> /* but vendors change the VendorId so how know for sure ? */
> /* Only General Info and Port Xmit Wait Counters */
> /* queries are currently supported */
> - if (!general_info && !xmit_wait)
> + if (!general_info && !xmit_wait) {
> + mad_rpc_close_port(srcport);
> IBEXIT("at least one of -N and -w must be specified");
> -
> + }
> /* Would need a list of these and it might not be complete */
> /* so for right now, punt on this */
>
> /* vendor ClassPortInfo is required attribute if class supported */
> memset(&buf, 0, sizeof(buf));
> if (do_vendor(&portid, srcport, IB_MLX_VENDOR_CLASS, IB_MAD_METHOD_GET,
> - CLASS_PORT_INFO, 0, buf))
> + CLASS_PORT_INFO, 0, buf)) {
> + mad_rpc_close_port(srcport);
> IBEXIT("classportinfo query");
> -
> + }
> memset(&buf, 0, sizeof(buf));
> gi_is3 = (is3_general_info_t *) &buf;
> if (do_vendor(&portid, srcport, IB_MLX_VENDOR_CLASS, IB_MAD_METHOD_GET,
> - IB_MLX_IS3_GENERAL_INFO, 0, gi_is3))
> + IB_MLX_IS3_GENERAL_INFO, 0, gi_is3)) {
> + mad_rpc_close_port(srcport);
> IBEXIT("generalinfo query");
> + }
>
> if (is_ext_fw_info_supported(ntohs(gi_is3->hw_info.device_id))) {
> gi_is4 = (is4_general_info_t *) &buf;
> @@ -452,10 +472,11 @@ int main(int argc, char **argv)
> is3_config_space_t *cs;
> unsigned i;
>
> - if (ntohs(gi_is3->hw_info.device_id) != IS3_DEVICE_ID)
> + if (ntohs(gi_is3->hw_info.device_id) != IS3_DEVICE_ID) {
> + mad_rpc_close_port(srcport);
> IBEXIT("Unsupported device ID 0x%x",
> ntohs(gi_is3->hw_info.device_id));
> -
> + }
> memset(&buf, 0, sizeof(buf));
> /* Set record addresses for each port */
> cs = (is3_config_space_t *) & buf;
> @@ -464,9 +485,10 @@ int main(int argc, char **argv)
> htonl(IB_MLX_IS3_PORT_XMIT_WAIT + ((i + 1) << 12));
> if (do_vendor(&portid, srcport, IB_MLX_VENDOR_CLASS,
> IB_MAD_METHOD_GET, IB_MLX_IS3_CONFIG_SPACE_ACCESS,
> - 2 << 22 | 16 << 16, cs))
> + 2 << 22 | 16 << 16, cs)) {
> + mad_rpc_close_port(srcport);
> IBEXIT("vendstat");
> -
> + }
> for (i = 0; i < 16; i++)
> if (cs->record[i].data) /* PortXmitWait is 32 bit counter */
> printf("Port %d: PortXmitWait 0x%x\n", i + 4, ntohl(cs->record[i].data)); /* port 4 is first port */
> @@ -480,8 +502,10 @@ int main(int argc, char **argv)
> htonl(IB_MLX_IS3_PORT_XMIT_WAIT + ((i + 17) << 12));
> if (do_vendor(&portid, srcport, IB_MLX_VENDOR_CLASS,
> IB_MAD_METHOD_GET, IB_MLX_IS3_CONFIG_SPACE_ACCESS,
> - 2 << 22 | 8 << 16, cs))
> + 2 << 22 | 8 << 16, cs)) {
> + mad_rpc_close_port(srcport);
> IBEXIT("vendstat");
> + }
>
> for (i = 0; i < 8; i++)
> if (cs->record[i].data) /* PortXmitWait is 32 bit counter */
> --
> 1.7.8.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
prev parent reply other threads:[~2015-04-22 15:26 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-21 14:01 [PATCH infiniband-diags] vendstat: mad_rpc_close_port not called in corner cases Hal Rosenstock
[not found] ` <5536584C.8070505-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2015-04-22 15:26 ` ira.weiny [this message]
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=20150422152649.GA7601@phlsvsds.ph.intel.com \
--to=ira.weiny-ral2jqcrhueavxtiumwx3w@public.gmane.org \
--cc=hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org \
--cc=linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=vladimirk-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \
/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.