[PATCH] pretty boot menu entry for cdrom drives

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 src/util.h  |  1 +
 src/boot.c  |  7 +++++++
 src/cdrom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/src/util.h b/src/util.h
index 7a23b518fc..6dd080f673 100644
--- a/src/util.h
+++ b/src/util.h
@@ -50,6 +50,7 @@ struct disk_op_s;
 int cdemu_process_op(struct disk_op_s *op);
 void cdrom_prepboot(void);
 int cdrom_boot(struct drive_s *drive_g);
+char *cdrom_media_info(struct drive_s *drive_g);

// clock.c
 void clock_setup(void);
diff --git a/src/boot.c b/src/boot.c
index ff705fd47f..80bcc13535 100644
--- a/src/boot.c
+++ b/src/boot.c
@@ -395,6 +395,13 @@ boot_add_hd(struct drive_s *drive, const char *desc, int prio)
 void
 boot_add_cd(struct drive_s *drive, const char *desc, int prio)
 {
+    if (GET_GLOBAL(PlatformRunningOn) & PF_QEMU) {
+        // We want short boot times.  But on physical hardware even
+        // the test unit ready can take several seconds.  So do media
+        // access on qemu only, where we know it will be fast.
+        char *extra = cdrom_media_info(drive);
+        desc = znprintf(MAXDESCSIZE, "%s (%s)", desc, extra);
+    }
     bootentry_add(IPL_TYPE_CDROM, defPrio(prio, DefaultCDPrio)
                   , (u32)drive, desc);
 }
diff --git a/src/cdrom.c b/src/cdrom.c
index 828fb3b842..b4c36622ae 100644
--- a/src/cdrom.c
+++ b/src/cdrom.c
@@ -274,3 +274,54 @@ cdrom_boot(struct drive_s *drive)

return 0;
 }
+
+// go figure some cdrom information, for a pretty boot menu entry.
+char*
+cdrom_media_info(struct drive_s *drive)
+{
+    ASSERT32FLAT();
+
+    struct disk_op_s dop;
+    memset(&dop, 0, sizeof(dop));
+    dop.drive_fl = drive;
+
+    int ret = scsi_is_ready(&dop);
+    if (ret)
+        return "empty";
+
+    // Read the Boot Record Volume Descriptor
+    u8 buffer[CDROM_SECTOR_SIZE];
+    dop.command = CMD_READ;
+    dop.lba = 0x11;
+    dop.count = 1;
+    dop.buf_fl = buffer;
+    ret = process_op(&dop);
+    if (ret)
+        return "read error";
+
+    // Is it bootable?
+    if (buffer[0])
+        return "not bootable";
+    if (strcmp((char*)&buffer[1], "CD001\001EL TORITO SPECIFICATION") != 0)
+        return "not bootable";
+
+    // Read the Primary Volume Descriptor
+    dop.command = CMD_READ;
+    dop.lba = 0x10;
+    dop.count = 1;
+    dop.buf_fl = buffer;
+    ret = process_op(&dop);
+    if (ret)
+        return "read error";
+
+    // Read volume id, trim trailing spaces
+    char *volume = znprintf(30, "%s", buffer + 40);
+    char *h = volume + strlen(volume);
+    while (h > volume) {
+        h--;
+        if (*h != ' ')
+            break;
+        *h = 0;
+    }
+    return volume;
+}

[Kevin O'Connor]

On Fri, Sep 21, 2018 at 09:48:01AM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
>
> src/util.h  |  1 +
>  src/boot.c  |  7 +++++++
>  src/cdrom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
>
> diff --git a/src/util.h b/src/util.h
> index 7a23b518fc..6dd080f673 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -50,6 +50,7 @@ struct disk_op_s;
>  int cdemu_process_op(struct disk_op_s *op);
>  void cdrom_prepboot(void);
>  int cdrom_boot(struct drive_s *drive_g);
> +char *cdrom_media_info(struct drive_s *drive_g);
>  
>  // clock.c
>  void clock_setup(void);
> diff --git a/src/boot.c b/src/boot.c
> index ff705fd47f..80bcc13535 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -395,6 +395,13 @@ boot_add_hd(struct drive_s *drive, const char *desc, int prio)
>  void
>  boot_add_cd(struct drive_s *drive, const char *desc, int prio)
>  {
> if (GET_GLOBAL(PlatformRunningOn) & PF_QEMU) {
>    // We want short boot times.  But on physical hardware even
>    // the test unit ready can take several seconds.  So do media
>    // access on qemu only, where we know it will be fast.
>    char *extra = cdrom_media_info(drive);
>    desc = znprintf(MAXDESCSIZE, "%s (%s)", desc, extra);
> }
> bootentry_add(IPL_TYPE_CDROM, defPrio(prio, DefaultCDPrio)
>               , (u32)drive, desc);
>
> }
> diff --git a/src/cdrom.c b/src/cdrom.c
> index 828fb3b842..b4c36622ae 100644
> --- a/src/cdrom.c
> +++ b/src/cdrom.c
> @@ -274,3 +274,54 @@ cdrom_boot(struct drive_s *drive)
>  
>      return 0;
>  }
>
> +// go figure some cdrom information, for a pretty boot menu entry.
> +char*
> +cdrom_media_info(struct drive_s *drive)
> +{
> ASSERT32FLAT();
> struct disk_op_s dop;
> memset(&dop, 0, sizeof(dop));
> dop.drive_fl = drive;
> int ret = scsi_is_ready(&dop);
> if (ret)
>    return "empty";
> // Read the Boot Record Volume Descriptor
> u8 buffer[CDROM_SECTOR_SIZE];
> dop.command = CMD_READ;
> dop.lba = 0x11;
> dop.count = 1;
> dop.buf_fl = buffer;
> ret = process_op(&dop);
> if (ret)
>    return "read error";
> // Is it bootable?
> if (buffer[0])
>    return "not bootable";
> if (strcmp((char*)&buffer[1], "CD001\001EL TORITO SPECIFICATION") != 0)
>    return "not bootable";
> // Read the Primary Volume Descriptor
> dop.command = CMD_READ;
> dop.lba = 0x10;
> dop.count = 1;
> dop.buf_fl = buffer;
> ret = process_op(&dop);
> if (ret)
>    return "read error";
> // Read volume id, trim trailing spaces
> char *volume = znprintf(30, "%s", buffer + 40);
> char *h = volume + strlen(volume);
> while (h > volume) {
>    h--;
>    if (*h != ' ')
>        break;
>    *h = 0;
> }
> return volume;
>
> +}

Thanks.  In general, looks fine to me.

Some minor comments:

If there's a read error or non-bootable device, shouldn't it just
revert to the original description?

There's a nullTrailingSpace() function to remove trailing spaces.

-Kevin

[Gerd Hoffmann]

> Thanks.  In general, looks fine to me.
>
> Some minor comments:
>
> If there's a read error or non-bootable device, shouldn't it just
> revert to the original description?

What about "empty" for drives not ready?  Drop that too?

I think we should be consistent here:  Either be verbose and report
"empty" / "read error" / "not-bootable" status in case we figure we
can't boot from the drive for one of the listed reasons.  Or be quiet
and just report the volume label for bootable drives and nothing
otherwise.

I've also toyed with the idea to use a higher default priority for
non-bootable cdrom drives, so with multiple cdroms and no explicit
boot order the bootable would be listed first and used by default.
What do you think about this?
> There's a nullTrailingSpace() function to remove trailing spaces.

Ah, good, I'll use that.

cheers,
  Gerd

[Kevin O'Connor]

On Wed, Sep 26, 2018 at 07:03:37AM +0200, Gerd Hoffmann wrote:
> > Thanks.  In general, looks fine to me.
> >
> > Some minor comments:
> >
> > If there's a read error or non-bootable device, shouldn't it just
> > revert to the original description?
>
> What about "empty" for drives not ready?  Drop that too?

Well, if I saw:

4. DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD] (empty)

I don't think I'd know what "empty" meant.  Also, it's a little odd to
have a C function sometimes return a dynamically allocated string and
sometimes return a constant string.  That said, I don't have a strong
opinion.

-Kevin

[Peter Stuge]

Kevin O'Connor wrote:
> it's a little odd to have a C function sometimes return a dynamically
> allocated string and sometimes return a constant string.

Gerd, please don't do that. Sure, maybe nothing is ever free()d, but
that's still very poor practice. Don't spread it.

//Peter