On Fri, 2017-06-02 at 17:02 +0000, Bart Van Assche wrote:
> 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.
>
Let's see. I'll try some failures and try to make it OOPs.
> > > +
> > > +/*
> > > + * 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.
>
Elegant huh..?
I guess we have a different idea of what elegance means then. ;)
> > > +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.
No need. I apply and fix this part up myself.
If it ends up being problematic, we can just drop it later.
--
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
