Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xrdssi v2 #542

Merged
merged 99 commits into from
Aug 1, 2017
Merged

Xrdssi v2 #542

merged 99 commits into from
Aug 1, 2017

Conversation

abh3
Copy link
Member

@abh3 abh3 commented Jul 13, 2017

No description provided.

abh3 and others added 30 commits March 18, 2015 14:31
        o Allow the SSI framework to coexist with a filesystem
        o Fully objectify access to the the SSI framework (no extern C)
        o Vector all errors via the callback to avoid split error checks
        o Fully implement the QueryResource() interface
        o Introduce a provider class to rationalize classes
        o Some bug fixes along the way
…ework.

Set default debugging level to off. Turn it on if envar XRDSSIDEBUG is set.
Fix pedantic compiler complaints.
Add pthread library to the XrdSsiShMap.so build.
Conflicts:
	docs/PreReleaseNotes.txt
Do not use the message response thread for callbacks.
Cleanup some tracing information to get a better idea of the flow.
the event that the session is unprovisioned before they complete.
This allows the callback to call Finished() and delete the request.
Conflicts:
	docs/PreReleaseNotes.txt
Conflicts:
	docs/PreReleaseNotes.txt
Conflicts:
	docs/PreReleaseNotes.txt
Clang reports a problem with the variable being used to calculate the
size of the string buffer in XrdSsiFileReq::Alloc -

    XrdSsiFileReq.cc:142:33: error: invalid use of member 'rID' in static member

The fix is to use sizeof() on the same variable that is receiving the string.
Use correct snprintf sizeof variable
Add rUser member to the resource object (xfer query ID).
Add rInfo member to the resource object (xfer CGO information).
Add uConn argument to Provision(). This specifies how TCP
connections should be handled relative to rUser.
Add respwt directive to specify default response wait time.
Conflicts:
	docs/PreReleaseNotes.txt
Increase maximum number of parallel requests in a session to 1024.
Add check if a responder is trying to respond to the wrong request.
Above check usually caches responding to a cancelled request.
Allow short data responses to be immediately sent.
Various fixes and improvements.
Fix sizeof() issue when clearing a buffer.
Conflicts:
	docs/PreReleaseNotes.txt
@abh3 abh3 requested a review from simonmichal July 13, 2017 02:09
@abh3 abh3 self-assigned this Jul 13, 2017
Copy link
Contributor

@simonmichal simonmichal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Are we sure that SSI should be part of xrootd-server-libs? We could also create a separate package for ssi, say xrootd-ssi-libs and xrootd-ssi-devel.
  2. libXrdSsi and libXrdSsiLog are not versioned and the unversioned libs are not packaged.
  3. IMHO it's better to keep all SSI specific staff in XrdSsi.cmake and don't split it between XrdSsi.cmake and XrdPlugins.cmake.

@@ -29,6 +29,7 @@ include( XrdXml )
include( XrdPosix )
include( XrdFfs )
include( XrdPlugins )
include( XrdSsi )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to include XrdSsi in 4.7.0? If not this needs to be optional (similarly as for http and cepth).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we want SSI and http to be part of the core. People depend on it being packaged that way. Ceph is problematic because of the library dependencies. However, let's keep Ceph he way it is for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I will consolidate the SSI stuff as you suggest.

@@ -9,6 +9,8 @@ set( LIB_XRD_N2NO2P XrdN2No2p-${PLUGIN_VERSION} )
set( LIB_XRD_PSS XrdPss-${PLUGIN_VERSION} )
set( LIB_XRD_GPFS XrdOssSIgpfsT-${PLUGIN_VERSION} )
set( LIB_XRD_ZCRC32 XrdCksCalczcrc32-${PLUGIN_VERSION} )
set( LIB_XRD_SSI XrdSsi-${PLUGIN_VERSION} )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it would be better to keep all XrdSsi cmake staff in XrdSsi.cmake. Adding it partially to XrdPlugins.cmake is misleading and clutters the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

XrdUtils
XrdServer )

set_target_properties(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library is not versioned!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the library is installed but is not packaged.
Note this creates libXrdSsi.so which is installed but not packaged. libXrdSsi.so.* are, in turn not created but they are packaged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! Will fix.

XrdUtils
XrdServer )

set_target_properties(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library is not versioned!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the library is installed but is not packaged.
Note this creates libXrdSsiLog.so which is installed but not packaged. libXrdSsiLog.so.* are, in turn not created but they are packaged.

@@ -733,6 +733,10 @@ fi
%{_libdir}/libXrdN2No2p-4.so
%{_libdir}/libXrdOssSIgpfsT-4.so
%{_libdir}/libXrdServer.so.*
%{_libdir}/libXrdSsi-4.so.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this should go to server-libs, and not to a new package, .e.g. ssi-libs ?
Actually, since libXrdSsi is not versioned this fails as there are no libXrdSsi-4.si.*, there is only the libXrdSsi-4.so, which in turn is installed but not packaged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will give this another go. There is no reason for it not being part of server libs like the other libs.

@@ -733,6 +733,10 @@ fi
%{_libdir}/libXrdN2No2p-4.so
%{_libdir}/libXrdOssSIgpfsT-4.so
%{_libdir}/libXrdServer.so.*
%{_libdir}/libXrdSsi-4.so.*
%{_libdir}/libXrdSsiLib.so.*
%{_libdir}/libXrdSsiLog-4.so.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, since libXrdSsiLog is not versioned this fails as there are no libXrdSsiLog-4.si.*, there is only the libXrdSsiLog-4.so, which in turn is installed but not packaged.

@@ -125,6 +125,18 @@ set( XROOTD_PRIVATE_HEADERS
XrdOfs/XrdOfsHandle.hh
XrdOfs/XrdOfsTrace.hh
XrdOfs/XrdOfsTPCInfo.hh
XrdSsi/XrdSsiAtomics.hh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we don't want this in a separate package, say ssi-devel (together with *.so files) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason these are in the private section is because the Castor people wanted some additional enhancements that may or may not break ABI. So, this is temporary until we get that straightened out. After that the headers will go into the public section.

@abh3
Copy link
Member Author

abh3 commented Jul 14, 2017

OK, I will work on it today and reissue the pull.

@abh3 abh3 closed this Jul 14, 2017
@ljanyst
Copy link
Contributor

ljanyst commented Jul 14, 2017

FYI, there is no need to close and reissue. Any new commits you push to the branch will automatically appear as a revision of this pull request.

@abh3
Copy link
Member Author

abh3 commented Jul 14, 2017

OK, I have fixed hopefully all of he packaging issues. If not, please let me know.

@abh3 abh3 reopened this Jul 14, 2017
Copy link
Contributor

@simonmichal simonmichal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's almost perfect ;-)

The only problem now is that following *.so are installed but not packaged:
/usr/lib64/libXrdSsiLib.so
/usr/lib64/libXrdSsiShMap.so

I think they should go into server-devel, just add the following to the respective section in the spec file:
%{_libdir}/libXrdSsiLib.so
%{_libdir}/libXrdSsiShMap.so

Michal

@simonmichal simonmichal merged commit c4f43a2 into master Aug 1, 2017
@amadio amadio deleted the xrdssiV2 branch August 25, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants