Merge several fixes/features#2
Open
alesax wants to merge 7 commits into
Open
Conversation
Contributor
|
just formality from a quick glance: patch 2 and 3 are missing SoB. |
There was a problem hiding this comment.
Pull request overview
This PR merges multiple upstream patches to modernize nvmetcli’s Python dependencies, switch to exported configshell symbols, and extend configfs support (discovery NQN, reservations, and NVMe passthru target configuration) across the CLI and backing nvmet library.
Changes:
- Drop Python
sixusage and update code to native Python 3 APIs (e.g.,dict.items()), updating packaging/docs accordingly. - Add support for the
discovery_nqnconfigfs attribute and surface it in the CLI. - Introduce passthru and reservation-related plumbing in
nvmet/nvme.pyand expose passthru controls in the interactive CLI.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| rpm/nvmetcli.spec.tmpl | Removes the runtime dependency on python-six. |
| README | Updates stated Python support (Python 3) after removing six. |
| nvmetcli | Switches to exported configshell symbols; adds discovery summary, passthru UI node, and reservation status display. |
| nvmet/nvme.py | Removes six dependency; adds discovery attr group, passthru support, reservation attr group; adds kmod fallback behavior. |
| nvmet/init.py | Exposes Passthru in the package exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| nsids = [n.nsid for n in subsystem.namespaces] | ||
| for index in moves.xrange(1, self.MAX_NSID + 1): | ||
| for index in xrange(1, self.MAX_NSID + 1): |
|
|
||
| grpids = [n.grpid for n in port.ana_groups] | ||
| for index in moves.xrange(2, self.MAX_GRPID + 1): | ||
| for index in xrange(2, self.MAX_GRPID + 1): |
Comment on lines
+265
to
+272
| # Try the binary specified in /proc | ||
| try: | ||
| kmod = None | ||
| with open('/proc/sys/kernel/modprobe', 'r') as f: | ||
| kmod = f.read().rstrip() | ||
| os.system(kmod + ' ' + modname) | ||
| except Exception as e: | ||
| pass |
Comment on lines
538
to
542
| d['nqn'] = self.nqn | ||
| d['namespaces'] = [ns.dump() for ns in self.namespaces] | ||
| d['allowed_hosts'] = self.allowed_hosts | ||
| d['passthru'] = [self.passthru.dump()] | ||
| return d |
| try: | ||
| pt = Passthru(subsys) | ||
| except CFSError as e: | ||
| err_func("Could not create Namespace object: %s" % e) |
Comment on lines
173
to
+177
| def refresh(self): | ||
| self._children = set([]) | ||
| UINamespacesNode(self) | ||
| UIAllowedHostsNode(self) | ||
| UIPassthruNode(self) |
Comment on lines
+104
to
+107
| def summary(self): | ||
| info = [] | ||
| info.append("discovery=" + self.cfnode.get_attr("discovery", "nqn")) | ||
| return (", ".join(info), True) |
Comment on lines
+371
to
+372
| resv_enable = self.cfnode.get_attr("resv", "enable") | ||
| info.append("resv_enable=" + str(resv_enable)) |
Comment on lines
+257
to
+260
| if self.cfnode.admin_timeout != 0: | ||
| info.append("admin_tomeout=" + str(self.cfnode.admin_timeout)) | ||
| if self.cfnode.io_timeout != 0: | ||
| info.append("io_tomeout=" + str(self.cfnode.io_timeout)) |
Contributor
|
May I suggest to consider this one too? https://lore.kernel.org/linux-nvme/20250728155504.403426-1-mlombard@redhat.com/ |
Add support for reservation configuration. Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> Ales: handle if resv_enable doesn't exist
kmodpy is an unmaintained project. Python is not really good at backwards compatibility. With the upstream project not keeping up with python churn using the library is a maintenance burden. nvmet does not use the python library in any substantial way, it only loads a module once. This can be easily accomplished without any library using the modprobe tool directly. Ales: use subprocess.run() instead of system(), take into account if there are extra arguments in /proc/sys/kernel/modprobe.
This change adds the support for configuring NVMe passthru target. Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> Ales: handle if kernel doesn't support passthru target. Fix typos (Namespace->Passthru, tomeout->timeout)
Python 2 has been truly and completely dead for over five years, I don't know about any Linux distribution which wouldn't have Python 3 as its default Python. Please, let it die. Ales: s/xrange/range/
Implement support for the 'discovery_nqn' configfs attribute to allow the user to read or modify the persistent discovery NQN. Signed-off-by: Hannes Reinecke <hare@kernel.org> Ales: handle lack of support of discovery_nqn
The configshell-fb module is now called configshell. From v2.0.0 onwards the preferred way is to import the module as configshell. Signed-off-by: Stephan Müller <stephan.mueller@phys.ethz.ch>
Replace direct invocations of configshell internals with actually exported symbols. Signed-off-by: Stephan Müller <stephan.mueller@phys.ethz.ch>
Author
Yes, that was my candidate for 2nd run, will do. |
Comment on lines
176
to
181
| def refresh(self): | ||
| self._children = set([]) | ||
| UINamespacesNode(self) | ||
| UIAllowedHostsNode(self) | ||
| UIPassthruNode(self) | ||
|
|
Comment on lines
+199
to
+201
| def refresh(self): | ||
| pass | ||
|
|
Comment on lines
832
to
836
| try: | ||
| shell = configshell.shell.ConfigShell('~/.nvmetcli') | ||
| shell = configshell.ConfigShell('~/.nvmetcli') | ||
| UIRootNode(shell) | ||
| except Exception as msg: | ||
| shell.log.error(str(msg)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This merges several patches submitted to the mailing list: