On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> ---
> src/openvz/openvz_conf.c | 45 ++++++++++++++++++++++-----------------------
> src/openvz/openvz_driver.c | 39 +++++++++++++++------------------------
> 2 files changed, 37 insertions(+), 47 deletions(-)
>
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> @@ -607,10 +603,10 @@ int openvzLoadDomains(struct openvz_driver *driver) {
> goto cleanup;
> }
>
> - if (!(def->os.type = strdup("exe")))
> - goto no_memory;
> - if (!(def->os.init = strdup("/sbin/init")))
> - goto no_memory;
> + if (VIR_STRDUP(def->os.type, "exe") < 0)
> + goto cleanup;
> + if (VIR_STRDUP(def->os.init, "/sbin/init") < 0)
> + goto cleanup;
Could chain this in one 'if'.
> @@ -800,8 +796,7 @@ openvzReadConfigParam(const char *conf_file, const char *param, char **value)
> saveptr = NULL;
> if ((token = strtok_r(sf, "\"\t\n", &saveptr)) != NULL) {
> VIR_FREE(*value);
> - *value = strdup(token);
> - if (*value == NULL) {
> + if (VIR_STRDUP(*value, token) < 0) {
> err = 1;
silent->noisy, but leaves other error paths in this function silent (for
example, if fopen fails) - so there is no way the caller could be
reporting all errors correctly. This one could probably use another
followup patch to clean it all up to have this function report errors on
all paths, and audit callers to avoid duplicate reporting.
Furthermore, we are using 'int err' as a boolean, so while we clean
things up, we should fix that.
> +++ b/src/openvz/openvz_driver.c
> @@ -88,7 +88,7 @@ static void openvzDriverUnlock(struct openvz_driver *driver)
>
> struct openvz_driver ovz_driver;
>
> -static void cmdExecFree(const char *cmdExec[])
> +static void cmdExecFree(char *cmdExec[])
Do we even need this function, or can virStringFreeList do the same?
For that matter, can we use virCommand instead of hand-rolling our own
char*[] for command execution? But that can be a followup patch.
> @@ -772,12 +766,14 @@ openvzGenerateVethName(int veid, char *dev_name_ve)
> {
> char dev_name[32];
> int ifNo = 0;
> + char *ret;
>
> if (sscanf(dev_name_ve, "%*[^0-9]%d", &ifNo) != 1)
> return NULL;
> if (snprintf(dev_name, sizeof(dev_name), "veth%d.%d", veid, ifNo) < 7)
> return NULL;
> - return strdup(dev_name);
> + ignore_value(VIR_STRDUP(ret, dev_name));
silent->noisy, but only on some of the failure paths. Callers are
currently using virReportError(), which means you won't notice this on
your pass to clean up oom reporting; so add this to the list of things
that needs explicit followup.
> @@ -788,7 +784,7 @@ openvzGenerateContainerVethName(int veid)
>
> /* try to get line "^NETIF=..." from config */
> if (openvzReadVPSConfigParam(veid, "NETIF", &temp) <= 0) {
> - name = strdup("eth0");
> + ignore_value(VIR_STRDUP(name, "eth0"));
Wow. This function was previously reporting oom but callers were then
throwing it away to report internal error. Again, something we ought to
clean up.
> @@ -821,7 +814,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
> virBufferPtr configBuf)
> {
> int rc = 0, narg;
> - const char *prog[OPENVZ_MAX_ARG];
> + char *prog[OPENVZ_MAX_ARG];
> char macaddr[VIR_MAC_STRING_BUFLEN];
> virMacAddr host_mac;
> char host_macaddr[VIR_MAC_STRING_BUFLEN];
> @@ -830,9 +823,9 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
>
> #define ADD_ARG_LIT(thisarg) \
> do { \
> - if (narg >= OPENVZ_MAX_ARG) \
> + if (narg >= OPENVZ_MAX_ARG) \
> goto no_memory; \
> - if ((prog[narg++] = strdup(thisarg)) == NULL) \
> + if (VIR_STRDUP(prog[narg++], thisarg) < 0) \
> goto no_memory; \
> } while (0)
Again, why aren't we using virCommand?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list