Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor spa, vdev, and metaslab commands #206

Open
sdimitro opened this issue Apr 8, 2020 · 0 comments
Open

Refactor spa, vdev, and metaslab commands #206

sdimitro opened this issue Apr 8, 2020 · 0 comments

Comments

@sdimitro
Copy link
Contributor

sdimitro commented Apr 8, 2020

These commands were hacked together during the initial prototype of sdb based on crash-python and then they were shoved into drgn-based prototype from which point they were never revisited. As a result their code is far from optimal and since they are used so often we'll want to refactor them soon. Below is a list of problems with that code:

[1] man/help messages are missing - also the descriptions of each of their command line options is lacking

[2] The arg_string convention used/passed between these commands is very ad-hoc and MDB-like (meaning Python should allow us to do better) - here is what I'm talking about

spa.py:

    def __init__(self, args: str = "", name: str = "_") -> None:
        super().__init__(args, name)
        self.arg_string = ""
        if self.args.metaslab:
            self.arg_string += "-m "
        if self.args.histogram:
            self.arg_string += "-H "
        if self.args.weight:
            self.arg_string += "-w "

...

    def pretty_print(self, spas: Iterable[drgn.Object]) -> None:
            ...
            if self.args.vdevs:
                vdevs = sdb.execute_pipeline([spa], [Vdev()])
                Vdev(self.arg_string).pretty_print(vdevs, 5)   // <---- arg_string passed here

then in vdev.py we do the same for metaslab.py.

[3] It is annoying that in order to print just the metaslabs for now through spa (e.g. ::spa -m in mdb) we are forced to provide the -v option on top of -m. E.g.

sdb> spa -m                           // <----- The -m option is basically a no-op here
ADDR               NAME
------------------------------------------------------------
0xffff94234ca54000 rpool


sdb> spa -vm
ADDR               NAME
------------------------------------------------------------
0xffff94234ca54000 rpool
      ADDR               STATE   AUX  DESCRIPTION
      ------------------------------------------------------------
      0xffff94234cc8c000 HEALTHY NONE  root
      0xffff94234cde4000 HEALTHY NONE    /dev/sda1
           ADDR                 ID           OFFSET     FREE  FRAG     UCMU
           -----------------------------------------------------------------
           0xffff94234cf94000    0              0x0     59MB   54%       0B
           0xffff94234cf93000    1       0x20000000    301MB    6%       0B
           0xffff94234cf92000    2       0x40000000    307MB    0%       0B

In general we need to rethink what we want the behavior of the options to be and think of a clean way to pass them between those commands (see [2]).

[4] As it is visible in the regression output introduced in the histogram PR #207 , we have a pool (rpool) where the histograms feature is not enabled. It would be nice to check if that version is enabled before attempting to print anything histogram related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant