| Message ID | 1477693948-14419-1-git-send-email-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
Message-id: 1477693948-14419-1-git-send-email-jsnow@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
55c3ba0 atapi: classify read_cd as conditionally returning data
=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/1: ...
ERROR: space required before the open parenthesis '('
#66: FILE: hw/ide/atapi.c:1060:
+ switch(transfer_request) {
total: 1 errors, 0 warnings, 78 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Am 29.10.2016 um 00:32 hat John Snow geschrieben: > For the purposes of byte_count_limit verification, add a new flag that > identifies read_cd as sometimes returning data, then check the BCL in > its command handler after we know that it will indeed return data. > > Reported-by: Hervé Poussineau <hpoussin@reactos.org> > Signed-off-by: John Snow <jsnow@redhat.com> Wouldn't it be useful to actually add the new flag to cmd_read_cd then? While at it, I would also split the patch into one for adding the flag and one for updating cmd_read_cd. Also, would it be hard to add a qtest case for this? Kevin
On 10/31/2016 10:06 AM, Kevin Wolf wrote: > Am 29.10.2016 um 00:32 hat John Snow geschrieben: >> For the purposes of byte_count_limit verification, add a new flag that >> identifies read_cd as sometimes returning data, then check the BCL in >> its command handler after we know that it will indeed return data. >> >> Reported-by: Hervé Poussineau <hpoussin@reactos.org> >> Signed-off-by: John Snow <jsnow@redhat.com> > > Wouldn't it be useful to actually add the new flag to cmd_read_cd then? > While at it, I would also split the patch into one for adding the flag > and one for updating cmd_read_cd. > As pointed out by Herve, no -- sorry. > Also, would it be hard to add a qtest case for this? > I could add it to AHCI pretty easily. > Kevin >
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 6189675..f26e3f4 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s, return 8; /* We wrote to 4 extra bytes from the header */ } +/* + * Before transferring data or otherwise signalling acceptance of a command + * marked CONDDATA, we must check the validity of the byte_count_limit. + */ +static bool validate_bcl(IDEState *s) +{ + /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently 0) */ + if (s->atapi_dma || atapi_byte_count_limit(s)) { + return true; + } + + /* TODO: Move abort back into core.c and introduce proper error flow between + * ATAPI layer and IDE core layer */ + ide_abort_command(s); + return false; +} + static void cmd_get_event_status_notification(IDEState *s, uint8_t *buf) { @@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t* buf) return; } - transfer_request = buf[9]; - switch(transfer_request & 0xf8) { - case 0x00: + transfer_request = buf[9] & 0xf8; + if (transfer_request == 0x00) { /* nothing */ ide_atapi_cmd_ok(s); - break; + return; + } + + /* Check validity of BCL before transferring data */ + if (!validate_bcl(s)) { + return; + } + + switch(transfer_request) { case 0x10: /* normal read */ ide_atapi_cmd_read(s, lba, nb_sectors, 2048); @@ -1266,6 +1290,14 @@ enum { * See ATA8-ACS3 "7.21.5 Byte Count Limit" */ NONDATA = 0x04, + + /* + * CONDDATA implies a command that transfers data only conditionally based + * on the presence of suboptions. It should be exempt from the BCL check at + * command validation time, but it needs to be checked at the command + * handler level instead. + */ + CONDDATA = 0x08, }; static const struct AtapiCmd { @@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s) return; } - /* Nondata commands permit the byte_count_limit to be 0. + /* Commands that don't transfer DATA permit the byte_count_limit to be 0. * If this is a data-transferring PIO command and BCL is 0, * we abort at the /ATA/ level, not the ATAPI level. * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */ - if (cmd->handler && !(cmd->flags & NONDATA)) { - /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */ - if (!(atapi_byte_count_limit(s) || s->atapi_dma)) { - /* TODO: Move abort back into core.c and make static inline again */ - ide_abort_command(s); + if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) { + if (!validate_bcl(s)) { return; } }
For the purposes of byte_count_limit verification, add a new flag that identifies read_cd as sometimes returning data, then check the BCL in its command handler after we know that it will indeed return data. Reported-by: Hervé Poussineau <hpoussin@reactos.org> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/atapi.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-)