Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function

[Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe>]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Alexander

sorry if I wasn't clear enough.  I wasn't arguing about your patch (which is
correct) but was trying to explain why the old implementation was done the way
it was, in an (obviously excessive) effort to prevent a buffer overflow for the
response (which used to be hardcoded to 32 bytes regardless of the size of the
buffer) as shown by :

  http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161

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

[-- Attachment #2: kvm-69-qemu-ide-dvdrom.patch --]
[-- Type: text/plain, Size: 3610 bytes --]

--- kvm-69/qemu/hw/ide.c        2008-05-12 04:30:43.000000000 -0700
+++ kvm-69/qemu/hw/ide.c        2008-05-25 01:44:27.000000000 -0700
@@ -1716,6 +1716,7 @@
     case GPCMD_GET_CONFIGURATION:
         {
             uint32_t len;
+            uint8_t index = 0;
 
             /* only feature 0 is supported */
             if (packet[2] != 0 || packet[3] != 0) {
@@ -1726,41 +1727,40 @@
 
             /* 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)
-             *
-             *      Only a problem if the feature/profiles grow exponentially.
-             */
-            if (max_len > 512) /* XXX: assume 1 sector */
-                max_len = 512;
-
-            memset(buf, 0, max_len);
-            /* 
-             * the number of sectors from the media tells us which profile
-             * to use as current.  0 means there is no media
-             *
-             * 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))
-                    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);
-            }
-            cpu_to_ube32(buf, len - 4); /* data length */
+            if (max_len > 0) {
+                /*
+                 * 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 
+                 */
+                if (max_len > 512) /* XXX: assume 1 sector */
+                    max_len = 512;
+
+                memset(buf, 0, max_len);
+                /* 
+                 * the number of sectors from the media tells us which profile
+                 * to use as current.  0 means there is no media
+                 *
+                 * 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)
+                        cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
+                    else
+                        cpu_to_ube16(buf + 6, 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 */
+            } else
+                len = 0;
 
             ide_atapi_cmd_reply(s, len, max_len);
             break;