[Info-vax] openvms and xterm
Dan Cross
cross at spitfire.i.gajendra.net
Fri May 3 21:04:22 EDT 2024
In article <v1366u$lf87$1 at dont-email.me>,
Dave Froble <davef at tsoft-inc.com> wrote:
>:-)
>
>The following is some snippets from what was basically a research program, and I
>consider commenting such should be rather terse. For production programs I'd
>want a lot more.
I hope you won't mind some feedback?
Overall, there's a concept of, "too much of a good thing."
>First, declare the purpose. Shouldn't this always be done?
Yes, but see below.
> !********************************************************************
> !
What does the line of asterisks buy you?
> ! Program: TCP_PEEK.BAS
Why do you need this? Is it not in a file that is already
named, "TCP_PEEK.BAS"? What do you get by repeating it in the
file itself?
> ! Function: Test Using TCP/IP Sockets as a Listener
> ! Version: 1.00
> ! Created: 01-Dec-2011
> ! Author(s): DFE
Why the elaborate formatting of this front matter? What does
some of it even mean? How did you decide that this was version
1.00, for example? (Something like semver is much more useful
than an arbitrary major.minor here.) Is the creation date
useful versus, say, the last modification date?
Moreover, where do you record the history of the module?
Knowing how code has evolved over time can be very useful.
Frankly, all of this is metadata, which is better captured in a
revision control system than in comments at the top of a file,
which can easily get out of date over time.
> !
> ! Purpose/description:
Well, which is it?
> !
> ! This program will set up TCP/IP sockets to allow
> ! itself to listen for connection requests. When
> ! a connection request is received, this program
> ! will accept the connection, and then attempt to
> ! PEEK the message, ie; read it but leave it available
> ! to be re-read.
This is good, but honestly, kind of all you need at the top of
the file.
>When using custom defined structures, it might be nice to know what they will be
>used for.
>
> !**************************************************
> ! Declare Variables of User Defined Structures
> !**************************************************
>
> DECLARE IOSB_STRUCT IOSB, ! I/O status blk &
> ITEMLIST_2 SERVER.ITEMLST, ! Server item list &
> ITEMLIST_2 SOCKOPT.ITEMLST, ! Socket options list &
> ITEMLIST_2 REUSEADR.ITEMLST, ! Reuse adr list &
> ITEMLIST_3 CLIENT.ITEMLST, ! Client item list &
> SOCKET_OPTIONS LISTEN.OPTN, ! Socket options &
> SOCK_ADDR CLIENT.ADR, ! Client IP adr/port &
> SOCK_ADDR SERVER.ADR, ! Server IP adr/port &
> BUFF CLIENT.NAME, ! Client name buffer &
> BUFF SERVER.NAME, ! Server name buffer &
> IP_ADR IP, ! Ip address &
> BUFF MSG ! Message buffer
I don't think that these comments are useful at all, with the
possible exception of the one on IOSB. These comments just
parrot the code. If I saw, "SOCKOPT.ITEMLIST" how is it not
obvious that that is a, "Socket options list"? Note also that
the comment omits the fact that it's an item list, which is a
specific thing, rather than just a generic list. Half of the
item lists are annotated as item lists in the comments, but
the other half are not.
>I consider the following rather terse. Either the programmer knows how to use
>system services, or perhaps remedial training is called for.
>
> !**************************************************
> ! Assign channels to 'TCPIP$DEVICE:'
> !**************************************************
>
> Dev$ = "TCPIP$DEVICE:"
>
> Stat% = SYS$ASSIGN( Dev$ , ListenCh% , , )
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to assign listener channel - "; E$
> GoTo 4900
> End If
>
> Print #KB%, "Internal VMS channel for listener socket:"; ListenCh%
>
> Stat% = SYS$ASSIGN( Dev$ , ClientCh% , , )
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to assign client channel - "; E$
> GoTo 4900
> End If
It's also rather repetitive. It'd be better to wrap assignment
in some kind of helper function, IMHO. Without knowing more
about VMS BASIC, however, it's difficult to tell whether one
could do much better.
>However, when details might be helpful, there is usually never too much.
>
> !**************************************************
> ! Create Listener socket
> ! Bind server's IP address and port # to listener
> ! socket, set socket as a passive socket
I'm not sure this comment is accurate: it appears that most of
what this code is doing until the $QIOW is setting up data for
the $QIOW. Something like this may be more useful:
! Initialize IOSB data for listener socket creation,
! then create the socket and bind it to the server's
! IP address with the given port number.
> ! Note: we used to do this in 2 calls, but can be combined
Grammar: "but they were combined." Again, we see how missing
the history can lead to questions: why were they combined, and
when? Is that better somehow, other than the general principle
of doing less work?
> !**************************************************
>
> LISTEN.OPTN::PROTOCOL% = TCPIP$C_TCP ! Listener socket optn
> LISTEN.OPTN::TYP$ = Chr$(TCPIP$C_STREAM)
> LISTEN.OPTN::AF$ = Chr$(TCPIP$C_AF_INET)
>
> SOCKOPT.ITEMLST::LEN% = 8% ! Socket options buffer
> SOCKOPT.ITEMLST::TYP% = TCPIP$C_SOCKOPT
> SOCKOPT.ITEMLST::ADR% = Loc(REUSEADR.ITEMLST::Len%)
>
> REUSEADR.ITEMLST::LEN% = 4% ! Reuse adr (port #)
> REUSEADR.ITEMLST::TYP% = TCPIP$C_REUSEADDR
> REUSEADR.ITEMLST::ADR% = Loc(ReuseAdrVal%)
>
> ReuseAdrVal% = 1% ! Set to 'True'
>
> SERVER.ITEMLST::LEN% = 16% ! Server item list
> SERVER.ITEMLST::TYP% = TCPIP$C_SOCK_NAME
> SERVER.ITEMLST::ADR% = Loc(SERVER.ADR::Fam%)
>
> SERVER.ADR::Fam% = TCPIP$C_AF_INET ! Server Ip adr/port
> SERVER.ADR::PORT% = SWAP%(ServerPort%)
> SERVER.ADR::IP.ADR% = TCPIP$C_INADDR_ANY
> SERVER.ADR::ZERO1% = 0%
> SERVER.ADR::ZERO2% = 0%
>
> BACKLOG% = 1%
>
> Stat% = SYS$QIOW( , ! Event flag &
> ListenCh% By Value, ! VMS channel &
> IO$_SETCHAR By Value, ! Operation &
> IOSB::Stat%, ! I/O status block &
> , ! AST routine &
> , ! AST parameter &
> LISTEN.OPTN::Protocol%, ! P1 &
> , ! P2 &
> SERVER.ITEMLST::Len%, ! P3 - local socket nam
>e &
> BACKLOG% By Value, ! P4 - connection backl
>og &
> SOCKOPT.ITEMLST::Len%, ! P5 - socket options &
> ) ! P6
>
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to queue create and bind listener socket - "
>; E$
> GoTo 4900
> End If
>
> If ( IOSB::Stat% And SS$_NORMAL ) = 0%
> Then Stat% = IOSB::Stat%
> E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to create and bind listener socket - "; E$
> GoTo 4900
> End If
>
>My opinion is, the above is essential, without it, there would be much studying
>of code, wondering what is being referenced, and such. I always use one line
>for each argument in a QIO and such, which makes it very clear what is
>happening. Without that, even the best will still have some "fun" reading the
>code to figure out what is happening.
I agree that well-commented code is useful.
- Dan C.
More information about the Info-vax
mailing list