Bug 234420 - adminutil: Use FHS paths and general code cleanup
adminutil: Use FHS paths and general code cleanup
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Admin (Show other bugs)
1.0.4
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rich Megginson
Viktor Ashirov
:
Depends On:
Blocks: 152373 240316 FDS1.1.0
  Show dependency treegraph
 
Reported: 2007-03-28 23:03 EDT by Rich Megginson
Modified: 2015-12-07 11:44 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-07 11:44:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
diffs (182.26 KB, patch)
2007-03-28 23:03 EDT, Rich Megginson
no flags Details | Diff
cvs commit log (8.20 KB, text/plain)
2007-04-04 15:39 EDT, Rich Megginson
no flags Details

  None (edit)
Description Rich Megginson 2007-03-28 23:03:31 EDT
adminutil should install its libs in the regular libdir.  The library needs to
be able to find its property bundles at runtime, and these should be in
datadir/packagename.  The dbswitch.conf has been folded into adm.conf (the new
ldapurl parameter) and other stuff has been changed (e.g. a securitydir
parameter instead of separate key/cert db parameters, and other new parameters).

In addition, there is a lot of dead code and just plain incorrect code in this
library.
Comment 1 Rich Megginson 2007-03-28 23:03:32 EDT
Created attachment 151177 [details]
diffs
Comment 2 Nathan Kinder 2007-04-04 12:56:47 EDT
Great work Rich!  There's a lot of cleanup there.

The only question I have is if errorStr needs to be freed at the end of
psetErrorString()?
Comment 3 Rich Megginson 2007-04-04 13:07:18 EDT
(In reply to comment #2)
> The only question I have is if errorStr needs to be freed at the end of
> psetErrorString()?

...
  case PSET_OP_OK:
    if (admutil_i18nResource) 
      errorStr = res_getstring(admutil_i18nResource, DBT_pset_OP_OK, acceptLang,
buffer, bufsize, rc);
    if (errorStr) return errorStr;
    else errorStr = "Operation OK";
    break;
...
  if (buffer) {
      PL_strncpyz(buffer, errorStr, bufsize);
      return buffer;
  }

  return PL_strdup(errorStr);

I don't think so.  Either errorStr will be the value returned by
res_getstring(), and it will be returned immediately, or errorStr will point to
a static string.  In neither case should it be freed.  The caller is responsible
for freeing the returned string in the last PL_strdup() case.
Comment 4 Nathan Kinder 2007-04-04 14:36:51 EDT
ok, looks good.
Comment 5 Rich Megginson 2007-04-04 15:39:20 EDT
Created attachment 151707 [details]
cvs commit log

Reviewed by: nkinder (Thanks!)
Files: see diff
Branch: HEAD
Fix Description:
1) Added a propertydir parameter to Makefile.am.  This is where the .res files
go.  This also gets baked into the code so that the library knows where to find
them.
2) The icu code expects the .res files to be in a packagename directory -
packagename/foo.res not packagename_foo.res.  I don't know how this ever
worked.  I also added en_US.res and en.res - icu recommends having the actual
locale file rather than just falling back to the default root.res - see
http://icu-project.org/userguide/ResourceManagement.html
3) There was quite a bit of dead code that I got rid of
4) Fixed many compiler warnings
5) There were quite a few memory leaks.  The biggest one was probably in
psetDelete, which did not actually delete the pset.  Another one was the
resource string handling - this returns malloc'd memory, and was never freed. 
I added the option to pass in a static sized buffer to hold the resource string
- this may be truncated but we usually won't care.  There were several places
where the code was calling PR_Free on a data structure pointer - doing a
"shallow" free rather than a "deep" free of all of the pointers in the data
structure. 6) I merged in configuration from dbswitch.conf and other config
files so that we could get rid of them and just have adm.conf.	We'll have to
take care of this during migration.
Platforms tested: RHEL4, FC6
Flag Day: no
Doc impact: no
Comment 6 Anh Nguyen 2007-12-03 10:50:04 EST
Changed QA Whiteboard to to_be_verified_by_dev.
Comment 7 Nathan Kinder 2007-12-14 18:52:23 EST
Verified that adminutil installs it's libs in /usr/lib and it's resource files
in /usr/share/adminutil.  I also verified that the build output has no warnings
or errors other than a few unused variable warnings.  The code cleanup is
verified as a by-product of our migration and Console tests that exercise the
adminutil code.

Note You need to log in before you can comment on or make changes to this bug.