( sorry for getting off the list ) I would say that fixing a bug would be a lot of trouble in case we have separate functions because we are doing a lot of code copying and hence bug copying. I now think having a single function with all the switches in place is OK w.r.t having separate functions for each. IMHO both techniques have their trade-offs.
On Wed, May 5, 2010 at 5:15 PM, Steve Simmons <[email protected]> wrote: > > On May 5, 2010, at 5:26 AM, Sanket Agarwal wrote: > > > Actually I have been thinking on the same lines for some time. The patch > was very hacky I agree. And there needs to be definite change to the patch > to a function which takes in the input as type of format. For example you > can have something lilke DisplayByFormat(..., int type) etc. > > > > But I think that's not going to save any redundancy. What I believe you > might end up with is something like, > > > > In function DisplayByFormat you'll have if/switch-case or some other > means of branching for each of the functionality of XML or RAW output. This > puts all the stuff in one place but if you want to still update it I'll have > to do it at each of the places. For example: > > > > if(type==RAW) > > fprintf(STDOUT, "RAW") > > else if(type==XML) > > fprintf(STDOUT, "<xml>XML</xml>") > > > > this would change to something like: > > > > if(type==XML) > > fprintf(STDOUT, "XML") > > else if(type==RAW) > > fprintf(STDOUT, "<raw>raw</raw>") > > > > In terms of modularity we don't gain much ? Besides if we dump all of > them in one place it becomes tougher for a new format to be supported. IMHO > it's not the most brilliant idea ? > > Yeah, and it actually starts getting uglier than that for repeating data. > SubEnumerateFOO is fine for stock -format and -format with xml, but breaks > down horribly for comma-separated version. csv is intended for input into > things like spreadsheets. As such, the number of fields has to be > predictable. The way I had intended to handle things like RO copies of a RW > volume was by multiple outputs, one per copy: > > root.afs,536872193,server,partition,......,RW > root.afs,536872193,server,partition,......,RO > root.afs,536872193,server2,partitiona,......,RO > root.afs,536872193,server3,partitionb,......,RO > > That doesn't fit well at all into the the current code where an Enumerate > function is called after all the fixed data is printed. I have some ideas > for how to do it better, but need to think them through at bit more. > > > Stepping back to the original issue - > > We've had a number of serious bugs in AFS which were exacerbated by what I > call "copy-and-pasteware". If you look over time at the code that does vos > move and vos copy, you'll see that they clearly do very similar functions. > But rather than isolating those similarities into smaller functions, > somebody did a lot of copy and paste of code. Later, bugs were found in the > locking code. Sometimes the bug was found in the move function, but the fix not applied to > the copy code. Sometimes it was the opposite. Sometimes different fixes were > applied to the two sections, and either fix was right - but not both > together. This was a nightmare to fix. > > I was concerned that we might be introducing the same problem into > XML/CSV/vanilla output from vos examine. We're running with an experimental > patch for shadow volumes, a RO copy of a volume that doesn't appear in the > vldb (sounds odd, I know, but just assume it's a good idea for the moment). > We take over the spare3 field for that. As part of the implementation, we > modified the output of 'vos e -format' to change the name of the field and > print 'Y' or 'N' to indicate if the volume is a shadow or not. In the 'one > subroutine per output format' world, one has to change all the output > functions to reflect the changed meaning of spare3. I' > > /* Print value of spare3 field as integer */ > if ( type == RAW ) > fprintf( STDOUT, "%spare3\t\t %d (Optional)\n", value ); > else if ( type == XML ) > fprintf( STDOUT, "\t\t<spare3>%d</spare3>\n", value ); > else if ( type == CSV ) > fprintf( STDOUT, "%,%d", value ); > > gets changed to > > /* Print value of shadow bit */ > > else if ( type == CSV ) { > fprintf( STDOUT, ",%s", TagYorN( value ) ); > } > > if ( type == RAW ) > fprintf( STDOUT, "%spare3\t\t %d (Optional)\n", value ); > else if ( type == XML ) > fprintf( STDOUT, "\t\t<spare3>%d</spare3>\n", value ); > else if ( type == CSV ) > fprintf( STDOUT, ",%s", TagYorN( value ); > > The last line assumes the existence of a helping function something like > > static char static *TagYorN( int val ) { > if ( val ) return "Y"; > return "N" > } > > In this case, all the changed needed to handle a changed data field are > closely connected in the code rather than being scattered all over the > place. But I think the problems with enumeration are going to force us into > separate printing functions with just the scattering I'm trying to avoid. > > Comments and thoughts welcomed. > > Best, > > Steve
