Took the storage ones, expect them early last week or right after the holiday.
On Wed, Apr 5, 2017 at 11:34 PM, Moti Asayag <masa...@redhat.com> wrote: > > > On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgo...@redhat.com> wrote: > >> >> >> On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masa...@redhat.com> wrote: >> >>> Hi All, >>> >>> ATM, there are 78 occurrences of "Injector.injectMembers(new >>> AuditLogableBase())" in ovirt-engine project, which their main purpose is >>> to ease the resolve of the placeholders of the audit log message while >>> logging an event. >>> >>> For instance AuditLogType.MAC_ADDRESS_IS_EXTERNAL is being used from >>> ImportVmCommandBase.java in the following way: >>> >>> private AuditLogableBase createExternalMacsAuditLog(VM vm, Set<String> >>> externalMacs) { >>> AuditLogableBase logable = *Injector.injectMembers*(new >>> AuditLogableBase()); >>> logable.setVmId(vm.getId()); >>> logable.setCustomCommaSeparatedValues("MACAddr", externalMacs); >>> return logable; >>> } >>> >>> The entry in the properties file is: >>> MAC_ADDRESS_IS_EXTERNAL=VM ${*VmName*} has MAC address(es) ${MACAddr}, >>> which is/are out of its MAC pool definitions. >>> >>> Therefore the only purpose of the injection is to allow the >>> AuditLogDirector to resolve the ${*VmName*} which is already known at >>> the time of creating the AuditLogableBase entry. >>> >>> The result is injecting the DAOs for the AuditLogableBase instance and >>> using the VM dao to retrieve the VM entry from the DB. >>> This is just a wastef of injection and DB access while both can be >>> spared. >>> >>> This could have been easily replaced by one of the following: >>> >>> - auditLogableBase.setVmName(vm.getName()); >>> >>> - setVmName is protected so not usable as is >> > > It will become public if we agree on > > https://gerrit.ovirt.org/#/c/75244/2/backend/manager/ > modules/dal/src/main/java/org/ovirt/engine/core/dal/ > dbbroker/auditloghandling/AuditLogableBase.java > > >> >>> - auditLogableBase.addCustomValue("VmName", vm.getName()); >>> >>> I prefer this, it is readable. and BTW it is fluent, it returns 'this' >> so use >> >> AuditLogDirector(new AuditLogableBase(type) >> .addCustomValue("VmName", vm.getName())); >> > > I'm okay with this as well. > > >> Please pick up any occurrence from your domain and send a patch to >>> replace it where possible. >>> Thanks in advance, >>> Moti >>> >> >> +1 >> >> Frankly the fact that all the DAOs sets protected access in >> AuditLogableBase is a total abuse. Every command should declare its own >> deps. >> >> > That will require a huge effort. > > >> _______________________________________________ >> >>> Devel mailing list >>> Devel@ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/devel >> >> > > > -- > Regards, > Moti > > _______________________________________________ > Devel mailing list > Devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel >
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel