Side deck for plugins #30

fkovinAtRocket · 2019-05-02T16:48:12Z

side deck that a plugin can use to link with the functions in the server
script that I used to generate it
header file that has all the functions that are in the side deck

- side deck that a plugin can use to link with the functions in the server - script that I used to generate it - header file that has all the functions that are in the side deck Signed-off-by: Fyodor Kovin <[email protected]>

jordanfilteau1995

The line endings in this file look strange. I know that the side file must have 80 character across. Is this going to cause an issue when it's checked out?

fkovinAtRocket · 2019-05-03T10:46:15Z

Jordan, the file is supposed to be 80 char fixed block, so it doesn't have line endings. Every line is exactly 80 characters, padded with spaces if needed.

The only problem would be if it's stored internally represented in ISO8859-1: it's unlikely that the compiler supports file tags and auto conversion. It looks like git check out stuff in 1047 by default though, so that should be alright.

fkovinAtRocket · 2019-05-03T10:58:18Z

What really concerns me though is that we're adding many functions whose name don't have prefixes, like: writeString(), writeHeader(), readByte(), indexOf(), etc. They might clash with other functions in the plugin code.

Some of them might be non-essential, but the functions in httpserver.c definitely must go there, and they all are unprefixed, with names like finishResponse(), writeBytes() writeRequest() etc. Note that it's absolutely required for a plugin to be able to read HTTP requests and send responses.

So I'm wondering if we should take the time and rename them to e.g. httpServerFinishResponse(), etc

ifakhrutdinov · 2019-05-03T14:13:23Z

Would we want to include the recovery.c functions (recoveryPush, recoveryPop, etc)? The recovery context is created by ZSS, so it's better if we provide those functions as they may be incompatible with the context if linked statically.

Signed-off-by: Fyodor Kovin <[email protected]>

1000TurquoisePogs · 2019-05-03T15:15:33Z

How far do we go here? charsets.c and xml.c also look like someone may want to use them.

fkovinAtRocket · 2019-05-03T15:35:57Z

As I though of this, the criteria is not so much whether someone wants to use it or not. The code is always there and they can link statically. Each new file added to this side deck is a potential pain for us, since everything that has once been added there becomes an API and thus a maintenance burden for us. Since this PR is approved, we won't be able to easily rename a function from http.c, for example, because someone's plugin will break and fail to load because a symbol will not be found.

Rather, the criteria was: include everything that's required for communication between ZSS and the plugin. Nothing good will happen if ZSS passes a plugin a HTTPRequest that was made by an incompatibly changed constructor function, and the plugin has statically linked an older version. HTTP, dataserivce, json, logging, recovery, even utils because of StringList are all shared between ZSS and plugins.

1000TurquoisePogs

This approach to side file generation should not be done for two reasons

It includes several internal functions that shouldn't be called at least due to them having no good meaning or purpose to plugins
It generates a nearly unreadable file that breaks our rule for 120 character line length limit. side file specification does not say that you cannot have a newline on the last byte of the 80 column line.

A good start would be a hand-crafted file including only useful functions, or, having a file generated by including only functions that are marked by some means as being valid for inclusion. It is easier to add on request as needed than to remove when people wrongly depended on what should have been an internal function that we didn't sufficiently protect in the first place.

Now is also not the right time to be attempting to rename internal functions to work-around the automated pitfalls. That could be significant effort and could cause significant problems for direct extenders of zowe-common-c.

fkovinAtRocket · 2019-05-03T20:27:31Z

Please define "useful function". This was my first question and it turned out that nobody knew what exactly it was. It looks like alloc.c might be safely removed, and we don't need all of the stuff in utils.c, but I would argue that logging, recovery and http/dataservice stuff need to be included in full, since they are functions that work with data/state shared by ZSS and plugins.
I'm not sure how collision-prone names are "automated pitfalls". They're pretty manual pitfalls instead: C doesn't have any namespaces, and any global function/variable name should be reasonably unique. If we're forcing every plugin to link with a symbol called "finishResponse" then chances are moderately high that they might also have a function with that name but a different meaning/arguments/return value: it's just a pretty common name for a function. If it were called "httpServerFinishResponse()" or "zoweHttpFinishResponse()", then, OTOH, chances would be much lower.
And this particular function finishResponse meets the definition of a "useful function" at least empirically: it can be observed that most plugins, and, generally HTTP endpoints, use it, to, well, finish an HTTP response. And it's also a function that works with data shared by ZSS and plugins, so it has to be part of the side file.
A side file is not a human readable file anyway, so I thought line breaks shouldn't matter. It's a coincidence, I think, that it's not binary in XL C, but it easily could've been. It looks like they followed the path of least resistance and made it a piece of binder control statements. Now, if we need line breaks in a non-human readable file for some reason, that's doable, of course.

1000TurquoisePogs · 2019-05-03T20:59:11Z

Side files are human-readable files when written to only include functions that plugins should be able to use, and exclude those they shouldn't rely on. They then serve as a form of documentation. What's a "useful" function will change over time, so we should start with a short list and only extend it when requested and the request has merit.

1000TurquoisePogs · 2019-05-03T21:02:24Z

Example of something that shouldn't be in the side file but is in this PR: dequeueHttpRequest

Signed-off-by: Fyodor Kovin <[email protected]>

fkovinAtRocket · 2019-05-06T17:18:12Z

It turns out that we actually need xml.c because there's a global function in httpserver that creates an XML object.

1000TurquoisePogs

I think this approach can work, but there's more to blacklist.

1000TurquoisePogs · 2019-05-07T22:22:00Z

lib/zss.x

+ IMPORT CODE,'zssServer','setConfiguredProperty'                               
+ IMPORT CODE,'zssServer','setContentType'                                      
+ IMPORT CODE,'zssServer','setDefaultJSONRESTHeaders'                           
+ IMPORT CODE,'zssServer','setHttpAuthTrace'                                    


I'd blacklist all the tracers... doesn't seem like something that plugins should be touching.

1000TurquoisePogs · 2019-05-07T22:24:37Z

lib/zss.x

+ IMPORT CODE,'zssServer','printStderr'                                         
+ IMPORT CODE,'zssServer','printStdout'                                         
+ IMPORT CODE,'zssServer','printXMLToken'                                       
+ IMPORT CODE,'zssServer','processHttpFragment'                                 


This seems too internal

1000TurquoisePogs · 2019-05-07T22:26:33Z

lib/zss.x

+ IMPORT CODE,'zssServer','logGetLevel'                                         
+ IMPORT CODE,'zssServer','logSetLevel'                                         
+ IMPORT CODE,'zssServer','logShouldTraceInternal'                              
+ IMPORT CODE,'zssServer','mainHttpLoop'                                        


Wouldn't be used by a plugin.

Signed-off-by: Fyodor Kovin <[email protected]>

…fication-of-addresses Adding ipAddresses field for zlux and agent(zss) for specifying binding to specific IPs

Side deck for plugins

fa5ea95

- side deck that a plugin can use to link with the functions in the server - script that I used to generate it - header file that has all the functions that are in the side deck Signed-off-by: Fyodor Kovin <[email protected]>

ghost assigned fkovinAtRocket May 2, 2019

ghost added in progress review labels May 2, 2019

jordanfilteau1995 self-requested a review May 2, 2019 17:24

jordanfilteau1995 reviewed May 2, 2019

View reviewed changes

ifakhrutdinov self-requested a review May 3, 2019 14:12

add http.o and recovery.o to the side deck

2477bbd

Signed-off-by: Fyodor Kovin <[email protected]>

1000TurquoisePogs requested changes May 3, 2019

View reviewed changes

add line breaks

891d207

Signed-off-by: Fyodor Kovin <[email protected]>

fkovinAtRocket force-pushed the feature/zss-side-deck branch 2 times, most recently from 686c21f to d90953b Compare May 6, 2019 17:05

adjust the bl/wl

4f5cd0e

Signed-off-by: Fyodor Kovin <[email protected]>

fkovinAtRocket force-pushed the feature/zss-side-deck branch from 6316f0c to 4f5cd0e Compare May 6, 2019 17:17

1000TurquoisePogs requested changes May 7, 2019

View reviewed changes

fkovinAtRocket added 2 commits May 8, 2019 11:21

blacklist a few more httpserver functions

a5feda6

Signed-off-by: Fyodor Kovin <[email protected]>

Side deck: add more utils and ZIS functions, remove mainHttpLoop

3dd0910

Signed-off-by: Fyodor Kovin <[email protected]>

sakeerthy pushed a commit to sakeerthy/zss that referenced this pull request May 11, 2020

Merge pull request zowe#30 from 1000TurquoisePogs/feature/allow-speci…

cd0ec64

…fication-of-addresses Adding ipAddresses field for zlux and agent(zss) for specifying binding to specific IPs

1000TurquoisePogs mentioned this pull request May 29, 2020

Make ZSS build fail when RC != 0 #188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Side deck for plugins #30

Side deck for plugins #30

fkovinAtRocket commented May 2, 2019

jordanfilteau1995 left a comment

fkovinAtRocket commented May 3, 2019 •

edited

Loading

fkovinAtRocket commented May 3, 2019 •

edited

Loading

ifakhrutdinov commented May 3, 2019 •

edited

Loading

1000TurquoisePogs commented May 3, 2019

fkovinAtRocket commented May 3, 2019 •

edited

Loading

1000TurquoisePogs left a comment

fkovinAtRocket commented May 3, 2019 •

edited

Loading

1000TurquoisePogs commented May 3, 2019

1000TurquoisePogs commented May 3, 2019

fkovinAtRocket commented May 6, 2019

1000TurquoisePogs left a comment

1000TurquoisePogs May 7, 2019

1000TurquoisePogs May 7, 2019

1000TurquoisePogs May 7, 2019

Side deck for plugins #30

Are you sure you want to change the base?

Side deck for plugins #30

Conversation

fkovinAtRocket commented May 2, 2019

jordanfilteau1995 left a comment

Choose a reason for hiding this comment

fkovinAtRocket commented May 3, 2019 • edited Loading

fkovinAtRocket commented May 3, 2019 • edited Loading

ifakhrutdinov commented May 3, 2019 • edited Loading

1000TurquoisePogs commented May 3, 2019

fkovinAtRocket commented May 3, 2019 • edited Loading

1000TurquoisePogs left a comment

Choose a reason for hiding this comment

fkovinAtRocket commented May 3, 2019 • edited Loading

1000TurquoisePogs commented May 3, 2019

1000TurquoisePogs commented May 3, 2019

fkovinAtRocket commented May 6, 2019

1000TurquoisePogs left a comment

Choose a reason for hiding this comment

1000TurquoisePogs May 7, 2019

Choose a reason for hiding this comment

1000TurquoisePogs May 7, 2019

Choose a reason for hiding this comment

1000TurquoisePogs May 7, 2019

Choose a reason for hiding this comment

fkovinAtRocket commented May 3, 2019 •

edited

Loading

fkovinAtRocket commented May 3, 2019 •

edited

Loading

ifakhrutdinov commented May 3, 2019 •

edited

Loading

fkovinAtRocket commented May 3, 2019 •

edited

Loading

fkovinAtRocket commented May 3, 2019 •

edited

Loading