Re: [Qemu-devel] [PATCH 1/5] ide: Prohibit RESET on IDE drives

[Paolo Bonzini]

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;

Re: [Qemu-devel] [PATCH 1/5] ide: Prohibit RESET on IDE drives

[John Snow]

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?

Re: [Qemu-devel] [PATCH 1/5] ide: Prohibit RESET on IDE drives

[Laszlo Ersek]

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

[Qemu-devel] [PATCH 1/5] ide: Prohibit RESET on IDE drives

[John Snow]

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 
---
 hw/ide/core.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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);
-- 
2.4.3