On Thu, 2017-06-01 at 21:31 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > [ ... ]
> > +static void target_append_str(char **str, const char *txt)
> > +{
> > + char *prev = *str;
> > +
> > + *str = *str ? kasprintf(GFP_ATOMIC, "%s,%s", *str, txt) :
> > + kstrdup(txt, GFP_ATOMIC);
> > + kfree(prev);
> > +}
>
> Let's avoid doing memory allocations, since the failure path it not
> handled at all.
The failure path is handled, namely by assigning NULL to *str. I am aware that
this will result in suppression of output if an out-of-memory condition is
triggered. But if that happens a kernel warning with call trace will appear
anyway in the kernel log so the user will be informed.
> > +
> > +/*
> > + * Convert a transport state bitmask into a string. The caller is
> > + * responsible for freeing the returned pointer.
> > + */
> > +static char *target_ts_to_str(u32 ts)
> > +{
> > + char *str = NULL;
> > +
> > + if (ts & CMD_T_ABORTED)
> > + target_append_str(&str, "aborted");
> > + if (ts & CMD_T_ACTIVE)
> > + target_append_str(&str, "active");
> > + if (ts & CMD_T_COMPLETE)
> > + target_append_str(&str, "complete");
> > + if (ts & CMD_T_SENT)
> > + target_append_str(&str, "sent");
> > + if (ts & CMD_T_STOP)
> > + target_append_str(&str, "stop");
> > + if (ts & CMD_T_FABRIC_STOP)
> > + target_append_str(&str, "fabric_stop");
> > +
> > + return str;
> > +}
> > +
>
> Yeah, find a way to do this without memory allocations for every state
> bit.
What's wrong about using memory allocations in debug code that is never called
from a hot path where performance matters? This is a much more elegant solution
than e.g. allocating a fixed size buffer that is either too large or too small.
> > +void target_show_cmd(const char *pfx, struct se_cmd *cmd)
> > +{
> > + char *ts_str = target_ts_to_str(cmd->transport_state);
> > + const u8 *cdb = cmd->t_task_cdb;
> > + struct se_tmr_req *tmf = cmd->se_tmr_req;
> > +
> > + if (!(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
> > + pr_debug("%scmd %#02x:%#02x with tag %#llx dir %s i_state %d t_state %s len %d refcnt %d transport_state %s\n",
> > + pfx, cdb[0], cdb[1], cmd->tag,
> > + data_dir_name(cmd->data_direction),
> > + cmd->se_tfo->get_cmd_state(cmd),
> > + cmd_state_name(cmd->t_state), cmd->data_length,
> > + kref_read(&cmd->cmd_kref), ts_str);
> > + else
> > + pr_debug("%stmf %s with tag %#llx ref_task_tag %#llx i_state %d t_state %s refcnt %d transport_state %s\n",
> > + pfx, target_tmf_name(tmf->function), cmd->tag,
> > + tmf->ref_task_tag, cmd->se_tfo->get_cmd_state(cmd),
> > + cmd_state_name(cmd->t_state),
> > + kref_read(&cmd->cmd_kref), ts_str);
> > + kfree(ts_str);
>
> Missing brackets for multi-line conditionals.
I will add braces around these statements.
Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
