Re: [libvirt] [PATCH 04/17] virsh: Add helper to request string arguments with error reporting

On 02/04/13 12:28, Osier Yang wrote: > On 2013年02月04日 18:47, Peter Krempa wrote: >> On 01/31/13 06:18, Osier Yang wrote: >>> On 2013年01月22日 02:07, Peter Krempa wrote: >>>> This patch adds a helper function with similar semantics to >>>> vshCommandOptString that requests a string argument, but does some >>>> error >>>> reporting without the need to do it in the functions themselves. >>>> >>>> The error reporting also provides information about the parameter >>>> whose >>>> retrieval failed. >>>> --- >>>> tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> tools/virsh.h | 4 ++++ >>>> 2 files changed, 55 insertions(+) >>>> >>>> diff --git a/tools/virsh.c b/tools/virsh.c >>>> index 908c6a1..1a3cab0 100644 >>>> --- a/tools/virsh.c >>>> +++ b/tools/virsh.c >>>> @@ -1417,6 +1417,57 @@ vshCommandOptString(const vshCmd *cmd, const >>>> char *name, const char **value) >>>> } >>>> >>>> /** >>>> + * vshCommandOptStringReq: Get a required string argumment >>> >>> Trivial, but we usually describe what the function does at [1]. >>> >>>> + * @ctl virsh control structure >>> >>> And have a ":" after @foo. >> >> Not in virsh.c. All surrounding functions don't have the colon in the >> comment. This is worth cleaning up separately instead of doing it in >> multiple ways in a single file. > > Agreed, a follow up patch will be good then. But others still stands. I changed the comment to: /** + * vshCommandOptStringReq: + * @ctl virsh control structure + * @cmd command structure + * @name option name + * @value result (updated to NULL or the option argument) + * + * Gets a option argument as string. This function reports errors.

+ * + * Returns 0 on success or when the option is not present and not + * required, *value is set to the option argument. On error -1 is + * returned and error message printed. + */ Peter