Bug 243318 - GEOM /dev/diskid '%20' in names
Summary: GEOM /dev/diskid '%20' in names
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Conrad Meyer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-13 14:01 UTC by Peter Eriksson
Modified: 2020-01-18 03:34 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Eriksson 2020-01-13 14:01:19 UTC
Hardware: HP ProLiant DL380 Gen9, running FreeBSD 12.1 with HP RAID controllers (P440ar on the motherboard to which the disks in question is connected).


# ls -l /dev/diskid | egrep 18411
crw-r-----  1 root  operator  0x123 Jan 11 00:38 DISK-18411F9829AB%20%20%20%20%20%20%20%20

# geom disk list da4
Geom name: da4
Providers:
1. Name: da4
   Mediasize: 480103981056 (447G)
   Sectorsize: 512
   Stripesize: 4096
   Stripeoffset: 0
   Mode: r1w1e2
   descr: ATA MK000480GWUGF
   lunid: 3001438043777a48
   ident: 18411F9829AB
   rotationrate: 0
   fwsectors: 32
   fwheads: 255

# camcontrol devlist |egrep da4
<ATA MK000480GWUGF HPG0>           at scbus1 target 4 lun 0 (da4,pass4)

# camcontrol inq da4
pass4: <ATA MK000480GWUGF HPG0> Fixed Direct Access SPC-4 SCSI device
pass4: Serial Number 18411F9829AB        
pass4: 135.168MB/s transfers, Command Queueing Enabled

# dmesg | egrep da4
da4 at ciss0 bus 32 scbus1 target 4 lun 0
da4: <ATA MK000480GWUGF HPG0> Fixed Direct Access SPC-4 SCSI device
da4: Serial Number 18411F9829AB        
da4: 135.168MB/s transfers
da4: Command Queueing enabled
da4: 457862MB (937703088 512 byte sectors)

# dmesg | egrep ciss0
ciss0: <HP Smart Array P440ar> port 0x4000-0x40ff mem 0x95600000-0x956fffff,0x95700000-0x957003ff at device 0.0 numa-domain 0 on pci3
ciss0: PERFORMANT Transport


We don't see the '%20' stuff on disks on another "ciss" (HP Smart Array H241" controller, but those are other types of disks too).
Comment 1 Conrad Meyer freebsd_committer 2020-01-13 20:23:26 UTC
The device node comes from GEOM label.  g_label_disk_ident taste gets the string from g_io_getattr GEOM::ident and does not modify it.  It is then returned to g_label.c::g_label_taste, 

386                 g_labels[i]->ld_taste(cp, label, sizeof(label));
387                 g_label_mangle_name(label, sizeof(label));
388                 g_topology_lock();
389                 if (label[0] == '\0')
390                         continue;
391                 g_label_create(NULL, mp, pp, label, g_labels[i]->ld_dir,
392                     pp->mediasize);

g_label_mangle_name is what converts spaces into valid /dev names (no spaces):

178 g_label_mangle_name(char *label, size_t size)
179 {
180         struct sbuf *sb;
181         const u_char *c;
182
183         sb = sbuf_new(NULL, NULL, size, SBUF_FIXEDLEN);
184         for (c = label; *c != '\0'; c++) {
185                 if (!isprint(*c) || isspace(*c) || *c =='"' || *c == '%')
186                         sbuf_printf(sb, "%%%02X", *c);
187                 else
188                         sbuf_putc(sb, *c);

Where does GEOM::ident come from?  Only two places: md(4) (not this case) and cam_xpt (this case):

xpt_getattr:

  1233         struct ccb_dev_advinfo cdai;
  1239         memset(&cdai, 0, sizeof(cdai));
  1240         xpt_setup_ccb(&cdai.ccb_h, path, CAM_PRIORITY_NORMAL);
  1241         cdai.ccb_h.func_code = XPT_DEV_ADVINFO;
  1242         cdai.flags = CDAI_FLAG_NONE;
  1243         cdai.bufsiz = len;
  1244         cdai.buf = buf;
  1245
  1246         if (!strcmp(attr, "GEOM::ident"))
  1247                 cdai.buftype = CDAI_TYPE_SERIAL_NUM;

Ok.  There are 4 CAM transports that handle CDAI_TYPE_SERIAL_NUM: ata, mmc, nvme, and scsi.  This is scsi.

  2512 scsi_dev_advinfo(union ccb *start_ccb)
  2513 {
  2514         struct cam_ed *device;
...
  2534         case CDAI_TYPE_SERIAL_NUM:
  2543                 memcpy(cdai->buf, device->serial_num, amt);

So... what's setting it in device->serial_num?  cam/scsi/scsi_xpt.c:: probestart() PROBE_SERIAL_NUM -> probedone PROBE_SERIAL_NUM:

We try to trim off leading spaces:

  1574         case PROBE_SERIAL_NUM:
  1575         {
...
  1603                                 start = strspn(serial_buf->serial_num, " ");
  1604                                 slen = serial_buf->length - start;
...
  1613                                 memcpy(path->device->serial_num,
  1614                                        &serial_buf->serial_num[start], slen);
  1615                                 path->device->serial_num_len = slen;
  1616                                 path->device->serial_num[slen] = '\0';

But not trailing spaces, AFAICT.

So, it seems to me like we should probably trim trailing spaces at some layer.  It could be done in CAM, since the serial number probably does end in literal spaces (fixed size field).  It could be done in g_label, but that would impact all g_label devices.  (I'm not sure that's a bad thing.)  If you label your GPT partition "foo     ", is it really helpful to have /dev/gpt/foo%20%20%20%20?  I suspect not.
Comment 2 Conrad Meyer freebsd_committer 2020-01-18 03:33:54 UTC
Testing notes:

testvm# mdconfig -a -t swap -s 5m
md0
testvm# gpart create -s gpt md0
md0 created
testvm# gpart add -t freebsd -i 1 -l "   test test test    " md0
md0s1 added
testvm# geom -t
Geom                                             Class      Provider
md0                                              MD         md0
  md0                                            PART       md0s1
    md0s1                                        LABEL      gpt/test%20test%20test
      gpt/test%20test%20test                     DEV
Comment 3 commit-hook freebsd_committer 2020-01-18 03:34:12 UTC
A commit references this bug:

Author: cem
Date: Sat Jan 18 03:33:44 UTC 2020
New revision: 356861
URL: https://svnweb.freebsd.org/changeset/base/356861

Log:
  GEOM label: strip leading/trailing space synthesizing devfs names

  %20%20%20 is ugly and doesn't really help make human-readable devfs names.

  PR:		243318
  Reported by:	Peter Eriksson <pen AT lysator.liu.se>
  Relnotes:	yes

Changes:
  head/sys/geom/label/g_label.c