Updated MLDEV device to handle 2- and 3-char ITS hostnames.

[eswenson1]

Addresses #2231.

Also a prior change to support the ITS ITSNMS table broke some of the case handling in MLDEV. This PR fixes those as well.

[larsbrinkhoff]

This is a big change which is difficult to understand fully. I'm posting a few pro-forma trite remarks.

[bictorv]

This is a big change which is difficult to understand fully. I'm posting a few pro-forma trite remarks.

I agree. I'll try to look into it later today.

[larsbrinkhoff]

I order to understand and better review the change, I jotted down some notes about what checks and actions happen:

1. Is it host+SHOE? Then host=host, device=SHOE.
2. Is it DIR+host? Then host=host, device=DIR.
3. Is it X+host? Then host=host, device=DSK.
4. Is it hh+DIR? Then host=hh, device=DIR.
5. Is it hhh and a known host? Then host=hhh, device=DSK.
6. Is it ddd and one of a plethora of special devices? Then host=special, device=special.
7. Is it hh+device? Then host=hh, device=device.
8. Is it hhh+device? Then host=hhh, device=device.
9. Else it must be just host=host. Not sure device is set?

[larsbrinkhoff]

I notice there are three routines involved in checking a host name.

1. isitsp runs through the internal list of known ITS hosts.
2. chkhst calls isitsp but ignores the result and then calls gothst.
3. gothst sets a flag whether the host name is two or three characters but the flag is unused. It then looks up the hostname in the hosts table.

It seems to me chkhst is somewhat unnecessary and we could probably call gothst directly. Some other places call isitsp for good reason, but having chkhst to call isitsp (whether it ignores the result or not) and then gothst looking up the hostname seems like the two checks mostly overlap.

[bictorv]

I order to understand and better review the change, I jotted down some notes about what checks and actions happen:

I did something similar but from scratch, and coded it up. It looks much simpler in my mind.

0. Check for xxSHOE
1. If RH is empty: Check for HH, HHH, or xHH
2. If len = 5, check for HHDDD or DDDHH
3. else check HHHDDD or DDDHHH

See if you agree? Note: I haven't even compiled the code. mldev.108.txt

[eswenson1]

I addressed all of Lars' comments. And yes, I think some of this could now be simplified. The hhhDIR case is the same as the hhhDDD case now. I'll look at Björn's code.

[eswenson1]

I order to understand and better review the change, I jotted down some notes about what checks and actions happen:

1. Is it host+SHOE? Then host=host, device=SHOE.
2. Is it DIR+host? Then host=host, device=DIR.
3. Is it X+host? Then host=host, device=DSK.
4. Is it hh+DIR? Then host=hh, device=DIR.
5. Is it hhh and a known host? Then host=hhh, device=DSK.
6. Is it ddd and one of a plethora of special devices? Then host=special, device=special.
7. Is it hh+device? Then host=hh, device=device.
8. Is it hhh+device? Then host=hhh, device=device.
9. Else it must be just host=host. Not sure device is set?

This looks right. You may be right that the fallback case at the end doesn’t set device. I’ll check on this. I wonder if we ever actually reach that case?

[eswenson1]

I notice there are three routines involved in checking a host name.

1. isitsp runs through the internal list of known ITS hosts.
2. chkhst calls isitsp but ignores the result and then calls gothst.
3. gothst sets a flag whether the host name is two or three characters but the flag is unused. It then looks up the hostname in the hosts table.

It seems to me chkhst is somewhat unnecessary and we could probably call gothst directly. Some other places call isitsp for good reason, but having chkhst to call isitsp (whether it ignores the result or not) and then gothst looking up the hostname seems like the two checks mostly overlap.

I got rid of chkhst — in an earlier version it did more, but I whittled it down and you are right, it is not useful. The calls to isitsp where the error return just proceeds were in the original code. I left them there but after your review, I got rid of them since, as you pointed out, they are really a noop.

[eswenson1]

I updated this PR again to fix a few bugs in MLDEV introduced by last update.

Tested these cases: hhSHOE, DIRhh, DIRhhh, hhDIR, hhhDIR, Xhh, Xhhh, hhDDD, hhhDDD, xgp, dvr, dvs, tpl, glp, and xxx, where xxx is an unknown (bad) device.

[larsbrinkhoff]

Seems to cover most cases!

[larsbrinkhoff]

FYI, I'm going to squash those fix commits into the first commit. Force push alert!

[bictorv]

It's really weird about me commenting on old code: I'm pretty sure I clicked on "files changed" and commented on the code presented there. Also when I look at the new code in the master branch, e.g. "jumpn e,hhhddd" still appears, and the other code I commented on also. Please double check that what appears in the repo is what you intended?

I think I agree on skipping ITSNMS, but then I think it should be skipped consistently. Better to check for a valid host name with HSTLOOK.

For the XGP etc hackery, what I suggested was to comment it out, not remove it. But as it is, it prevents us from using configs other than what is hardcoded here, doesn't it? Hardcoded machine names should not be used, I think, except for "aliases" such as local-time-server etc.

[eswenson1]

@bictorv: I just pulled master branch and I do see alignment between your comments and the code -- I'm not sure why I didn't think it matched before. I'm going to review all these comments again to make sure they don't reveal outstanding issues.

I also compared the current mldev.108 in master branch with the mldev.108 that I'm running on ES (where I did the development and testing) and they match as well. So I did test what I committed and what is on the master branch now. Still, I'll re-review your comments.

[eswenson1]

I have a vastly simplified version of MLDEV now, that appears to work. I've tested EX, EXA, EXDIR, EXADIR, DIREX, DIREXA, XGP, EXSHOE, EXSHO, XEX, XEXA.

Have I forgotten any cases? I'll submit a new PR for this version and WON'T MERGE IT until both Björn and Lars are happy. I'll also run with it and give it more of a workout on my various ITS systems.