| Message ID | 1453179117-17909-2-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 19/01/2016 05:51, John Snow wrote: > + /* Only RESET is allowed to an ATAPI device while BSY and/or DRQ are set. */ > + if (s->status & (BUSY_STAT|DRQ_STAT)) { > + if (!(val == WIN_DEVICE_RESET) && (s->drive_kind == IDE_CD)) { I was going to complain about Pascal-ish parentheses, but actually I think there is a bug here; the expression just looks weird. Did you mean if (!(val == WIN_DEVICE_RESET && s->drive_kind == IDE_CD)) or equivalently applying de Morgan's law: if (s->drive_kind != IDE_CD || val != WIN_DEVICE_RESET) ? Paolo > + return;
On 01/19/2016 06:48 AM, Paolo Bonzini wrote: > > > On 19/01/2016 05:51, John Snow wrote: >> + /* Only RESET is allowed to an ATAPI device while BSY and/or DRQ are set. */ >> + if (s->status & (BUSY_STAT|DRQ_STAT)) { >> + if (!(val == WIN_DEVICE_RESET) && (s->drive_kind == IDE_CD)) { > > I was going to complain about Pascal-ish parentheses, but actually I > think there is a bug here; the expression just looks weird. > > Did you mean > > if (!(val == WIN_DEVICE_RESET && s->drive_kind == IDE_CD)) > > or equivalently applying de Morgan's law: > > if (s->drive_kind != IDE_CD || val != WIN_DEVICE_RESET) > > ? > > Paolo > >> + return; > ugh, yes, I typo'd. Thank you. If you're still up, which do you find more readable? The (!(A && B)) form or the (!A || !B) form?
On 01/19/16 18:04, John Snow wrote: > > > On 01/19/2016 06:48 AM, Paolo Bonzini wrote: >> >> >> On 19/01/2016 05:51, John Snow wrote: >>> + /* Only RESET is allowed to an ATAPI device while BSY and/or DRQ are set. */ >>> + if (s->status & (BUSY_STAT|DRQ_STAT)) { >>> + if (!(val == WIN_DEVICE_RESET) && (s->drive_kind == IDE_CD)) { >> >> I was going to complain about Pascal-ish parentheses, but actually I >> think there is a bug here; the expression just looks weird. >> >> Did you mean >> >> if (!(val == WIN_DEVICE_RESET && s->drive_kind == IDE_CD)) >> >> or equivalently applying de Morgan's law: >> >> if (s->drive_kind != IDE_CD || val != WIN_DEVICE_RESET) >> >> ? >> >> Paolo >> >>> + return; >> > > ugh, yes, I typo'd. Thank you. > > If you're still up, which do you find more readable? > The (!(A && B)) form or the (!A || !B) form? You didn't ask me, but that's no problem for me. :) The logical negation operator "!" has much-much stronger binding than the logical "and" and logical "or" ones. If you use the first form, !(A && B) it works, but spacetime will curl every time someone sees those parentheses overriding the nice n' tight binding of "!". So, for me, only the second form *exists* -- for me, the operand of the logical negation operator must always be as "indivisible" an expression as possible :) So, (!A || !B) without a doubt. Thanks Laszlo
diff --git a/hw/ide/core.c b/hw/ide/core.c index da3baab..ba33064 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1876,9 +1876,12 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) return; } - /* Only DEVICE RESET is allowed while BSY or/and DRQ are set */ - if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET) - return; + /* Only RESET is allowed to an ATAPI device while BSY and/or DRQ are set. */ + if (s->status & (BUSY_STAT|DRQ_STAT)) { + if (!(val == WIN_DEVICE_RESET) && (s->drive_kind == IDE_CD)) { + return; + } + } if (!ide_cmd_permitted(s, val)) { ide_abort_command(s);
This command is meant for ATAPI devices only, prohibit acknowledging it with a command aborted response when an IDE device is busy. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)