search

IBM / xmlservice

XML-based interface for accessing IBM i resources
https://xmlservice.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
35 stars 18 forks source link

Update ilePGM() and ileSRV() to count parameters backwards so they don't leave off subsequent parameters after an omitted parameter #64

Closed brandonp42 closed 9 months ago

brandonp42 commented 1 year ago

Closes #63

The current logic starting at lines https://github.com/IBM/xmlservice/blob/master/src/plugile.rpgle#L4342 and https://github.com/IBM/xmlservice/blob/master/src/plugile.rpgle#L4985 doesn't work correctly because it assumes once you hit a parameter with a null pointer there are no other valid parameters following that. Unfortunately if you have a parameter that allows *OMIT and then specify a later parameter then it won't pass that later parameter.

My changes walk backwards through the parameter pointer array instead, finding the last valid parameter. I also put code in place to ensure it doesn't evaluate memory that's not technically in use (using sArgvSz) although in theory those unused pointers are already initialized to null.

brandonp42 commented 1 year ago

Tagging @amagid and @GajenderI

GajenderI commented 1 year ago

@brandonp42 Thank you very much for this.

brandonp42 commented 1 year ago

I am trying to understand why this code even needs to calculate how many args there are being passed.

In the ilePGM() function (invoked for mode="opm") it needs to know argc so it can pass that directly to _CALLPGMV.

While ileSRV() (again invoked for mode="opm") doesn't use _CALLPGMV, it still needs to know how argc to decide which generic prototype to use based on how many parameters it should pass on. Basically this function used for mode="opm" gets around the _ILECALL (lack of) operational descriptor issue by using generic prototypes that allows RPG to build the operational descriptor that subsequent RPG functions need for %PARMS(). I did consider changing it to use a single generic prototype with all the parameters set for OPTIONS(*NOPASS) but started to get more uncomfortable with that due to the testing situation so ultimately decided to just leave that be for now.

The argument array is being pointed to a global variable sArgvBegP, which gets set elsewhere. There's also another global variable sArgvSz, couldn't that just be used to calculate the count? I really hate global variables since it makes it hard to reason about the program flow and who sets what where, but it seems to me that whoever is setting up this array should know how many elements are in it already and it should be responsible for keeping the count (maybe we need a new global to track? 🤮).

I agree we really ought to add a new global argument count variable. Going back to automated testing though I was trying to make strategic localized changes instead of bigger picture stuff that would be worrisome without a good way to prove it all works right.

So FWIW I'd be fine if you want to leave this PR open until better testing is in place. Also I could change it back to a draft until then, however you'd like to approach it is totally fine with me.