Vf/4018/env variables #130
Conversation
|
Great hacking!! I'll give more specific feedback on the line comments. |
| @@ -12,6 +12,10 @@ local intel10g = require("apps.intel.intel10g") | |||
| local vfio = require("lib.hardware.vfio") | |||
| local config = require("core.config") | |||
|
|
|||
| -- default id of pci device used in test | |||
| -- This ID is used if the "SNABB_TEST_PCI_ID" environment variable is not defined | |||
| DEFAULT_TEST_PCI_ID = "0000:01:00.0" | |||
There was a problem hiding this comment.
I don't think there should be a default value for the PCI address. This is too machine-dependent. Instead this test should 'skip' if the address is not set manually via the environment variable.
also: please match the existing style of the code for capitalisation. maybe not obvious, but in this code it's lower_case_with_underscores for all function/variable/constant names (but CamelCase for classes). I don't say it's the best :-))
There was a problem hiding this comment.
I suggest explicitly documenting lower_case for constants guideline in the README. UPPER_CASE for constants vs. lower_case for variables and function names is at least just as frequent (thanks to constants-as-C-macros).
|
Thanks for diving in and attacking the problem. I would say that the most important things to conditionalize are the PCI address for the 10G NIC (because most machines have no such PCI card and others will have it in different slots) and the filename for the vhost_user socket file. Those parameters should be mandatory and the tests should be skipped if they are not supplied (with a message saying why). I don't think we need to conditionalize the proc/sys files because (afaik) we can reasonably expect them to have stable paths between servers (?). It could be nice to fail with a good error message if we don't find one of those files though. |
|
Fixed BIG_LETTERS naming, removed situational defaults, added few additional checks, including filename for the vhost_user socket file and updated doc accordingly. Thanks for feedback! |
| * SNABB_VHOST_USER_SOCKET_FILE | ||
| No default value | ||
|
|
||
| * SNABB_TEST_PCI_ID |
There was a problem hiding this comment.
I suggest we can have just two variables (names slightly changed from the above):
SNABB_TEST_VHOST_USER_SOCKETfilename of the vhost_user socket file.SNABB_TEST_INTEL10G_PCI_IDPCI ID of an Intel 82599 network device.
... which contain information that needs to be supplied by the developer to make the tests meaningful (otherwise they are better skipped).
I don't think that we need to have configuration options for the /proc and /sys paths (?) that should be consistent for Linux hosts.
|
Obsolete, see #134. |
VLAN tag configuration should not require 0x8100 prefix
See issue #120.
Several hardcoded paths and values now can be defined as environment variables:
Default value "/sys/bus/pci/drivers/vfio-pci"
Default value "/sys/kernel/iommu_groups"
Default value "/sys/bus/pci/devices"
Default value "0000:01:00.0"
Default value "/proc/sys/vm/nr_hugepages"
Default value "/proc/meminfo"
'src/doc/hacking.md' file added with variables listing and examples.