[Bug 772243] Review Request: rds-tools - utilities for testing rds protocol networking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=772243

--- Comment #5 from Doug Ledford <dledford@xxxxxxxxxx> 2012-01-09 15:15:24 EST ---
(In reply to comment #1)
> Just some comments:
> 
> - License seems to be GPLv2 or BSD -- not GPLv2+ or BSD
> - You can use the name macro in Source0
> - Comment on the patches in the spec file, if possible

All done.

> - FSF address is wrong in rds-sample.c

While checking on this, I noted that the main source files redirect a person to
a copy of the GPLv2 in the file COPYING, yet that file isn't present.  

> - Please harmonize the use of RPM_BUILD_ROOT and buildroot macro

Done.

> - If you don't go for EPEL 5, you can delete the buildroot definition, the
> clean section and the rm in the install section

The package will be shared with RHEL, including RHEL5, so these should stay for
now.

> - defattr is no longer necessary

See above.

> - Remove README from the documentation as it holds no valuable information

Done.

> - The optflags are not used when compiling

This is fixed, but it's kind of ugly.  The upstream build files simply do not
honor passed in CFLAGS via the environment, only if you explicitly set CFLAGS
to the make comment are they honored (and this is in contradiction with the
fact that configure says you can do CFLAGS=blah and it will put it in the
makefile, it doesn't actually do so however).  And the makefile doesn't append
its own stuff to the passed in CFLAGS, it's either the built in CFLAGS or the
passed in, no merging.  So, I had to include the one item that the makefile
needs in addition to the rpm opt stuff in order for it to compile.

> - You can patch the permissions for the executables in rds-tools-make.patch
> instead of changing them in the install section

Done.

> - What is that rds.conf file about?

The rds protocol is autoloaded if anyone attempts to open an rds socket. 
However, there is no method in place to determine if either the tcp transport,
rdma transport, or both should be loaded in order to satisfy the needs of the
rds protocol.  So, whenever we load the rds protocol, also load both transport
modules.  That way, whatever one is appropriate can be used to service the
request versus failing to open an rds socket even though the rds protocol was
successfully loaded.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]