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

fix xdr parsing of enum, union #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

xrdavies
Copy link

+1.1 - 120730 - beta1 - Rui Xie, Alcatel-Lucent, Inc:

  • - fix xdr parsing of enum, union
  • - add feature random encoding for XDR encoding/decoding.

+1.1 - 120730 - beta1 - Rui Xie, Alcatel-Lucent, Inc:
+ - fix xdr parsing of enum, union
+ - add feature random encoding for XDR encoding/decoding.
@xrdavies xrdavies closed this Nov 9, 2016
@xrdavies xrdavies reopened this Nov 9, 2016
@@ -53,4 +53,4 @@ each of the following conditions is met:
ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.

$Revision$, Last updated $Date$
Copy link
Owner

Choose a reason for hiding this comment

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

Please no expanded CVS IDs

ERL = erl
#

EBIN=../ebin
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't submit configs of your build environment upstream


../ebin/rpc.app: rpc.app.src
perl -e $(APPSCRIPT) "$(RPC_VSN)" $(RPC_MODULES) < $< > $@

Copy link
Owner

Choose a reason for hiding this comment

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

from the last comment till here, is this an unrelated change?

if [ ! -f $noxinfile.x ]; then
echo "Sorry, file $noxinfile.x does not exist"
if [ ! -f $xfile ]; then
echo "Sorry, file $xfile does not exist"
Copy link
Owner

Choose a reason for hiding this comment

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

where does this difference come from?

@@ -2,7 +2,7 @@
%%% Author : Tony Rogvall <[email protected]>
%%% Purpose : Port mapper interface
%%% Created : 14 Aug 1997 by Tony Rogvall <[email protected]>
%%% Copyright (c) 2000 Sendmail, Inc. All rights reserved.
%%% Copyright (c) 2000, 2001 Sendmail, Inc. All rights reserved.
Copy link
Owner

Choose a reason for hiding this comment

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

where does this difference come from?

bytes_out = Size, xid = S#state.xid, packet = Call},
S1 = S#state{xid = S#state.xid+1, pending = [Pnew|Ps],
pendinglen = Plen + 1},
{noreply, incr_call_stats(S1, Procedure)}.
Copy link
Owner

Choose a reason for hiding this comment

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

what was the source version this pull request is based on?
It's definitely not the version from git

Copy link
Owner

@gebi gebi left a comment

Choose a reason for hiding this comment

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

Thank you for submitting such a big enhancement!

Though i don't think i'm qualified enough to merge them as i've no way to review the changes for soundness on the logical level.

If i'd do a review just on the syntax alone I don't think this could be merged as is.

Some things to note:

  • it seems your submitted pull request is not based on the version from rpc where your git branch is forked of
  • you have some suspicious and unrelated changes included
  • there are also local build configuration changes for your environment included in the PR
  • as other project related files which would have no place upstream, eg. gitattributes, gitignore files) and also superfluous line ending changes.

But even iff you'd change those things and get the PR in a state where it could be merged, i'd still have the problem of missing the knowledge to be able to decide if this a change which should be merged, how/if does it affect current systems and implementations and if this is a feature that should be merged into upstream.

As it stand's now i'm sorry but i can't merge your changes as long as no more knowledgeable person steps forward who can review this change and the PR is cleaned up.

@xrdavies
Copy link
Author

Hi gebi,

Thanks for review.
It's a patch for xdr parsing issue in 2012, when I was in Lucent. The code is a little bit out of date. I will try to clean the changes and request a new PR later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants