On Mon, May 26, 2008 at 07:46:10PM +0200, Alexander Graf wrote:
On May 25, 2008, at 12:38 PM, Carlo Marcelo Arenas Belon wrote:
a slightly modified version of your patch (which I'd been using
against kvm
and validated correct with Linux and Solaris guests) attached.
the only difference, is that it handles explicitly the empty buffer
case and
cleans up the len calculation which shouldn't had been calculated
conditionally as you pointed out.
Carlo
<kvm-69-qemu-ide-dvdrom.patch>
Hum ... it might be a good idea to inline patches so commenting on
them is easier.
patch was attached inline just like you did with your original one
(except it
was using MIME type text/plain instead of text/x-patch). inlining it
below so it is easier to comment on.
You don't need to check for max_len > 0, because if max_len == 0 the
packet will be truncated back to 0 bytes by ide_atapi_cmd_reply(s,
len, max_len).
yes, the check is not needed but as I said was added so that it can
explicitly handle those cases for clarity and to save the work of
building
the response just to be discarded at the end. anyway, removed from
the last
version which should be also ready for commit.
Carlo
---
Index: hw/ide.c
===================================================================
--- hw/ide.c (revision 4589)
+++ hw/ide.c (working copy)
@@ -1715,6 +1715,7 @@
case GPCMD_GET_CONFIGURATION:
{
uint32_t len;
+ uint8_t index = 0;
/* only feature 0 is supported */
if (packet[2] != 0 || packet[3] != 0) {
@@ -1725,12 +1726,13 @@
/* XXX: could result in alignment problems in some
architectures */
max_len = ube16_to_cpu(packet + 7);
+
/*
- * XXX: avoid overflow for io_buffer if max_len is
bigger than the
- * size of that buffer (dimensioned to max number
of sectors
- * to transfer at once)
+ * XXX: avoid overflow for io_buffer if max_len is
bigger than
+ * the size of that buffer (dimensioned to max
number of
+ * sectors to transfer at once)
*
- * Only a problem if the feature/profiles grow
exponentially.
+ * Only a problem if the feature/profiles grow
*/
if (max_len > 512) /* XXX: assume 1 sector */
max_len = 512;
@@ -1743,22 +1745,17 @@
* XXX: fails to detect correctly DVDs with less data
burned
* than what a CD can hold
*/
- if ((s -> nb_sectors)) {
- if ((s -> nb_sectors > CD_MAX_SECTORS))
+ if (s -> nb_sectors) {
+ if (s -> nb_sectors > CD_MAX_SECTORS)
cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
else
cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
}
- len = 8; /* header completed */
- if (max_len > len) {
- uint8_t index = 0;
-
- buf[10] = 0x02 | 0x01; /* persistent and current */
- len += 4; /* header */
- len += ide_atapi_set_profile(buf, &index,
MMC_PROFILE_DVD_ROM);
- len += ide_atapi_set_profile(buf, &index,
MMC_PROFILE_CD_ROM);
- }
+ buf[10] = 0x02 | 0x01; /* persistent and current */
+ len = 12; /* headers: 8 + 4 */
+ len += ide_atapi_set_profile(buf, &index,
MMC_PROFILE_DVD_ROM);
+ len += ide_atapi_set_profile(buf, &index,
MMC_PROFILE_CD_ROM);
cpu_to_ube32(buf, len - 4); /* data length */
ide_atapi_cmd_reply(s, len, max_len);