Comment 6 for bug 1417608

This review was made against lsvpd version 1.7.5-0ubuntu1; I think this
codebase would benefit from ASAN, valgrind, and consistent use of Lindent
or similar script around indent(1) or similar code formatter.

There is recurring "hiding" of errno and other specific errors that are
replaced by less-useful generic errors or incorrect errors. This can
lead to more difficult debugging of live systems. Sometimes potentially
incorrect assumptions are drawn based solely on an error return without
checking errno for potential remediation or accurate error messages. I
recommend the entire codebase be examined with this in mind, it is so
frequent.

There's more issues:

deleteList() while (!head) should be while (head) -- the current
 implementation never frees anything, NULL dereference and sigsegv if passed NULL
__lsvpdInit() does not memset the struct sigaction sigact to zero
ensureEnv() doesn't check if env is a directory, has expected ownership.
  has expected permissions; it returns 0 for most conditions and -1 only
  if the mkdir() fails. I suspect the function is entirely incorrect.
SysFSTreeCollector::getDevTreePath() misses fgets() error check
lsvpd_hexify() uses delete instead of delete[]
hexify() uses delete instead of delete[]
hexify() leaks ret if the input length is 0
device_scsi_sg_resp_len() returns uninitialized garbage if evpd isn't 0 or 1
RtasCollector::rtasGetVPD() may leak locCode via most branches of switch
RtasCollector::rtasGetVPD() may leak list via error return
RtasCollector::rtasGetVPD() does not check size += current->size; loop for
  overflow, is this a possibility?
FSWalk::fs_getDirContents() does not validate length of path_t, nor files
  in the directory, before copying their names into a fixed size buffer;
  probably the whole function should be re-written to use C++ strings
  instead
device_open() uses incorrect <= 0 error return from open(), could leave a
  device node created and opened
device_open() error handling is needlessly nested too deep, hard to read
device_open() calls device_close() if a sprintf() fails? is this correct?
device_close() doesn't actually close any devices, it only unlinks files
Why do device_close() and device_open() use /tmp? Is there no better
  place? Why not use udev-populated /dev/? Easily-guessable /tmp/names
  lead to denial-of-service possibilities.
Gatherer::~Gatherer() use-after-free, delete *i; ++i;
Gatherer::getComponentTree() may leak root or ret on error
FSWalk::fileScout() len is not reset to 0 on every newline
extractTagValue() fails to handle the case when a tag ends with :, as in
  the line "power management:"
archiveDB() reads an entire database into memory before compressing it,
  which requires the entire database to fit in RAM + swap, is this fine?

Thanks