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

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

 



On 02/04/13 12:35, Osier Yang wrote:
On 2013年02月04日 19:32, Peter Krempa wrote:
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.

I think "The function reports errors" can just be removed. As it
doesn't report any error on success.

Okay.


+ *
+ * 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

ACK with that.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]