|
Hi Emmanuel,I'll give you my opinions on your code. Some of my suggestions relate to actual bugs in your implementation, and others aren't causing problems, but just make maintenance more difficult or making the code more confusing. Here are my thoughts:
[SNIP]
midrange.com
0005.00 * Open - Open an IFS file
0006.00 D open PR 10I 0 ExtProc('open')
0007.00 D filename * Value
0008.00 D openflags 10I 0 Value
0009.00 D mode 10U 0 Value options(*nopass)
0010.00 D codepage 10U 0 Value options(*nopass)
Unless you need your code to work at V3R2, you should have OPTIONS(*STRING) on the filename parameter. That way, you don't need to do the extra work of adding x'00' to the filename, and you don't need to pass a pointer to the API.
That will make your code easier to read, and will save you work when writing it.
Also, you should have your prototypes, constants and data structures that are related to the IFS APIs in a /COPY member. Here's why:
a) You won't have to redefine them every time you write a program, this reduces the potential for errors and saves you time.
b) It ensures that the definitions consistent across all programs. That way, someone reading your code won't make incorrect assumptions about how the prototype works, and won't have to study the prototypes in every program.
c) Because you don't list all of the definitions in your source code, directly, the code is shorter, simpler and easier to read.
0014.00 D access PR 10I 0 ExtProc('access')
0015.00 D filename * Value
0016.00 D modeI 10U 0 Value
Options(*STRING) shoudl also be added to the filename here.
0034.00 D unlink PR 10I 0 ExtProc('unlink')
0035.00 D filename * Value
Options(*STRING) should also be added here.
0063.00 D CodePage S 10U 0 Inz(437)
I'd recommend against using 437 as a codepage unless you're still working with old copies of MS-DOS. Instead, consider using 1252 (Windows Latin-1).
0067.00 * Values for oflag parameter, used by open() 0068.00 D O_APPEND S 10I 0 Inz(256) 0069.00 D O_CODEPAGE S 10I 0 Inz(8388608) 0070.00 D O_CREAT S 10I 0 Inz(8) 0071.00 D O_EXCL S 10I 0 Inz(16) 0072.00 D O_RDONLY S 10I 0 Inz(1) 0073.00 D O_RDWR S 10I 0 Inz(4) 0074.00 D O_TEXTDATA S 10I 0 Inz(16777216) 0075.00 D O_TRUNC S 10I 0 Inz(64) 0076.00 D O_WRONLY S 10I 0 Inz(2)
These should all be constants rather than variables!! You don't want someone to be able to change the value of O_TEXTDATA, for example. You want the name O_TEXTDATA to be a synonym for 16777216.
Making these constants instead of variables makes your code more self-documenting, because you make it clear that the names (like O_TEXTDATA) are symbolic representations of the numbers.
Since you used variables here, the next guy who reads the code could potentially think that these are not symbolic representations, but rather are variables that control something.
For example, he could think that you want to append to the file 256 times and that if he wanted to change the numebr of times, he should change that variable. That would be incorrect thinking, but it's a conclusion you could lead him to making these variables instead of constants.
0078.00 * User authorities for omode paramter, used by open() 0079.00 * from QSYSINC/SYS, member STAT 0080.00 D S_IRUSR S 10I 0 Inz(256) 0081.00 D S_IWUSR S 10I 0 Inz(128) 0082.00 D S_IXUSR S 10I 0 Inz(64) 0083.00 D S_IRWXU S 10I 0 Inz(448) 0084.00 0085.00 * Group authorities 0086.00 D S_IRGRP S 10I 0 Inz(32) 0087.00 D S_IWGRP S 10I 0 Inz(16) 0088.00 D S_IXGRP S 10I 0 Inz(8) 0089.00 D S_IRWXG S 10I 0 Inz(56) 0090.00 0091.00 * Other authorities 0092.00 D S_IROTH S 10I 0 Inz(4) 0093.00 D S_IWOTH S 10I 0 Inz(2) 0094.00 D S_IXOTH S 10I 0 Inz(1) 0095.00 D S_IRWXO S 10I 0 Inz(7)
Same problem here. These should be constants rather than variables!!
0101.00 D Null C Const(x'00')
I'd try to avoid using "Null" in your program, but if you do have to use it, this is a good way to do it. I'm happy to see that this one, at least, is a constant.
0105.00 // Set Pc file details 0106.00 L = Pr_SetFile (); 0107.00 If L = -1; 0108.00 0109.00 ExSr *PSSR; 0110.00 0111.00 EndIf; 0112.00 0113.00 // Open PC file in IFS directory 0114.00 L = Pr_OpnFile (); 0115.00 If L = -1; 0116.00 0117.00 ExSr *PSSR; 0118.00 0119.00 EndIf;
The reason you call Pr_SetFile() is to create a file with the right attributes in the IFS. Then you call Pr_OpnFile() to open that file and turn on ASCII/EBCDIC translation.
The problem is, you never close the file you opened with Pr_SetFile(). So, instead of creating it and then opening it, you've resulted in opening it twice, in two different modes.
0123.00 C eval nbytesread = read(FileDescI:%addr(bufI): 0124.00 C nbyteset) 0125.00 0126.00 C If nbytesread > 0 0127.00 C Eval *InLr = *On 0128.00 C Else 0129.00 C Callp DspError 0130.00 C EndIf
In Pr_OpnFile() you set a variable called Err_Flag. That's a confusing name for what is actually a file descriptor. This descriptor needs to be passed to every API that you want to work with that file.
That's how the operating system keeps track of all of the different files you have open. Each time you open a new one, it assigns it a number. Then when you want to operate on that file, you pass the number back to the API that operates on it.
In this case, however, you're not doing that. Err_Flag is the file descriptor, but you're passing something else (FileDescI) as a parameter to read(). So, the read() won't work. I assume that this is a copy/paste error, since it wouldn't provide the error message that you referred to.
0132.00 ********************************************************************** 0133.00 * *Inzsr - Program level initialisation 0134.00 ********************************************************************** 0135.00 C *Inzsr BegSr 0136.00 0137.00 * Received Parameters 0138.00 C *Entry Plist 0139.00 C Parm PrmFileName 0140.00 C Parm PrmPssrErr 0141.00 0142.00 /free 0143.00 0144.00 // Default path name 0145.00 Filename = %Trim(PrmFileName); 0146.00 0147.00 /end-free 0148.00 0149.00 C EndSr
In this routine you use %trim() to remove the blanks from PrmFileName. Unfortunately, it doesn't work. The "Filename" field is a fixed-length 80 character field. It always has to have 80 characters in it, it can never have less.
When you assign something that's less than 80 characters to an 80-character field, what does it do? How does it ensure that the field has all 80 chars filled it?
It adds blanks. So, in essence, this is what your %trim() line does: a) Gets the contents of the PrmFileName variable. b) Removes all of the blanks from it. c) Adds blanks back on until the field is 80 chars long.That's confusing, and it makes you look like you don't know what you're doing. If you want to remove teh blanks and have them stay off, use a VARYING field for "filename".
If your goal is to only remove the blanks from the left (which is the net result) and not from the right end of the string, then use %TRIML, it'll be faster.
Alternately, just use PrmFileName directly on the open() call, and put the %trim() right there on the open() statement. That way, the output without the blanks won't ever be assigned to a fixed-length string, instead it'll be passed to the API.
0171.00 ********************************************************************** 0172.00 * Pr_SetFile - Set PC file details 0173.00 ********************************************************************** 0174.00 P Pr_SetFile B 0175.00 0176.00 D Pr_SetFile PI 1 0 0177.00 0178.00 /free 0179.00 0180.00 Oflag = O_CREAT + O_CODEPAGE + O_RDWR + O_TRUNC; 0181.00 0182.00 Omode = S_IRWXU + S_IRWXG + S_IROTH + S_IXOTH; 0183.00 0184.00 Filename = %Trim(FileName) + Null; 0185.00 0186.00 /end-free 0187.00 0188.00 C Eval Err_Flag = Open(%Addr(Filename) : OFLAG : 0189.00 C Omode: CodePage) 0190.00 0191.00 /free 0192.00 0193.00 If Err_Flag < 0; 0194.00 Return -1; 0195.00 Else; 0196.00 Return 0; 0197.00 EndIf; 0198.00 0199.00 /end-free 0200.00 0201.00 P Pr_SetFile E
I don't understand why this routine switches in and out of free format. Why not make the open() API call be free-format? That would be easier to read, since you'd have more space.
Also, as I mentioned above:a) you should be using options(*string) so you don't have to manually add NULL, and you don't have to use %addr() with the open() API.
b) you should remember to close the file here.c) Subprocedures should avoid using global variables if possible, and in this case, it would be easy to do so. Why use globals? All it does is make your code less reusable.
0205.00 ********************************************************************** 0206.00 * Pr_OpnFile - Open PC File in IFS directory 0207.00 ********************************************************************** 0208.00 P Pr_OpnFile B 0209.00 0210.00 D Pr_OpnFile PI 1 0 0211.00 0212.00 /free 0213.00 0214.00 Oflag = O_WRONLY + O_TEXTDATA; 0215.00 0216.00 /end-free 0217.00 0218.00 C Eval Err_Flag = Open(%Addr(Filename) : Oflag) 0219.00 0220.00 /free 0221.00 0222.00 If Err_Flag < 0; 0223.00 Return -1; 0224.00 Else; 0225.00 Return 0; 0226.00 EndIf; 0227.00 0228.00 /end-free 0229.00 0230.00 P Pr_OpnFile E
Here you've opened the file for WRITING ONLY (O_WRONLY), but the mainline tries to use the read() API to read it.
That's why you're getting an ENOTSUPP error. Because it doesn't support reading from a write-only file.
0236.00 P DspError B 0237.00 0238.00 D DspError PI 0239.00 0240.00 D text S 10A 0241.00 0242.00 D errorNo@ S * Inz 0243.00 D errorNo S 10I 0 Based(Errorno@) 0244.00 D errorMsg@ S * Inz 0245.00 D errorMsg S 100A Based(errorMsg@) 0246.00 D errortxt S 52A Inz(*Blanks) 0247.00 0248.00 0249.00 /free 0250.00 0251.00 errorNo@ = GetErrNo; 0252.00 0253.00 errormsg@ = StrError(errorNo); 0254.00 0255.00 errortxt = %trim(text) + '->' + 0256.00 %char(errorNo) + 0257.00 ':' + %subSt(errormsg:1:37); 0258.00 /end-free 0259.00 0260.00 0261.00 0262.00 P DspError E
The string that StrError() points to isn't 100A. It can be any length from zero to hundreds of bytes long. The way you can tell when you reach the end of the string is that it'll have a null (x'00') character.
Unfortunately, this code doesn't take that into consideration. Instead, it just takes the first 100 bytes of the error message (whether the message is that long or not!) and reads it.
Then, after making the code look at 100 bytes of memory, even though it doesn't make sense, you turn around and chop off the first 37 bytes of that 100 bytes. (Makes me wonder why you used 100A instead of 37A in the first place)
The code is confusing and incorrect.Instead, get rid of the errormsg@ and errormsg fields. They're unnecesary. Make the routine look like this:
D errorNo@ S * Inz
D errorNo S 10I 0 Based(Errorno@)
D errortxt S 52A Inz(*Blanks)
/free
errorNo@ = GetErrNo;
errortxt = %trim(text) + '->' +
%char(errorNo) +
':' + %str(strerror(errorNo));
The %STR() BIF will read the C-style (null-terminated) string from memory
and convert it to an RPG style one. I can add the result of %STR() right
onto my error message.
Anyway, I hope that helps.
As an Amazon Associate we earn from qualifying purchases.
This mailing list archive is Copyright 1997-2024 by midrange.com and David Gibbs as a compilation work. Use of the archive is restricted to research of a business or technical nature. Any other uses are prohibited. Full details are available on our policy page. If you have questions about this, please contact [javascript protected email address].
Operating expenses for this site are earned using the Amazon Associate program and Google Adsense.
