[openib-general] [PATCH] opensm: strict osm_log arguments/format check

Yevgeny Kliteynik kliteyn at dev.mellanox.co.il
Thu Nov 2 05:38:47 PST 2006


Hi Sasha.

Good catch with those missing arguments. 
One question: in several places you used cl_hton64() to print guid.
Shouldn't there be cl_ntoh64() instead?

...and yes, I know that these two functions are actually the same macro :)

Thanks

-- Yevgeny


Sasha Khapyorsky wrote:
> This adds gcc attribute to osm_log() which causes the compiler to check
> argument types against a format string. And also there are related fixes
> in osm_log() usage in opensm and osmtest.
> 
> Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
> ---
>  osm/include/opensm/osm_log.h         |    8 +++++++-
>  osm/libvendor/osm_vendor_ibumad_sa.c |    2 +-
>  osm/opensm/main.c                    |    3 ++-
>  osm/opensm/osm_pkey_mgr.c            |    1 +
>  osm/opensm/osm_port_info_rcv.c       |    5 +++--
>  osm/opensm/osm_sa_informinfo.c       |    4 ++--
>  osm/opensm/osm_sa_link_record.c      |    8 ++++----
>  osm/opensm/osm_sa_mad_ctrl.c         |    3 ++-
>  osm/opensm/osm_sa_response.c         |    2 +-
>  osm/opensm/osm_sm_state_mgr.c        |    3 ++-
>  osm/opensm/osm_sminfo_rcv.c          |    9 +++++----
>  osm/opensm/osm_state_mgr.c           |    8 ++++----
>  osm/osmtest/osmt_multicast.c         |   12 +++++++-----
>  osm/osmtest/osmt_service.c           |    6 +++---
>  osm/osmtest/osmtest.c                |    8 ++++----
>  15 files changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/osm/include/opensm/osm_log.h b/osm/include/opensm/osm_log.h
> index 62f3a0c..2b24886 100644
> --- a/osm/include/opensm/osm_log.h
> +++ b/osm/include/opensm/osm_log.h
> @@ -60,6 +60,12 @@
>  #include <iba/ib_types.h>
>  #include <stdio.h>
>  
> +#ifdef __GNUC__
> +#define STRICT_OSM_LOG_FORMAT __attribute__((format(printf, 3, 4)))
> +#else
> +#define STRICT_OSM_LOG_FORMAT
> +#endif
> +
>  #ifdef __cplusplus
>  #  define BEGIN_C_DECLS extern "C" {
>  #  define END_C_DECLS   }
> @@ -374,7 +380,7 @@ void
>  osm_log(
>  	IN osm_log_t* const p_log,
>  	IN const osm_log_level_t verbosity,
> -	IN const char *p_str, ... );
> +	IN const char *p_str, ... ) STRICT_OSM_LOG_FORMAT;
>  
>  void
>  osm_log_raw(
> diff --git a/osm/libvendor/osm_vendor_ibumad_sa.c b/osm/libvendor/osm_vendor_ibumad_sa.c
> index 7fd0655..7c4a2f7 100644
> --- a/osm/libvendor/osm_vendor_ibumad_sa.c
> +++ b/osm/libvendor/osm_vendor_ibumad_sa.c
> @@ -853,7 +853,7 @@ osmv_query_sa(
>      if ( p_mpr_req->sgid_count + p_mpr_req->dgid_count > IB_MULTIPATH_MAX_GIDS )
>      {
>        osm_log( p_log, OSM_LOG_ERROR,
> -               "osmv_query_sa DBG:001 MULTIPATH_REC ",
> +               "osmv_query_sa DBG:001 MULTIPATH_REC "
>                 "SGID count %d DGID count %d max count %d\n",
>                  p_mpr_req->sgid_count, p_mpr_req->dgid_count,
>                  IB_MULTIPATH_MAX_GIDS );
> diff --git a/osm/opensm/main.c b/osm/opensm/main.c
> index 729702a..752b546 100644
> --- a/osm/opensm/main.c
> +++ b/osm/opensm/main.c
> @@ -460,7 +460,8 @@ parse_ignore_guids_file(IN char *guids_f
>    {
>      osm_log( &p_osm->log, OSM_LOG_ERROR,
>               "parse_ignore_guids_file: ERR 0601: "
> -             "Unable to open ignore guids file (%s)\n" );
> +             "Unable to open ignore guids file (%s)\n",
> +             guids_file_name );
>      status = IB_ERROR;
>      goto Exit;
>    }
> diff --git a/osm/opensm/osm_pkey_mgr.c b/osm/opensm/osm_pkey_mgr.c
> index f2cb221..735dc14 100644
> --- a/osm/opensm/osm_pkey_mgr.c
> +++ b/osm/opensm/osm_pkey_mgr.c
> @@ -139,6 +139,7 @@ pkey_mgr_process_physical_port(
>  	       "pkey_mgr_process_physical_port: ERR 0503: "
>  	       "Failed to obtain P_Key 0x%04x block and index for node "
>  	       "0x%016" PRIx64 " port %u\n",
> +	       ib_pkey_get_base( pkey ),
>  	       cl_ntoh64( osm_node_get_node_guid( p_node ) ),
>  	       osm_physp_get_port_num( p_physp ) );
>        return;
> diff --git a/osm/opensm/osm_port_info_rcv.c b/osm/opensm/osm_port_info_rcv.c
> index 95112dc..f6d3595 100644
> --- a/osm/opensm/osm_port_info_rcv.c
> +++ b/osm/opensm/osm_port_info_rcv.c
> @@ -724,8 +724,9 @@ osm_pi_rcv_process(
>    {
>      osm_log( p_rcv->p_log, OSM_LOG_VERBOSE,
>               "osm_pi_rcv_process: "
> -             "Got light sweep response from remote port of parent node GUID = 0x%" PRIx64
> -             " port = %u, Commencing heavy sweep\n",
> +             "Got light sweep response from remote port of parent node "
> +             "GUID = 0x%" PRIx64 " port = 0x%016" PRIx64
> +             ", Commencing heavy sweep\n",
>               cl_ntoh64( node_guid ),
>               cl_ntoh64( port_guid ) );
>      osm_state_mgr_process( p_rcv->p_state_mgr,
> diff --git a/osm/opensm/osm_sa_informinfo.c b/osm/opensm/osm_sa_informinfo.c
> index 69dca1d..da96d35 100644
> --- a/osm/opensm/osm_sa_informinfo.c
> +++ b/osm/opensm/osm_sa_informinfo.c
> @@ -163,8 +163,8 @@ __validate_ports_access_rights(
>      {
>        osm_log( p_rcv->p_log, OSM_LOG_ERROR,
>                 "__validate_ports_access_rights: ERR 4301: "
> -               "Invalid port guid: 0x%016\n",
> -               portguid );
> +               "Invalid port guid: 0x%016" PRIx64 "\n",
> +               cl_hton64(portguid) );
>        valid = FALSE;
>        goto Exit;
>      }
> diff --git a/osm/opensm/osm_sa_link_record.c b/osm/opensm/osm_sa_link_record.c
> index 751023f..0ca9092 100644
> --- a/osm/opensm/osm_sa_link_record.c
> +++ b/osm/opensm/osm_sa_link_record.c
> @@ -145,10 +145,10 @@ __osm_lr_rcv_build_physp_link(
>      osm_log( p_rcv->p_log, OSM_LOG_ERROR,
>               "__osm_lr_rcv_build_physp_link: ERR 1801: "
>               "Unable to acquire link record\n"
> -             "\t\t\t\tFrom port 0x%\n"
> -             "\t\t\t\tTo port   0x%\n"
> -             "\t\t\t\tFrom lid  0x%\n"
> -             "\t\t\t\tTo lid    0x%\n",
> +             "\t\t\t\tFrom port 0x%u\n"
> +             "\t\t\t\tTo port   0x%u\n"
> +             "\t\t\t\tFrom lid  0x%u\n"
> +             "\t\t\t\tTo lid    0x%u\n",
>               from_port, to_port,
>               cl_ntoh16(from_lid),
>               cl_ntoh16(to_lid) );
> diff --git a/osm/opensm/osm_sa_mad_ctrl.c b/osm/opensm/osm_sa_mad_ctrl.c
> index cd896b6..208f0d2 100644
> --- a/osm/opensm/osm_sa_mad_ctrl.c
> +++ b/osm/opensm/osm_sa_mad_ctrl.c
> @@ -132,7 +132,8 @@ __osm_sa_mad_ctrl_process(
>               "__osm_sa_mad_ctrl_process: "
>               /*             "Responding BUSY status since the dispatcher is already"*/
>               "Dropping MAD since the dispatcher is already"
> -	     " overloaded with %u messages and queue time of:%u[msec]\n",
> +	     " overloaded with %u messages and queue time of:"
> +	     "%" PRIu64 "[msec]\n",
>  	     num_messages, last_dispatched_msg_queue_time_msec );
>  
>      /* send a busy response */
> diff --git a/osm/opensm/osm_sa_response.c b/osm/opensm/osm_sa_response.c
> index db36ea2..27f4e9d 100644
> --- a/osm/opensm/osm_sa_response.c
> +++ b/osm/opensm/osm_sa_response.c
> @@ -117,7 +117,7 @@ osm_sa_send_error(
>    if (osm_exit_flag)
>    {
>      osm_log( p_resp->p_log, OSM_LOG_DEBUG,
> -             "osm_sa_send_error: ",
> +             "osm_sa_send_error: "
>               "Ignoring requested send after exit\n" );
>      goto Exit;
>    }
> diff --git a/osm/opensm/osm_sm_state_mgr.c b/osm/opensm/osm_sm_state_mgr.c
> index aadc43a..1ba5eda 100644
> --- a/osm/opensm/osm_sm_state_mgr.c
> +++ b/osm/opensm/osm_sm_state_mgr.c
> @@ -247,7 +247,8 @@ __osm_sm_state_mgr_send_master_sm_info_r
>     {
>        osm_log( p_sm_mgr->p_log, OSM_LOG_ERROR,
>                 "__osm_sm_state_mgr_send_master_sm_info_req: ERR 3203: "
> -               "No port object for GUID 0x%X\n", p_sm_mgr->master_guid );
> +               "No port object for GUID 0x%016" PRIx64 "\n",
> +               cl_hton64(p_sm_mgr->master_guid) );
>        goto Exit;
>     }
>  
> diff --git a/osm/opensm/osm_sminfo_rcv.c b/osm/opensm/osm_sminfo_rcv.c
> index 825b18b..7657e97 100644
> --- a/osm/opensm/osm_sminfo_rcv.c
> +++ b/osm/opensm/osm_sminfo_rcv.c
> @@ -402,8 +402,8 @@ __osm_sminfo_rcv_process_set_request(
>      osm_log( p_rcv->p_log, OSM_LOG_VERBOSE,
>               "__osm_sminfo_rcv_process_set_request: "
>               "Received a STANDBY signal. Updating "
> -             "sm_state_mgr master_guid: 0x%X\n",
> -             p_rcv_smi->guid );
> +             "sm_state_mgr master_guid: 0x%016" PRIx64 "\n",
> +             cl_hton64(p_rcv_smi->guid) );
>      p_rcv->p_sm_state_mgr->master_guid = p_rcv_smi->guid;
>    }
>  
> @@ -482,8 +482,9 @@ __osm_sminfo_rcv_process_get_sm(
>          /*  we will poll it - as long as it lives - we should be in Standby. */
>          osm_log( p_rcv->p_log, OSM_LOG_VERBOSE,
>                   "__osm_sminfo_rcv_process_get_sm: "
> -                 "Found higher SM. Updating sm_state_mgr master_guid: 0x%X\n",
> -                 p_sm->p_port->guid );
> +                 "Found higher SM. Updating sm_state_mgr master_guid:"
> +                 " 0x%016" PRIx64 "\n",
> +                 cl_hton64(p_sm->p_port->guid) );
>          p_rcv->p_sm_state_mgr->master_guid = p_sm->p_port->guid;
>        }
>        break;
> diff --git a/osm/opensm/osm_state_mgr.c b/osm/opensm/osm_state_mgr.c
> index 28e0c4c..ad22d9e 100644
> --- a/osm/opensm/osm_state_mgr.c
> +++ b/osm/opensm/osm_state_mgr.c
> @@ -481,7 +481,7 @@ __osm_state_mgr_signal_warning(
>  {
>     osm_log( p_mgr->p_log, OSM_LOG_VERBOSE,
>              "__osm_state_mgr_signal_warning: "
> -            "Invalid signal %s(%d) in state %s\n",
> +            "Invalid signal %s(%lu) in state %s\n",
>              osm_get_sm_signal_str( signal ),
>              signal, osm_get_sm_state_str( p_mgr->state ) );
>  }
> @@ -500,7 +500,7 @@ __osm_state_mgr_signal_error(
>     else
>        osm_log( p_mgr->p_log, OSM_LOG_ERROR,
>                 "__osm_state_mgr_signal_error: ERR 3303: "
> -               "Invalid signal %s(%d) in state %s\n",
> +               "Invalid signal %s(%lu) in state %s\n",
>                 osm_get_sm_signal_str( signal ),
>                 signal, osm_get_sm_state_str( p_mgr->state ) );
>  }
> @@ -1480,8 +1480,8 @@ __osm_state_mgr_exists_other_master_sm(
>        {
>           osm_log( p_mgr->p_log, OSM_LOG_VERBOSE,
>                    "__osm_state_mgr_exists_other_master_sm: "
> -                  "Found remote master SM with guid:0x%X\n",
> -                  p_sm->smi.guid );
> +                  "Found remote master SM with guid:0x%016" PRIx64 "\n",
> +                  cl_hton64(p_sm->smi.guid) );
>           p_sm_res = p_sm;
>           goto Exit;
>        }
> diff --git a/osm/osmtest/osmt_multicast.c b/osm/osmtest/osmt_multicast.c
> index 33a4f47..19f9d37 100644
> --- a/osm/osmtest/osmt_multicast.c
> +++ b/osm/osmtest/osmt_multicast.c
> @@ -1885,8 +1885,9 @@ osmt_run_mcast_flow( IN osmtest_t * cons
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmt_run_mcast_flow: ERR 0209: "
> -             "Validating MGID failed. MGID:0x%016" PRIx64 "\n",
> -             p_mc_res->mgid
> +             "Validating MGID failed. MGID:0x%016" PRIx64 ":%016" PRIx64 "\n",
> +             cl_ntoh64( p_mc_res->mgid.unicast.prefix ),
> +             cl_ntoh64( p_mc_res->mgid.unicast.interface_id )
>               );
>      status = IB_ERROR;
>      goto Exit;
> @@ -2044,8 +2045,9 @@ osmt_run_mcast_flow( IN osmtest_t * cons
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmt_run_mcast_flow: ERR 0212: "
> -             "Validating MGID failed. MGID:0x%016" PRIx64 "\n",
> -             p_mc_res->mgid
> +             "Validating MGID failed. MGID:0x%016" PRIx64 ":%016" PRIx64 "\n",
> +             cl_ntoh64( p_mc_res->mgid.unicast.prefix ),
> +             cl_ntoh64( p_mc_res->mgid.unicast.interface_id )
>               );
>      status = IB_ERROR;
>      goto Exit;
> @@ -3345,7 +3347,7 @@ osmt_run_mcast_flow( IN osmtest_t * cons
>    /* Delete all MCG that are not of IPoIB */
>    osm_log( &p_osmt->log, OSM_LOG_INFO,
>             "osmt_run_mcast_flow : "
> -           "Cleanup all MCG that are not IPoIB...\n", cnt );
> +           "Cleanup all MCG that are not IPoIB...\n" );
>  
>    p_mgrp_mlid_tbl = &p_osmt->exp_subn.mgrp_mlid_tbl;
>    p_mgrp = (osmtest_mgrp_t*)cl_qmap_head( p_mgrp_mlid_tbl );
> diff --git a/osm/osmtest/osmt_service.c b/osm/osmtest/osmt_service.c
> index ec9a39e..ab95fec 100644
> --- a/osm/osmtest/osmt_service.c
> +++ b/osm/osmtest/osmt_service.c
> @@ -1559,7 +1559,7 @@ osmt_run_service_records_flow( IN osmtes
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmt_run_service_records_flow: ERR 4A20: "
> -             "Found service: id: 0x%016 " PRIx64 
> +             "Found service: id: 0x%016" PRIx64 " "
>               "that is invalid\n",
>               id[7] );
>      status = IB_ERROR;
> @@ -1573,7 +1573,7 @@ osmt_run_service_records_flow( IN osmtes
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmt_run_service_records_flow: ERR 4A21: "
> -             "Fail to find service: id: 0x%016 " PRIx64
> +             "Fail to find service: id: 0x%016" PRIx64 " "
>               "name: %s\n",
>               id[0],
>               (char*)service_name[0] );
> @@ -1588,7 +1588,7 @@ osmt_run_service_records_flow( IN osmtes
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmt_run_service_records_flow: ERR 4A22: "
> -             "Fail to find service: id: 0x%016 " PRIx64
> +             "Fail to find service: id: 0x%016" PRIx64 " "
>               "name: %s\n",
>               id[5],
>               (char*)service_name[6] );
> diff --git a/osm/osmtest/osmtest.c b/osm/osmtest/osmtest.c
> index 92a4190..a35e0c5 100644
> --- a/osm/osmtest/osmtest.c
> +++ b/osm/osmtest/osmtest.c
> @@ -2787,7 +2787,8 @@ osmtest_create_inventory_file( IN osmtes
>    {
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmtest_create_inventory_file: ERR 0079: "
> -             "Unable to open inventory file (%s)\n" );
> +             "Unable to open inventory file (%s)\n",
> +             p_osmt->opt.file_name );
>      status = IB_ERROR;
>      goto Exit;
>    }
> @@ -3356,7 +3357,7 @@ osmtest_validate_path_data( IN osmtest_t
>      osm_log( &p_osmt->log, OSM_LOG_ERROR,
>               "osmtest_validate_path_data: ERR 0012: "
>               "PKEY mismatch on path SLID 0x%X to DLID 0x%X\n"
> -             "\t\t\t\tExpected 0x%X, received 0x%X\n",
> +             "\t\t\t\tExpected 0x%" PRIx64 ", received 0x%" PRIx64 "\n",
>               cl_ntoh16( p_path->rec.slid ),
>               cl_ntoh16( p_path->rec.dlid ),
>               cl_ntoh64( p_path->rec.pkey ), cl_ntoh64( p_rec->pkey ) );
> @@ -7165,8 +7166,7 @@ osmtest_bind( IN osmtest_t * p_osmt,
>      {
>        osm_log( &p_osmt->log, OSM_LOG_ERROR,
>                 "osmtest_bind: ERR 0135: "
> -               "No local ports.  Unable to proceed\n",
> -               ib_get_err_str( status ) );
> +               "No local ports.  Unable to proceed\n" );
>        goto Exit;
>      }
>      guid = attr_array[port_index].port_guid;




More information about the general mailing list