Allow symlinks in disksets

[exuvo]

SUMMARY

-D allows symlinks ex -D /dev/disk/by-id/ata-WDC_WD40EFZX-123abc --diskset test:/dev/disk/by-id/ata-WDC_WD40EFZX-123abc does not give an error but does not see any disk activity same problem with /dev/sda instead of straight sda.

The problem is that def basename(self, disk): on line 820 is never called for disksets. Normal disklist (-D) does the conversion in def vars(self): on line 844, something similar needs to be done for disksets.

Simple fix in my tests for /dev/sda is adding the line disk = self.basename(disk) after line 889 (for disk in op.diskset[diskset]) but maybe there is a better place for it. That however spams dool: symlink lines for /dev/disk/by-id/ but does work. It also errors out if the path does not exist which normal -D does not (which is very good as i have disks that are sometimes on and can use the same command).

I think something like members = map(self.basename, members) at the command parsing stage is the better way to do it but self.basename is not defined in class Options.

Related to #35

ISSUE TYPE

• Feature Idea

DOOL VERSION

git master

OS / ENVIRONMENT

arch-linux

STEPS TO REPRODUCE

--diskset test:/dev/disk/by-id/ata-WDC_WD40EFZX-123abc vs -D /dev/disk/by-id/ata-WDC_WD40EFZX-123abc

EXPECTED RESULTS

same output

ACTUAL RESULTS

0s on both read and write

[exuvo]

With my simple fix: For the print spam, just uncomment: print('dool: symlink %s -> %s' % (disk, target)).

I think the fix for error if the path does not exist is to just replace the print('dool: %s does not exist') with return disk (or any string that wont match any other disk) in basename(self,disk).

[scottchiefbaker]

This is new to me. AFAIK all the disk based plugins use the names found in /proc/diskstats.

dool --disk --diskset two:sda,sdb,sdc,sdd  # works
dool --disk --diskset os:/dev/sda,/dev/sdb # should not work?

Are you saying that using a symlink works in some places? I'm not sure if it should.

[exuvo]

It does because it first does a symlink resolve using self.basename(disk) before accesing /proc/diskstats.

[exuvo]

At line 820: def basename(self, disk). It strips /dev/ and resolves symlinks.

[exuvo]

I think my fix is actually fine, it re-resolvs the symlink each print but the cost is minimal for no having to restart the command when a disk appears.

[exuvo]

Patch

# diff -u /usr/bin/dool /usr/local/bin/dool 
--- /usr/bin/dool   2023-10-14 19:32:44.000000000 +0200
+++ /usr/local/bin/dool 2023-10-14 19:32:07.695668461 +0200
@@ -829,12 +829,13 @@
                     if target[0] != '/':
                         target = os.path.join(os.path.dirname(disk), target)
                         target = os.path.normpath(target)
-                    print('dool: symlink %s -> %s' % (disk, target))
+                    #print('dool: symlink %s -> %s' % (disk, target))
                     disk = target
                 # trim leading /dev/
                 return disk[5:]
             else:
-                print('dool: %s does not exist' % disk)
+                return disk
         else:
             return disk

@@ -887,6 +888,7 @@
             for diskset in self.vars:
                 if diskset in op.diskset:
                     for disk in op.diskset[diskset]:
+                        disk = self.basename(disk)
                         if fnmatch.fnmatch(name, disk):
                             self.set2[diskset] = ( self.set2[diskset][0] + int(l[5]), self.set2[diskset][1] + int(l[9]) )

[scottchiefbaker]

Looking at this issue holistically I think it makes the most sense to resolve the disksets upon initial creation (at line 131) instead of each time we iterate. I just landed some code that I think does what we need. Can you test that and let me know?

[exuvo]

Yes the next branch works well. Nice work.