Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.

Bug 234420

Summary: adminutil: Use FHS paths and general code cleanup
Product: [Retired] 389 Reporter: Rich Megginson <rmeggins>
Component: AdminAssignee: Rich Megginson <rmeggins>
Status: CLOSED CURRENTRELEASE QA Contact: Viktor Ashirov <vashirov>
Severity: medium Docs Contact:
Priority: medium    
Version: 1.0.4   
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-12-07 16:44:13 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On:    
Bug Blocks: 427409    
Attachments:
Description Flags
diffs
none
cvs commit log none

Description Rich Megginson 2007-03-29 03:03:31 UTC
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-29 03:03:32 UTC
Created attachment 151177 [details]
diffs

Comment 2 Nathan Kinder 2007-04-04 16:56:47 UTC
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 17:07:18 UTC
(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 18:36:51 UTC
ok, looks good.

Comment 5 Rich Megginson 2007-04-04 19:39:20 UTC
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 15:50:04 UTC
Changed QA Whiteboard to to_be_verified_by_dev.

Comment 7 Nathan Kinder 2007-12-14 23:52:23 UTC
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.