Workaround older versions of e2fsprogs #631
Conversation
Some distributions place filesystem utilities outside /sbin.
brunotm
force-pushed
the
brunotm.issue629
branch
from
4f54802
to
3a3ee56
Compare
|
@kerneltime it is possible to have access to the CI logs? Once more i don't get any errors in my tests. Thanks! |
|
@brunotm - this is CI issue, looks like password expiration. @kerneltime needs to take care of this as soon as sun rises in Palo Alto :) The log says: an all fails at this point. |
| @@ -37,6 +37,9 @@ const pciAddrLen = 10 // Length of PCI dev addr | |||
| // FstypeDefault contains the default FS when not specified by the user | |||
| const FstypeDefault = "ext4" | |||
|
|
|||
| // BinSearchPaths contains search paths for host binaries | |||
| var BinSearchPaths = [4]string{"/bin", "/sbin", "/usr/bin", "/usr/sbin"} | |||
There was a problem hiding this comment.
This may not be needed here, why not have a "default" search path in the default config used by the plugin and that can be used as the search path for all external executables that the plugin runs. The plugin behavior can be changed any time.
There was a problem hiding this comment.
Even if we want to have a list here maybe a slice can be used vs. an array - []string{"/bin", "/sbin", "/usr/bin", "/usr/sbin"}.
There was a problem hiding this comment.
This may not be needed here, why not have a "default" search path in the default config used by the plugin and that can be used as the search path for all external executables that the plugin runs. The plugin behavior can be changed any time.
I do think that is probably better to have it in the config file.
Please correct me if i am wrong, but we hold no reference from the config instance created in main.go (where the configfile can be other than the default), and from fs.go we would only be able to load it with config.DefaultConfigPath, this file can potentially be different from the user specified - If this is the case, IMO the required changes are beyond the scope of this PR.
Even if we want to have a list here maybe a slice can be used vs. an array - []string{"/bin", "/sbin", "/usr/bin", "/usr/sbin"}.
A slice is not really needed, but its not a problem :)
There was a problem hiding this comment.
Please correct me if i am wrong
Or overcomplicating the issue! :)
There was a problem hiding this comment.
What do you think about moving the BinSearchPath to the config package for now ?
There was a problem hiding this comment.
it is nice to have it extensible but I think it's super low priority ,and to me it does not feel like a config item
There was a problem hiding this comment.
Agree. But BinSearchPath also looks misplaced in fs.go.
So we keep it there for now?
There was a problem hiding this comment.
yes - Mkfs related stuff is ok in fs.go IMO
| // Workaround older versions of e2fsprogs, issue 629. | ||
| var err error | ||
| var out []byte | ||
| if strings.Split(mkfscmd, ".")[1][0:3] == "ext" { |
There was a problem hiding this comment.
can you add a comment with info on what pattern and where you are looking for ?
|
Looks good. One request for more comments , and it's good to go after CI is fixed and passes. |
| out, err = exec.Command(mkfscmd, "-F", "-L", label, device).CombinedOutput() | ||
| } else { | ||
| out, err = exec.Command(mkfscmd, "-L", label, device).CombinedOutput() | ||
| } |
There was a problem hiding this comment.
Just asking, can we use "-F -L" always? Unless the different versions don't accept -F
There was a problem hiding this comment.
The -F is not normalized for the most common filesystem utilities (ext*, btrfs, xfs..) :(
brunotm
force-pushed
the
brunotm.issue629
branch
from
3a3ee56
to
d6913a9
Compare
|
Rebased the commit to address the comments. |
* master: (25 commits) Update new ESX IP added forgotten .so file Install sqlite3 py libs on ESX and load for Python2 Added py code and binaries for sqlite3 python libs Update drone security Removed accidental .pyc files Handle byte to string conversions for status command. Auth configuration and operation admission check (Auth.liping) (vmware-archive#603) Revert "Cli auth.liping" Cli auth.liping (vmware-archive#640) Handle missing or invalid fs type on mount. (vmware-archive#639) Updated Admin CLI commands to support tenants. (vmware-archive#620) Workaround older versions of e2fsprogs (vmware-archive#631) Add auth proposal Made handing of missing metafile less harsh. (vmware-archive#627) Fixed ACLs in payload bin dir (vmware-archive#624) Fixed error handling for set command. (vmware-archive#610) Use new error variables when rolling back volume creation to avoid nil reassignment. (vmware-archive#617) Change wording Fix broken link ...
Issue #629
This commit workaround the error of creating ext/2/3/4 filesystems when using older versions of e2fsprogs. Those versions had an broken check and asks for confirmation when creating a filesystem using the whole block device (eg. /dev/sdb)
It also Broaden search path for filesystem utilities. Some distributions place filesystem utilities outside /sbin.