[OpenAFS-devel] Patch for XML switch for vos examine
Sanket Agarwal
sanket@sanketagarwal.com
Thu, 6 May 2010 15:01:45 +0200
--00148502ae9b7f29ae0485ec8b31
Content-Type: text/plain; charset=ISO-8859-1
( 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 <scs@umich.edu> 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
--00148502ae9b7f29ae0485ec8b31
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
( sorry for getting off the list )<br>I would say that fixing a bug would b=
e 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 fu=
nction with all the switches in place is OK w.r.t having separate functions=
for each. IMHO both techniques have their trade-offs.<br>
<br><div class=3D"gmail_quote">On Wed, May 5, 2010 at 5:15 PM, Steve Simmon=
s <span dir=3D"ltr"><<a href=3D"mailto:scs@umich.edu">scs@umich.edu</a>&=
gt;</span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"border-left=
: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1e=
x;">
<div class=3D"im"><br>
On May 5, 2010, at 5:26 AM, Sanket Agarwal wrote:<br>
<br>
> Actually I have been thinking on the same lines for some time. The pat=
ch was very hacky I agree. And there needs to be definite change to the pat=
ch to a function which takes in the input as type of format. For example yo=
u can have something lilke DisplayByFormat(..., int type) etc.<br>
><br>
> But I think that's not going to save any redundancy. What I believ=
e you might end up with is something like,<br>
><br>
> In function DisplayByFormat you'll have if/switch-case or some oth=
er means of branching for each of the functionality of XML or RAW output. T=
his puts all the stuff in one place but if you want to still update it I=
9;ll have to do it at each of the places. For example:<br>
><br>
> if(type=3D=3DRAW)<br>
> =A0 =A0 fprintf(STDOUT, "RAW")<br>
> else if(type=3D=3DXML)<br>
> =A0 =A0 fprintf(STDOUT, "<xml>XML</xml>")<br>
><br>
> this would change to something like:<br>
><br>
> if(type=3D=3DXML)<br>
> =A0 =A0 fprintf(STDOUT, "XML")<br>
> else if(type=3D=3DRAW)<br>
> =A0 =A0 fprintf(STDOUT, "<raw>raw</raw>")<br>
><br>
> 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 ?<br>
<br>
</div>Yeah, and it actually starts getting uglier than that for repeating d=
ata. SubEnumerateFOO is fine for stock -format and -format with xml, but br=
eaks down horribly for comma-separated version. csv is intended for input i=
nto things like spreadsheets. As such, the number of fields has to be predi=
ctable. The way I had intended to handle things like RO copies of a RW volu=
me was by multiple outputs, one per copy:<br>
<br>
root.afs,536872193,server,partition,......,RW<br>
root.afs,536872193,server,partition,......,RO<br>
root.afs,536872193,server2,partitiona,......,RO<br>
root.afs,536872193,server3,partitionb,......,RO<br>
<br>
That doesn't fit well at all into the the current code where an Enumera=
te function is called after all the fixed data is printed. I have some idea=
s for how to do it better, but need to think them through at bit more.<br>
<br>
<br>
Stepping back to the original issue -<br>
<br>
We've had a number of serious bugs in AFS which were exacerbated by wha=
t 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 s=
imilar 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.=A0=A0</blockquote>
<blockquote class=3D"gmail_quote" style=3D"border-left: 1px solid rgb(204, =
204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">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 th=
e two sections, and either fix was right - but not both together. This was =
a nightmare to fix.<br>
<br>
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 pat=
ch 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 mom=
ent). 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 t=
he field and print 'Y' or 'N' to indicate if the volume is =
a shadow or not. In the 'one subroutine per output format' world, o=
ne has to change all the output functions to reflect the changed meaning of=
spare3. I'<br>
<br>
/* Print value of spare3 field as integer */<br>
if ( type =3D=3D RAW )<br>
=A0 =A0fprintf( STDOUT, "%spare3\t\t %d (Optional)\n", value );<=
br>
else if ( type =3D=3D XML )<br>
=A0 =A0fprintf( STDOUT, "\t\t<spare3>%d</spare3>\n",=
value );<br>
else if ( type =3D=3D CSV )<br>
=A0 =A0fprintf( STDOUT, "%,%d", value );<br>
<br>
gets changed to<br>
<br>
/* Print value of shadow bit */<br>
<br>
else if ( type =3D=3D CSV ) {<br>
=A0 =A0fprintf( STDOUT, ",%s", TagYorN( value ) );<br>
}<br>
<br>
if ( type =3D=3D RAW )<br>
=A0 =A0fprintf( STDOUT, "%spare3\t\t %d (Optional)\n", value );<=
br>
else if ( type =3D=3D XML )<br>
=A0 =A0fprintf( STDOUT, "\t\t<spare3>%d</spare3>\n",=
value );<br>
else if ( type =3D=3D CSV )<br>
=A0 =A0fprintf( STDOUT, ",%s", TagYorN( value );<br>
<br>
The last line assumes the existence of a helping function something like<br=
>
<br>
static char static *TagYorN( int val ) {<br>
=A0 =A0if ( val ) return "Y";<br>
=A0 =A0return "N"<br>
}<br>
<br>
In this case, all the changed needed to handle a changed data field are clo=
sely 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 separa=
te printing functions with just the scattering I'm trying to avoid.<br>
<br>
Comments and thoughts welcomed.<br>
<br>
Best,<br>
<font color=3D"#888888"><br>
Steve</font></blockquote></div><br>
--00148502ae9b7f29ae0485ec8b31--