-
Notifications
You must be signed in to change notification settings - Fork 30
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
Create Qsam data set #109
base: v1.x/staging
Are you sure you want to change the base?
Create Qsam data set #109
Conversation
Signed-off-by: Steven Bollinger <[email protected]>
c/datasetjson.c
Outdated
} | ||
|
||
/* Allocate structure in A31 space */ | ||
EXTF *bpxwdyn=(EXTF *)fetch("BPXWDYN"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no matching release()
for this fetch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing that I have read has mentioned anything about releasing an item that has been fetched. What is your source for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source is in the manual. It won't leak resources because the CDE has a use count but it's always good to clean up. You could stash the bpxwdyn
pointer in a static variable and fetch it once. It will get released when the task ends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info. None of the examples on bpxwdyn has the release function shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I'm not sure that it won't leak memory because there is a fetch control block associated with fetch used by LE. The underlying LOAD macro used by fetch is fine but I would still always call DELETE when I use a LOAD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fetch issue not related to BPXWDYN. fetch is is just a callable service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would assume that any example showing how to use bpxwdyn would also show the release of the function since it already show the fetch of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
c/datasetjson.c
Outdated
strncpy( below2G->request, baseRequest, sizeof(below2G->request)); | ||
|
||
/* Initialize the DD/FI field */ | ||
ptr = (request->ddName == NULL ? QSAM_DEFAULT_DDNAME: request->ddName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have code for dynalloc in https://github.com/zowe/zowe-common-c/blob/master/c/dynalloc.c, you should re-use/enhance code from there.
h/datasetjson.h
Outdated
@@ -62,6 +63,7 @@ void respondWithDataset(HttpResponse* response, char* absolutePath, int jsonMode | |||
void respondWithVSAMDataset(HttpResponse* response, char* absolutePath, hashtable *acbTable, int jsonMode); | |||
void respondWithDatasetMetadata(HttpResponse *response); | |||
void respondWithHLQNames(HttpResponse *response, MetadataQueryCache *metadataQueryCache); | |||
int qsamAllocFile( qsamFileRequest *request, char* message, int messageLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no QSAM files or datasets, QSAM is an access method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From earlier comment. I am open to any name change from qsam. Following names in the jira ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ifakhrutdinov is correct. I would name it allocDataSet
which covers both PS and PDS(E) data sets.
Signed-off-by: Steven Bollinger <[email protected]>
Steve noted that strtok used here is not thread safe, so tokenizing alternatives may be needed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we use SVC99 instead of passing commands and then parsing the results.
char *expiration; | ||
char *spaceUnit; /* BLKS, TRKS, CYCLS, KB, MB, BYTES */ | ||
int dirBlk; | ||
} dataSetRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We start our own types with a capital letter.
char *spaceUnit; /* BLKS, TRKS, CYCLS, KB, MB, BYTES */ | ||
int dirBlk; | ||
} dataSetRequest; | ||
int allocDataSet( dataSetRequest *request, char* message, int messageLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please remove the blank char between
allocDataSet(
anddataSetRequest *request,
. - Is the content of message changed inside allocDataSet? If not, please use const.
- Please use the dynalloc prefix for new functions.
- I believe
size_t
would be more appropriate to represent a size.
} | ||
|
||
/* Allocate structure in A31 space */ | ||
EXTF *bpxwdyn=(EXTF *)fetch("BPXWDYN"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work in Metal C. Why not use the invokeSVC99
function?
@@ -29,6 +29,7 @@ | |||
#include "zowetypes.h" | |||
#include "alloc.h" | |||
#include "dynalloc.h" | |||
#include "logging.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're introducing a new dependency. Can we work out a different way of returning debug info? Perhaps return and reason codes.
strncpy(buffer, baseRequest, bufferSize); | ||
|
||
/* Initialize the DD/FI field */ | ||
ptr = (request->ddName == NULL ? DATASET_DEFAULT_DDNAME: request->ddName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the existing functions in this file, they use a programmatic way to pass this information.
return status; | ||
} | ||
|
||
static void allocDataSetCloseString(dataSetRequest *request, char *buffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about reusing dynallocUnallocDatasetByDDName
and the other unalloc functions?
|
||
/* Initialize organization */ | ||
if (request->organization != NULL) { | ||
snprintf(field, sizeof(field)-1, "DSORG(%s) ", request->organization); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snprintf
may failsnprintf
limit doesn't require a-1
/* Initialize record format */ | ||
if (request->recordFormat != NULL) { | ||
snprintf(field, sizeof(field)-1, "RECFM(%s) ", request->recordFormat); | ||
strncat(buffer, field, bufferSize - strlen(buffer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if strlen(buffer)
is greater than bufferSize
, the result will wrap around, you'll get a huge positive number and a buffer overflow.
Signed-off-by: Steven Bollinger [email protected]