Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 21 Aug 2019 23:16:30 -0400
From:   Branden Bonaby <brandonbonaby94@gmail.com>
To:     Harry Zhang <harz@microsoft.com>
Cc:     KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        "sashal@kernel.org" <sashal@kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/3] tools: hv: add vmbus testing tool
Message-ID: <20190822031630.GA37262@Test-Virtual-Machine>
References: <cover.1566340843.git.brandonbonaby94@gmail.com>
 <c63cae8e916cbfa4a3fe627da3a545736d0b45dc.1566340843.git.brandonbonaby94@gmail.com>
 <PS1P15301MB02497E19A17D71913DA32857C0A50@PS1P15301MB0249.APCP153.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PS1P15301MB02497E19A17D71913DA32857C0A50@PS1P15301MB0249.APCP153.PROD.OUTLOOK.COM>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 22, 2019 at 01:36:09AM +0000, Harry Zhang wrote:
> Tool function issues:  Please validate args errors for  '-p' and '--path',  in or following validate_args_path().  
> 
> Comments of functionality:
> -	it's confusing when fuzz_testing are all OFF, then user run ' python3 /home/lisa/vmbus_testing -p /sys/kernel/debug/hyperv/000d3a6e-4548-000d-3a6e-4548000d3a6e delay -d 0 0 -D ' which will enable all delay testing state ('Y' in state files).  even I used "-D", "--dis_all" param. 
> -	if we have subparsers of "disable-all" for the testing tool, then probably we don't need the mutually_exclusive_group under subparsers of "delay"
> -	the path argument (-p) could be an argument for subparsers of "delay" and "view" only.
> 
> Regards,
> Harry
>

So I made the choice to keep disabling the state and disabling delay
testing seperate, because once we start adding other testing options
you wouldn't want to inadvertently disable all testing especially
if you were doing more than one type of test at a time.
So with your configuration

'python3 /home/lisa/vmbus_testing -p /sys/kernel/debug/hyperv/000d3a6e-4548-000d-3a6e-4548000d3a6e delay -d 0 0 -D '

this would stop all delay testing on all the devices but wouldn't change
their test state to OFF 'N'.So thats why I have the option -s --state to
change the state to Off with a -s 0. Then to disable all types of testing
and change the state to OFF thats where the 'disable-all' subparser  comes in.
with:

'python3 /home/lisa/vmbus_testing disable-all

For that last point I don't understand what you mean, are you saying it would be
better to have something like this using  delay as an example?

'python3 /home/lisa/vmbus_testing delay -p /sys/kernel/debug/hyperv/000d3a6e-4548-000d-3a6e-4548000d3a6e'

If thats what you mean I figured it was better to make the -p accessible
to all test type so I made it apart of the main parser. This would allow
us to just have it there once instead of having to make a -p for every
subparser.

Also maybe I need to change the examples and the help text
because with the -D option for delay you wouldnt actually need to put in 
the path. As

'python3 /home/lisa/vmbus_testing delay -d 0 0 -D '

would suffice to stop delay testing on all devices; -E would enable
it for all devices and change the state to On 'Y' if it wasn't already.

let me know your thoughts

branden bonaby