Thanks Eddie for your analysis. Created PR 102 <https://github.com/apache/zookeeper/pull/102> just now that fixes all but the ZooDefs warnings. We can start from there and decide what to do with ZooDefs warnings.
On Mon, Nov 7, 2016 at 11:02 AM, Edward Ribeiro <[email protected]> wrote: > Hi folks, > > A cursory look at the new Findbug issues highlighted mostly trivial, and a > subtle but drastic change (IMHO): > > > 1. Dodgy code warnings > > * 3/4 of the cases spotted are due to the File.listFiles() method that > could return null if the abstract pathname does not denote a directory or > if an I/O error occurs. Probably it requires a: > > if (file.listFiles() != null etc) { > > More code verbosity, but doable, imo. > > * the remaining fourth case is at > DeleteCommand.retainCompatibility(String[]) (line 62) where > ' > newCmd' is created but never used. Trivial. > ------------------------------------------------------------ > ----------------------------------------------------------------------- > > 2. Performance warnings > > All of the cases are to replace this idiom > > for (K key : map.keySet()) { > V value = map.getKey(key) > > with this one: > > for (Entry<K,V> e: map.entrySet()) { > > therefore, trivial change. > > ------------------------------------------------------------ > ----------------------------------------------------------------------- > 3. > Malicious code vulnerability Warnings > > Findbugs is complaining because public or protected fields are mutable > collections as bellow: > > public static final HashMap<Integer,String> map = new HashMap<>(); > > The solution would be something along the lines of Map<Integer, String> map > = Collections.unmodifiableMap(new HashMap<>()); > > BUT this would break compatibility with ZooDefs.Ids.OPEN_ACL_UNSAFE and > ZooDefs.Ids.READ_ACL_UNSAFE :(((( > > The other mutable collections are used internally so I don't see any > further problem using an non modifiable collection, but those ZooDefs.Ids > fields can potentially break ZK clients into the wild. In fact, I commented > about this a couple years ago: > https://issues.apache.org/jira/browse/ZOOKEEPER-1362 > > ------------------------------------------------------------ > ----------------------------------------------------------------------- > > 4. Experimental Warnings > > Okay, I am totally unsure what we could alter here, but as far as I could > understand it is complaining that an I/O error could potentially leak > FileWriter, that is, it wants a: > > try { > /.../ > } > finally { > if (file != null) { > file.close(); > } > } > > As far as I understood. > ------------------------------------------------------------ > ----------------------------------------------------------------------- > > So, all in all, my question is: what to do about > ZooDefs.Ids.OPEN_ACL_UNSAFE and ZooDefs.Ids.OPEN_ACL_UNSAFE and > ZooDefs.Ids.READ_ACL_UNSAFE, aka ZOOKEEPER-1362? > > Cheers, > Edward > > > On Sat, Nov 5, 2016 at 2:01 PM, Jordan Zimmerman < > [email protected]> > wrote: > > > > Might as well fix the issues, merge to master and merge into the existing > PRs. Then it will be done. Thoughts? > > > > -Jordan > -- Cheers Michael.
