On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> There is code checking s->end_transfer_func and it was not taught
> about ide_transfer_cancel. We can just use ide_transfer_stop because
> s->end_transfer_func is only ever called in the DRQ phase: after
> ide_transfer_cancel, the value of s->end_transfer_func is only used
> as a marker and never used to actually invoke the function.
>
I think you're right, but I think it's also pretty non-obvious to a
reader that a callback might be used in this way.
"John, it's your component dude, why don't you fix that?"
Err, I'm afraid of disturbing the delicate balance of spaghetti code.
*cough*
Anyway, a little comment explaining that we don't actually *expect* that
callback to actually get called would probably answer a question or two
before they arise.
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> hw/ide/core.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c4710a6f55..447d9624df 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -553,10 +553,9 @@ static void ide_cmd_done(IDEState *s)
> }
> }
>
> -static void ide_transfer_halt(IDEState *s,
> - void(*end_transfer_func)(IDEState *))
> +static void ide_transfer_halt(IDEState *s)
> {
> - s->end_transfer_func = end_transfer_func;
> + s->end_transfer_func = ide_transfer_stop;
> s->data_ptr = s->io_buffer;
> s->data_end = s->io_buffer;
> s->status &= ~DRQ_STAT;
> @@ -564,7 +563,7 @@ static void ide_transfer_halt(IDEState *s,
>
> void ide_transfer_stop(IDEState *s)
> {
> - ide_transfer_halt(s, ide_transfer_stop);
> + ide_transfer_halt(s);
> if (s->bus->dma->ops->end_transfer) {
> s->bus->dma->ops->end_transfer(s->bus->dma);
> }
> @@ -573,7 +572,7 @@ void ide_transfer_stop(IDEState *s)
>
> static void ide_transfer_cancel(IDEState *s)
> {
> - ide_transfer_halt(s, ide_transfer_cancel);
> + ide_transfer_halt(s);
> }
>
> int64_t ide_get_sector(IDEState *s)
>
"LGTM"
