danomagnum / gologix

Ethernet/IP client library for Go inspired by pylogix that aims to be easy to use. Supports being a client and a class 3 / class 1 server.
MIT License
32 stars 8 forks source link

ReadMulti() potential problem due to struct having some non-gologix tags #15

Closed scottboon closed 5 months ago

scottboon commented 5 months ago

In ReadMulti(), the range that builds the lists for the ReadList() call, I believe it isn't quite right.

I believe this code

    for i := range vf {
        field := vf[i]                                                                                                                  
        tagpath, ok := field.Tag.Lookup("gologix")                                                                                      
        v := val.Field(i).Interface()                                                                                                   
        ct, elem := GoVarToCIPType(v)                                                                                                   
        types = append(types, ct)                                                                                                       
        elements = append(elements, elem)
        if !ok {
            continue
        }
        tags = append(tags, tagpath)                                                                                                    
        tag_map[tagpath] = i
    }                                                                                                                                   

should be changed to perform the !ok check before appending to types and elements. Ie, this:

    for i := range vf {
        field := vf[i]                                                                                                                  
        tagpath, ok := field.Tag.Lookup("gologix")                                                                                      
        if !ok {
            continue
        }
        v := val.Field(i).Interface()                                                                                                   
        ct, elem := GoVarToCIPType(v)                                                                                                   
        types = append(types, ct)                                                                                                       
        elements = append(elements, elem)
        tags = append(tags, tagpath)                                                                                                    
        tag_map[tagpath] = i
    }                                                                                                                                   

By doing this, ReadList() now has correct arguments wherein the indexes correctly correspond to all "gologix" tagged fields in the structure. By not doing this, ReadList() would associate a tag with an incorrect type and element.

Consider the following struct...

type pollDataMix struct {
    TruthOne   bool `gologix:"BOOL_ONE"`
    TruthTwo   bool `gologix:"BOOL_TWO"`
    LocalFlag1 uint64
    InspectTO  int16 `gologix:"INSPECT_TIMEOUT"`
    LocalFlag2 uint64
    LocalFlag3 uint64
}       

ReadList() would see a len(tags)=3, len(types)=6, len(elements)=6. And field InspectTO (index 2) would be assigned an incorrect type (LWORD instead of an INT). I can't see an issue with changing the code. Both of the affected lists (types, elements) are not used outside of being passed into ReadList() where they should reflect the tags list.

danomagnum commented 5 months ago

Good catch. I got the patch in and added a test for it.

Beta 20.3 is now released with the fix. It should make it up to the go package mirrors in a little bit.