[libvirt] [PATCH] openvzDomainSetNetwork: use virCommand
Currently, the openvzDomainSetNetwork function constructs an array of strings representing a command line for VZCTL binary. This is a overkill since our virCommand APIs can cover all the functionality. --- src/openvz/openvz_driver.c | 56 ++++++++++------------------------------------ 1 file changed, 12 insertions(+), 44 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 129e328..db41d72 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -88,15 +88,6 @@ static void openvzDriverUnlock(struct openvz_driver *driver) struct openvz_driver ovz_driver; -static void cmdExecFree(const char *cmdExec[]) -{ - int i=-1; - while (cmdExec[++i]) { - VIR_FREE(cmdExec[i]); - } -} - - static int openvzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -827,22 +818,13 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, virDomainNetDefPtr net, virBufferPtr configBuf) { - int rc = 0, narg; - const char *prog[OPENVZ_MAX_ARG]; + int rc = 0; char macaddr[VIR_MAC_STRING_BUFLEN]; virMacAddr host_mac; char host_macaddr[VIR_MAC_STRING_BUFLEN]; struct openvz_driver *driver = conn->privateData; char *opt = NULL; - -#define ADD_ARG_LIT(thisarg) \ - do { \ - if (narg >= OPENVZ_MAX_ARG) \ - goto no_memory; \ - if ((prog[narg++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - + virCommandPtr cmd = NULL; if (net == NULL) return 0; @@ -852,18 +834,9 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, return -1; } - for (narg = 0; narg < OPENVZ_MAX_ARG; narg++) - prog[narg] = NULL; - - narg = 0; - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - net->type == VIR_DOMAIN_NET_TYPE_ETHERNET) { - ADD_ARG_LIT(VZCTL); - ADD_ARG_LIT("--quiet"); - ADD_ARG_LIT("set"); - ADD_ARG_LIT(vpsid); - } + net->type == VIR_DOMAIN_NET_TYPE_ETHERNET) + cmd = virCommandNewArgList(VZCTL, "--quiet", "set", vpsid, NULL); virMacAddrFormat(&net->mac, macaddr); virDomainNetGenerateMAC(driver->xmlopt, &host_mac); @@ -875,9 +848,6 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, virBuffer buf = VIR_BUFFER_INITIALIZER; int veid = openvzGetVEID(vpsid); - /* --netif_add ifname[,mac,host_ifname,host_mac] */ - ADD_ARG_LIT("--netif_add") ; - /* if user doesn't specify guest interface name, * then we need to generate it */ if (net->data.ethernet.dev == NULL) { @@ -922,37 +892,35 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, if (!(opt = virBufferContentAndReset(&buf))) goto no_memory; - ADD_ARG_LIT(opt) ; + /* --netif_add ifname[,mac,host_ifname,host_mac] */ + virCommandAddArgList(cmd, "--netif_add", opt, NULL); VIR_FREE(opt); } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) { /* --ipadd ip */ - ADD_ARG_LIT("--ipadd") ; - ADD_ARG_LIT(net->data.ethernet.ipaddr) ; + virCommandAddArgList(cmd, "--ipadd", net->data.ethernet.ipaddr, NULL); } /* TODO: processing NAT and physical device */ - if (prog[0] != NULL) { - ADD_ARG_LIT("--save"); - if (virRun(prog, NULL) < 0) { + if (cmd) { + virCommandAddArg(cmd, "--save"); + if (virCommandRun(cmd, NULL) < 0) { rc = -1; goto exit; } } exit: - cmdExecFree(prog); + virCommandFree(cmd); return rc; no_memory: VIR_FREE(opt); virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not put argument to %s"), VZCTL); - cmdExecFree(prog); + virCommandFree(cmd); return -1; - -#undef ADD_ARG_LIT } -- 1.8.2.1
On 05/19/2013 11:52 AM, Michal Privoznik wrote:
Currently, the openvzDomainSetNetwork function constructs an array of strings representing a command line for VZCTL binary. This is a overkill since our virCommand APIs can cover all the functionality. --- src/openvz/openvz_driver.c | 56 ++++++++++------------------------------------ 1 file changed, 12 insertions(+), 44 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 129e328..db41d72 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c
snip snip snip
@@ -922,37 +892,35 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, if (!(opt = virBufferContentAndReset(&buf))) goto no_memory;
- ADD_ARG_LIT(opt) ; + /* --netif_add ifname[,mac,host_ifname,host_mac] */ + virCommandAddArgList(cmd, "--netif_add", opt, NULL); VIR_FREE(opt); } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->data.ethernet.ipaddr != NULL) { /* --ipadd ip */ - ADD_ARG_LIT("--ipadd") ; - ADD_ARG_LIT(net->data.ethernet.ipaddr) ; + virCommandAddArgList(cmd, "--ipadd", net->data.ethernet.ipaddr, NULL); }
/* TODO: processing NAT and physical device */
- if (prog[0] != NULL) { - ADD_ARG_LIT("--save"); - if (virRun(prog, NULL) < 0) { + if (cmd) {
cmd may be NULL here on OOM too.
+ virCommandAddArg(cmd, "--save"); + if (virCommandRun(cmd, NULL) < 0) { rc = -1; goto exit;
Redundant goto (unless you initialize rc to -1 and use it to skip over "rc = 0").
} }
exit: - cmdExecFree(prog); + virCommandFree(cmd); return rc;
no_memory: VIR_FREE(opt);
opt is NULL here.
virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not put argument to %s"), VZCTL);
I think a standard "out of memory" message would be more fitting here. You could also rename 'exit' to 'cleanup' and get rid of the 'no_memory' label completely. Jan
On 05/20/2013 02:46 AM, Ján Tomko wrote:
- if (prog[0] != NULL) { - ADD_ARG_LIT("--save"); - if (virRun(prog, NULL) < 0) { + if (cmd) {
cmd may be NULL here on OOM too.
That, and virCommandRun is designed to report OOM on your behalf if cmd is NULL, so that your client code shouldn't have to worry about whether virCommandNew... even succeeded. Code that does 'if (cmd)' is generally suspect as doing too much instead of letting virCommand do its job. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Ján Tomko -
Michal Privoznik