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

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

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