[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: detect programming errors with option parsing



On 17.08.2013 00:14, Eric Blake wrote:
> Noticed while reviewing another patch that had an accidental
> mismatch due to refactoring.  An audit of the code showed that
> very few callers of vshCommandOpt were expecting a return of
> -2, indicating programmer error, and of those that DID check,
> they just propagated that status to yet another caller that
> did not check.  Fix this by making the code blatantly warn
> the programmer, rather than silently ignoring it and possibly
> doing the wrong thing downstream.
> 
> I know that we frown on assert()/abort() inside libvirtd
> (libraries should NEVER kill the program that linked them),
> but as virsh is an app rather than the library, and as this
> is not the first use of assert() in virsh, I think this
> approach is okay.
> 
> * tools/virsh.h (vshCommandOpt): Drop declaration.
> * tools/virsh.c (vshCommandOpt): Make static, and add a
> parameter.  Abort on programmer errors rather than making callers
> repeat that logic.
> (vshCommandOptInt, vshCommandOptUInt, vshCommandOptUL)
> (vshCommandOptString, vshCommandOptStringReq)
> (vshCommandOptLongLong, vshCommandOptULongLong)
> (vshCommandOptBool): Adjust callers.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
> 
> In response to my observation on Don's email:
> https://www.redhat.com/archives/libvir-list/2013-August/msg00769.html
> 
>  tools/virsh.c | 97 +++++++++++++++++++++--------------------------------------
>  tools/virsh.h |  5 +--
>  2 files changed, 35 insertions(+), 67 deletions(-)

ACK and safe for freeze.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]