New and improved? patch

[Dieter]

>> Freshly updated with new found information on what the heck -f -g 
>> actually do!
>> 
>> See how this behaves for you.

usage:
> [-f force_root ]

Suggestion:  [-f force_root_node ] or maybe "desired_root_node" or maybe
"root_node" or maybe just "node", same as -c -d -o -s

> -g: broadcast gap_count by phy_config packet

Suggestion: -g: set gap_count by broadcasting phy_config packet
or maybe just: -g: set gap_count

> -f: broadcast force_root by phy_config packet

Suggestion: -f force a node to become the root node by broadcasting phy_config packet
or maybe just: -f force a node to become the root node

7.0 AMD64 # ./fwcontrol -f -5
fwcontrol: main:set_root_node out of range: No such file or directory

"No such file or directory" seems wrong

7.0 AMD64 # ./fwcontrol -g 70
fwcontrol: main:set_gap_count out of range: No such file or directory

"No such file or directory" seems wrong

7.0 AMD64 # ./fwcontrol -d 70
fwcontrol: no such node -1.

(a) 70 becomes -1 ?
(b) Wouldn't -d have the same range as -f ?

7.0 AMD64 # ./fwcontrol -c 70
fwcontrol: no such node -1.

(a) 70 becomes -1 ?
(b) Wouldn't -c have the same range as -f ?

7.0 AMD64 # ./fwcontrol -o 70
fwcontrol: main: node out of range: 70
: No such file or directory

"No such file or directory" seems wrong

7.0 AMD64 # ./fwcontrol -s 70
fwcontrol: main: node out of range: 70
: No such file or directory

"No such file or directory" seems wrong



-               asyreq->pkt.mode.ld[1] |= (root_node & 0x3f) << 24 | 1 << 23;
+               asyreq->pkt.mode.ld[1] |= ((root_node << 24) | (1 << 23));
        if (gap_count >= 0)
-               asyreq->pkt.mode.ld[1] |= 1 << 22 | (gap_count & 0x3f) << 16;
+               asyreq->pkt.mode.ld[1] |= ((1 << 22) | (gap_count << 16));

Any reason for pulling out the "& 0x3f" ?  Yeah it should be redundant
now that there is range checking on the arguments, but as you said,
fwcontrol is dangerous and the mask makes it safer at very little cost.


-               for (i = 0; i < 4; i++) {
-                       snprintf(name, sizeof(name), "%s.%d", devbase, i);
-                       if ((*fd = open(name, O_RDWR)) >= 0)
-                               break;
-               }
+               *fd = open(devname, O_RDWR);

Looking at various firewire man pages, I don't find any explanation of
the various /dev filenames, such as what the .%d part was/is for.  So I
have no clue why this code was changed.  Did I miss a discussion?