Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable

[Jeff Moyer <jmoyer@redhat.com>]

Ira Weiny <ira.weiny@intel.com> writes:

> On Mon, Oct 28, 2019 at 03:37:48PM -0400, Jeff Moyer wrote:
>> Ira Weiny <ira.weiny@intel.com> writes:
>> 
>> >> (Watching the unit test run fall into an infinite loop..) Nope, the
>> >> break is in the switch scope, the while loop needs the 'goto out'.
>> >> 
>> >> Yes this bit definitely needs to be refactored :)
>> >
>> > How about this patch instead?  Untested.
>> 
>> I'm not a fan of the looping with gotos.
>
> Me either... But... the logic here is not the same.
>
>>
>> I think separating out the
>> waiting for busy to its own function would make this more clear.
>> Looking more closely, there are other issues.  The timeout code looks at
>> the seconds, but ignores the fractions, so you could be off by almost an
>> entire second, there.
>
> For this operation that is probably not a big deal.  We should be waiting much
> longer than the operation should take anyway.
>
>>
>> It also doens't retry the sleep if interrupted.
>
> This could be an issue.
>
>> Finally, I find the variables names to be highly confusing.
>> 
>> I've decided not to fix those last two bugs just yet, but here's a patch
>> that shows the dirction I think it should go.  Compile-tested only for
>> now.  Let me know what you think.
>
> I thought about doing something similar but to make the logic the same it
> becomes a bit awkward.

[...]

>> +	wait_for_cmd_completion(cmd, fw, &start);
>
> wait_for_cmd_completion() does not call ndctl_cmd_submit()
>
> Now I find it odd that we need to resubmit the command but I assume the logic
> is correct.  Therefore we need to go back and call ndctl_cmd_submit() again.
>
> Or is this not required?

Ah.  Stupid mistake.  Yes, it definitely looks like the status query
command needs to be resubmitted, and that's the whole point of the
timeout between calls.  You can't ask too often.  ;-)

> anyway that is why I went ahead and used the goto...

I'll take another look.  Thanks for pointing out that obvious thinko.

-Jeff
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org