On Tue, Jan 19, 2010 at 2:27 PM, Mike Christie <[email protected]> wrote:
> On 12/28/2009 05:10 AM, Erez Zilber wrote:
>>
>> diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
>> index 6188f43..ee4cfc4 100644
>> --- a/usr/iscsiadm.c
>> +++ b/usr/iscsiadm.c
>> @@ -94,7 +94,7 @@ static struct option const long_options[] =
>> {"help", no_argument, NULL, 'h'},
>> {NULL, 0, NULL, 0},
>> };
>> -static char *short_options = "RlVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:u:i:";
>> +static char *short_options = "RlVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:ui:";
>>
>> static void usage(int status)
>> {
>>
>
> Sorry the late reply, and thanks again for working on this.
>
>
> @@ -317,6 +328,8 @@ enum iscsi_param {
> ISCSI_PARAM_ISID,
> ISCSI_PARAM_INITIATOR_NAME,
>
> + ISCSI_PARAM_NOOP_THRESHOLD,
> +
> ISCSI_PARAM_TGT_RESET_TMO,
>
>
> ISCSI_PARAM_NOOP_THRESHOLD should be after ISCSI_PARAM_TGT_RESET_TMO. I
> think it got messed up when you updated a patch.
Done.
>
>
> +struct iscsi_noop_info {
> + struct timeval tv;
>
>
> Can you pass this between userspace and the kernel safely? The fields in
> there are longs, and I thought those could be different sizes if you had
> something like 32bit userspace and 64 kernels.
What do you suggest to do here?
>
>
>
> + unsigned int elapsed_time_msec;
> + short init;
> +};
>
>
> ping_delay = jiffies - conn->last_ping;
> +
> + /* Check if the ping has almost timed out */
> + if (ping_delay >=
> + (session->noop_threshold * (conn->ping_timeout * HZ)) /
>
> Does this handle jiffies rollover ok? If jiffies rolled over and it is 1 but
> last_ping was ULONG_MAX, then ping_delay is going to be very large.
Fixed.
>
>
>
> +static noop_op_type_e
> +str_to_noop_op(char *str)
> +{
> + noop_op_type_e op;
> +
> + if (!strcmp("dump", str))
> + op = NOOP_OP_DUMP;
> + else if (!strcmp("clear", str))
> + op = NOOP_OP_CLEAR;
> + else
> + op = NOOP_OP_NONE;
> +
> + return op;
> +}
>
> Should we make these names clear that it resets/dumps the noop data. If we
> added a scsi cmd timer option, then we probably want to be able to
> dump/reset one or the other. So rename dump to dump_noop and clear to
> clear_noop.
Fixed. We also need to replace str_to_noop_op with
str_to_debug_info_op (and change the enum accordingly).
>
>
>
> case MODE_NODE:
> if ((rc = verify_mode_params(argc, argv, "RsPIdmlSonvupTUL",
>
> This line needs a 'i'. It looks like you added the node compat support but
> you need to enable it with the 'i'. I added it when I tested your patch and
> it worked fine, so just added it when you send the patch.
>
>
OK
> Also what about debugfs? Did you think you would want the flexibility? Is
> that too many interfaces (sysfs, netlink, bsg and debugfs)?
I'm not very familiar with debugfs. What do you want to do with it?
Erez
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
