Re: [Qemu-devel] [PATCH 3/5] ide: do not set s->end_transfer_func to ide_transfer_cancel

[John Snow <jsnow@redhat.com>]

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 <pbonzini@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"