On Wed, Jul 18, 2012 at 11:59:27PM +0530, Arun Sharma wrote: > Please let me know your comments.
See inline. > On 7/14/12 9:20 AM, "Arun Sharma" <[email protected]> wrote: > > >Please see comments inline. > > > >On 7/13/12 10:08 PM, "Ben Pfaff" <[email protected]> wrote: > > > >>On Wed, Jul 11, 2012 at 07:41:59PM -0700, Arun Sharma wrote: > >>> Option --ovs is added for ovs-bugtool command to collect > >>> only OpenvSwitch relevant information. To perform > >>> filtering in plugins, a new xml attribute filters="ovs" (optional) > >>> would be required in element 'command','files','directory' in > >>> openvswitch.xml. Value of 'filters' attribute will be compared > >>> with filtering option in load_plugins to get all relevant operation > >>> to collect information. If no "--ovs" option is passed then it will > >>> behave as earlier. > >>> > >>> Fixed an issue which occurs in scenario where option '--yestoall' > >>> is not passed and user keeps entering "y" or "n" on prompt. > >>> > >>> Plus, trailing whitespaces are fixed. White space before '=' and > >>> after in function def and call is also fixed. > >>> > >>> Signed-off-by: Arun Sharma <[email protected]> > >> > >>It looks to me like exactly one of only_ovs_info or collect_all_info > >>is always set. Is that true? If it is, then why would we ever write > >>"if collect_all_info or only_ovs_info:", as below, since it would > >>always be true: > > > >[Arun] - Yes, exactly one will be true. Currently the command passed in > >cmd_output is applicable for both collect_all_info and only_ovs_info. > > But in future if any other filtering requirement comes then this > >cmd_output call may not be applicable. > > I think we can remove this condition for now. Pl. confirm. Please remove it. > >>> - for b in bond_list(vspid): > >>> - cmd_output(CAP_NETWORK_STATUS, > >>> - [OVS_APPCTL, '-t', > >>>'@RUNDIR@/ovs-vswitchd.%s.ctl' % vspid, '-e' 'bond/show %s' % b], > >>> - 'ovs-appctl-bond-show-%s.out' % b) > >>> + if collect_all_info or only_ovs_info: > >>> + for b in bond_list(vspid): > >>> + cmd_output(CAP_NETWORK_STATUS, > >>> + [OVS_APPCTL, '-t', > >>> + '@RUNDIR@/ovs-vswitchd.%s.ctl' % > >>>vspid, '-e' 'bond/show %s' % b], > >>> + 'ovs-appctl-bond-show-%s.out' % b) > >>> except e: > >>> pass > >> > >>This commit changes the plugins, adding a new "filter" attribute. The > >>plugins are used not just by ovs-bugtool but also by xen-bugtool on > >>XenServer. Will xen-bugtool accept the plugins that have been > >>modified in this way, or reject them because they have unknown > >>attributes? > > > >[Arun] - Test using xen-bugtool with modified plugins works after adding > >"filters" attribute in xml files. > > The reason to add 'filters' attribute is to filter specific > >element relevant to 'ovs' from a capability in plugins. > > As an other alternative(which would not require any change in xml > >files), thought earlier was not to add 'filters' attribute in element and > >just fetch based on the capabilities like 'system-configuration'. But by > >adding it into list ovs_info_caps would fetch all elements in 'data' from > >'system-configuration'. This yields non relevant information in archive. > >So decided to add 'filters' attribute. Since it works with xen-bugtool, I'm happy with the plugin modifications. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
