Jim Fehlig wrote:

David Kiarie wrote:

...

From: Kiarie Kahurani <davidkiarie4@gmail.com>

I introduced the function xenFormatXMVif(virConfPtr conf, ......); which parses network configuration

signed-off-by: David Kiarie <davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 298 ++++++++++++++++++++++++++++------------------------- 1 file changed, 155 insertions(+), 143 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index dc840e5..26ebd5a 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -309,6 +309,159 @@ static int xenParseXMMem(virConfPtr conf, virDomainDefPtr def)

return 0; } +static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def)

Same comment here about blank lines between functions. I won't bore you with repeating myself in all the patches. Also, remember Eric's comment about function return type on one line and name and params on another. E.g.

static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def) { ... }

...

+{ + char *script = NULL; + virDomainNetDefPtr net = NULL; + virConfValuePtr list = virConfGetValue(conf, "vif");

Style is to have a blank line after local variable declarations.

I think you should also define a local variable ret, initialized to -1.

...

+ if (list && list->type == VIR_CONF_LIST) { + list = list->list; + while (list) { + char model[10]; + char type[10]; + char ip[16]; + char mac[18]; + char bridge[50]; + char vifname[50]; + char *key; + + bridge[0] = '\0'; + mac[0] = '\0'; + ip[0] = '\0'; + model[0] = '\0'; + type[0] = '\0'; + vifname[0] = '\0'; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skipnic; + + key = list->str; + while (key) { + char *data; + char *nextkey = strchr(key, ','); + + if (!(data = strchr(key, '='))) + goto skipnic; + data++; + + if (STRPREFIX(key, "mac=")) { + int len = nextkey ? (nextkey - data) : sizeof(mac) - 1; + if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("MAC address %s too big for destination"), + data); + goto skipnic; + } + } else if (STRPREFIX(key, "bridge=")) { + int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1; + if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bridge %s too big for destination"), + data); + goto skipnic; + } + } else if (STRPREFIX(key, "script=")) { + int len = nextkey ? (nextkey - data) : strlen(data); + VIR_FREE(script); + if (VIR_STRNDUP(script, data, len) < 0) + goto cleanup; + } else if (STRPREFIX(key, "model=")) { + int len = nextkey ? (nextkey - data) : sizeof(model) - 1; + if (virStrncpy(model, data, len, sizeof(model)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Model %s too big for destination"), data); + goto skipnic; + } + } else if (STRPREFIX(key, "type=")) { + int len = nextkey ? (nextkey - data) : sizeof(type) - 1; + if (virStrncpy(type, data, len, sizeof(type)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Type %s too big for destination"), data); + goto skipnic; + } + } else if (STRPREFIX(key, "vifname=")) { + int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1; + if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Vifname %s too big for destination"), + data); + goto skipnic; + } + } else if (STRPREFIX(key, "ip=")) { + int len = nextkey ? (nextkey - data) : sizeof(ip) - 1; + if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("IP %s too big for destination"), data); + goto skipnic; + } + } + + while (nextkey && (nextkey[0] == ',' || + nextkey[0] == ' ' || + nextkey[0] == '\t')) + nextkey++; + key = nextkey; + } + + if (VIR_ALLOC(net) < 0) + goto cleanup; + + if (mac[0]) { + if (virMacAddrParse(mac, &net->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; + } + } + + if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + STREQ_NULLABLE(script, "vif-vnic")) { + net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; + } else { + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + } + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0) + goto cleanup; + if (ip[0] && VIR_STRDUP(net->data.bridge.ipaddr, ip) < 0) + goto cleanup; + } else { + if (ip[0] && VIR_STRDUP(net->data.ethernet.ipaddr, ip) < 0) + goto cleanup; + } + + if (script && script[0] && + VIR_STRDUP(net->script, script) < 0) + goto cleanup; + + if (model[0] && + VIR_STRDUP(net->model, model) < 0) + goto cleanup; + + if (!model[0] && type[0] && STREQ(type, "netfront") && + VIR_STRDUP(net->model, "netfront") < 0) + goto cleanup; + + if (vifname[0] && + VIR_STRDUP(net->ifname, vifname) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) + goto cleanup; + + VIR_FREE(script);

Can be freed in cleanup.

...

+ skipnic: + list = list->next; + virDomainNetDefFree(net);

Set 'net = NULL' now that it has been freed.

Ignore this comment. It is similar to the comment I made in 7/17, which Eric clarified https://www.redhat.com/archives/libvir-list/2014-July/msg00628.html Regards, Jim

The use of virReportError, On Wed, Jul 16, 2014 at 4:33 PM, David kiarie <davidkiarie4@gmail.com> wrote:

I think you comments on PCI parsing code could also apply here

On Wed, Jul 16, 2014 at 12:19 AM, Jim Fehlig <jfehlig@suse.com> wrote:

...

Jim Fehlig wrote:

...

David Kiarie wrote:

...

From: Kiarie Kahurani <davidkiarie4@gmail.com>

I introduced the function xenFormatXMVif(virConfPtr conf, ......); which parses network configuration

signed-off-by: David Kiarie <davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 298 ++++++++++++++++++++++++++++------------------------- 1 file changed, 155 insertions(+), 143 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index dc840e5..26ebd5a 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -309,6 +309,159 @@ static int xenParseXMMem(virConfPtr conf, virDomainDefPtr def)

return 0; } +static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def)

Same comment here about blank lines between functions. I won't bore you with repeating myself in all the patches. Also, remember Eric's comment about function return type on one line and name and params on another. E.g.

static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def) { ... }

...

+{ + char *script = NULL; + virDomainNetDefPtr net = NULL; + virConfValuePtr list = virConfGetValue(conf, "vif");

Style is to have a blank line after local variable declarations.

I think you should also define a local variable ret, initialized to -1.

...

+ if (list && list->type == VIR_CONF_LIST) { + list = list->list; + while (list) { + char model[10]; + char type[10]; + char ip[16]; + char mac[18]; + char bridge[50]; + char vifname[50]; + char *key; + + bridge[0] = '\0'; + mac[0] = '\0'; + ip[0] = '\0'; + model[0] = '\0'; + type[0] = '\0'; + vifname[0] = '\0'; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skipnic; + + key = list->str; + while (key) { + char *data; + char *nextkey = strchr(key, ','); + + if (!(data = strchr(key, '='))) + goto skipnic; + data++; + + if (STRPREFIX(key, "mac=")) { + int len = nextkey ? (nextkey - data) : sizeof(mac) - 1; + if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("MAC address %s too big for destination"), + data); + goto skipnic; here + } + } else if (STRPREFIX(key, "bridge=")) { + int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1; + if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bridge %s too big for destination"), + data); + goto skipnic; here + } + } else if (STRPREFIX(key, "script=")) { + int len = nextkey ? (nextkey - data) : strlen(data); + VIR_FREE(script); + if (VIR_STRNDUP(script, data, len) < 0) + goto cleanup; + } else if (STRPREFIX(key, "model=")) { + int len = nextkey ? (nextkey - data) : sizeof(model) - 1; + if (virStrncpy(model, data, len, sizeof(model)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Model %s too big for destination"), data); + goto skipnic; here + } + } else if (STRPREFIX(key, "type=")) { + int len = nextkey ? (nextkey - data) : sizeof(type) - 1; + if (virStrncpy(type, data, len, sizeof(type)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Type %s too big for destination"), data); + goto skipnic; here + } + } else if (STRPREFIX(key, "vifname=")) { + int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1; + if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Vifname %s too big for destination"), + data); + goto skipnic; here + } + } else if (STRPREFIX(key, "ip=")) { + int len = nextkey ? (nextkey - data) : sizeof(ip) - 1; + if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("IP %s too big for destination"), data); + goto skipnic; here + } + } + + while (nextkey && (nextkey[0] == ',' || + nextkey[0] == ' ' || + nextkey[0] == '\t')) + nextkey++; + key = nextkey; + } + + if (VIR_ALLOC(net) < 0) + goto cleanup; + + if (mac[0]) { + if (virMacAddrParse(mac, &net->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; + } + } + + if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + STREQ_NULLABLE(script, "vif-vnic")) { + net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; + } else { + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + } + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0) + goto cleanup; + if (ip[0] && VIR_STRDUP(net->data.bridge.ipaddr, ip) < 0) + goto cleanup; + } else { + if (ip[0] && VIR_STRDUP(net->data.ethernet.ipaddr, ip) < 0) + goto cleanup; + } + + if (script && script[0] && + VIR_STRDUP(net->script, script) < 0) + goto cleanup; + + if (model[0] && + VIR_STRDUP(net->model, model) < 0) + goto cleanup; + + if (!model[0] && type[0] && STREQ(type, "netfront") && + VIR_STRDUP(net->model, "netfront") < 0) + goto cleanup; + + if (vifname[0] && + VIR_STRDUP(net->ifname, vifname) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) + goto cleanup; + + VIR_FREE(script);

Can be freed in cleanup.

...

+ skipnic: + list = list->next; + virDomainNetDefFree(net);

Set 'net = NULL' now that it has been freed.

Ignore this comment. It is similar to the comment I made in 7/17, which Eric clarified

https://www.redhat.com/archives/libvir-list/2014-July/msg00628.html

Regards, Jim

Should I got ahead and make the changes?

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce the functions xenParseXMTimeOffset(.......); xenParseXMEventActions(......);

These aren't related and should be split into two patches IMO. Regards, Jim

These parse the time config and Event actions configs respectively

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 128 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 72 insertions(+), 56 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 26ebd5a..e30ab9c 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -462,6 +462,73 @@ cleanup: VIR_FREE(script); return -1; } +static int xenParseXMTimeOffset(virConfPtr conf, virDomainDefPtr def, + int xendConfigVersion) +{ + int vmlocaltime; + + if (xenXMConfigGetBool(conf, "localtime", &vmlocaltime, 0) < 0) + return -1; + + if (STREQ(def->os.type, "hvm")) { + /* only managed HVM domains since 3.1.0 have persistent rtc_timeoffset */ + if (xendConfigVersion < XEND_CONFIG_VERSION_3_1_0) { + if (vmlocaltime) + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME; + else + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->clock.data.utc_reset = true; + } else { + unsigned long rtc_timeoffset; + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_VARIABLE; + if (xenXMConfigGetULong(conf, "rtc_timeoffset", &rtc_timeoffset, 0) < 0) + return -1; + def->clock.data.variable.adjustment = (int)rtc_timeoffset; + def->clock.data.variable.basis = vmlocaltime ? + VIR_DOMAIN_CLOCK_BASIS_LOCALTIME : + VIR_DOMAIN_CLOCK_BASIS_UTC; + } + } else { + /* PV domains do not have an emulated RTC and the offset is fixed. */ + def->clock.offset = vmlocaltime ? + VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME : + VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->clock.data.utc_reset = true; + } /* !hvm */ + + return 0; +} +static int xenParseXMEventsActions(virConfPtr conf, virDomainDefPtr def) +{ + const char *str; + + if (xenXMConfigGetString(conf, "on_poweroff", &str, "destroy") < 0) + return -1; + if ((def->onPoweroff = virDomainLifecycleTypeFromString(str)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected value %s for on_poweroff"), str); + return -1; + } + + if (xenXMConfigGetString(conf, "on_reboot", &str, "restart") < 0) + return -1; + if ((def->onReboot = virDomainLifecycleTypeFromString(str)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected value %s for on_reboot"), str); + return -1; + } + + if (xenXMConfigGetString(conf, "on_crash", &str, "restart") < 0) + return -1; + if ((def->onCrash = virDomainLifecycleCrashTypeFromString(str)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected value %s for on_crash"), str); + return -1; + } + + return 0; + +} virDomainDefPtr xenParseXM(virConfPtr conf, int xendConfigVersion, virCapsPtr caps) @@ -476,7 +543,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, virDomainGraphicsDefPtr graphics = NULL; virDomainHostdevDefPtr hostdev = NULL; size_t i; - int vmlocaltime = 0; unsigned long count; char *script = NULL; char *listenAddr = NULL; @@ -556,32 +622,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (str && (virBitmapParse(str, 0, &def->cpumask, 4096) < 0)) goto cleanup;

- if (xenXMConfigGetString(conf, "on_poweroff", &str, "destroy") < 0) - goto cleanup; - if ((def->onPoweroff = virDomainLifecycleTypeFromString(str)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected value %s for on_poweroff"), str); - goto cleanup; - } - - if (xenXMConfigGetString(conf, "on_reboot", &str, "restart") < 0) - goto cleanup; - if ((def->onReboot = virDomainLifecycleTypeFromString(str)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected value %s for on_reboot"), str); - goto cleanup; - } - - if (xenXMConfigGetString(conf, "on_crash", &str, "restart") < 0) - goto cleanup; - if ((def->onCrash = virDomainLifecycleCrashTypeFromString(str)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected value %s for on_crash"), str); - goto cleanup; - } - - - if (hvm) { if (xenXMConfigGetBool(conf, "pae", &val, 0) < 0) goto cleanup; @@ -621,35 +661,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, def->clock.timers[0] = timer; } } - if (xenXMConfigGetBool(conf, "localtime", &vmlocaltime, 0) < 0) - goto cleanup; - - if (hvm) { - /* only managed HVM domains since 3.1.0 have persistent rtc_timeoffset */ - if (xendConfigVersion < XEND_CONFIG_VERSION_3_1_0) { - if (vmlocaltime) - def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME; - else - def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; - def->clock.data.utc_reset = true; - } else { - unsigned long rtc_timeoffset; - def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_VARIABLE; - if (xenXMConfigGetULong(conf, "rtc_timeoffset", &rtc_timeoffset, 0) < 0) - goto cleanup; - def->clock.data.variable.adjustment = (int)rtc_timeoffset; - def->clock.data.variable.basis = vmlocaltime ? - VIR_DOMAIN_CLOCK_BASIS_LOCALTIME : - VIR_DOMAIN_CLOCK_BASIS_UTC; - } - } else { - /* PV domains do not have an emulated RTC and the offset is fixed. */ - def->clock.offset = vmlocaltime ? - VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME : - VIR_DOMAIN_CLOCK_OFFSET_UTC; - def->clock.data.utc_reset = true; - } /* !hvm */ - if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) goto cleanup;

@@ -833,6 +844,11 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (xenParseXMVif(conf, def) < 0) goto cleanup;

+ if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMEventsActions(conf, def) < 0) + goto cleanup; + list = virConfGetValue(conf, "pci"); if (list && list->type == VIR_CONF_LIST) { list = list->list;

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce the function xenParseXMPCI(.........); which parses PCI config

signed-off-by:David Kiarie <davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 192 +++++++++++++++++++++++++++-------------------------- 1 file changed, 99 insertions(+), 93 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index e30ab9c..7a5b47c 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -529,6 +529,102 @@ static int xenParseXMEventsActions(virConfPtr conf, virDomainDefPtr def) return 0;

} +static int +xenParseXMPCI(virConfPtr conf, virDomainDefPtr def) +{ + virConfValuePtr list = virConfGetValue(conf, "pci"); + virDomainHostdevDefPtr hostdev = NULL;

Blank line after local variables.

+ if (list && list->type == VIR_CONF_LIST) { + list = list->list; + while (list) { + char domain[5]; + char bus[3]; + char slot[3]; + char func[2]; + char *key, *nextkey; + int domainID; + int busID; + int slotID; + int funcID; + + domain[0] = bus[0] = slot[0] = func[0] = '\0'; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skippci; + + /* pci=['0000:00:1b.0','0000:00:13.0'] */ + if (!(key = list->str)) + goto skippci; + if (!(nextkey = strchr(key, ':'))) + goto skippci; + + if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Domain %s too big for destination"), key);

Pre-existing, but while we are touching the code, I wonder if the use of virReportError here is correct? The device is skipped if there is a problem parsing it. I think these errors should be logged via VIR_WARN, but would like confirmation from another libvirt dev before asking you to change them. At the very least, the error should be changed to VIR_ERR_CONF_SYNTAX. Regards, Jim

+ goto skippci; + } + + key = nextkey + 1; + if (!(nextkey = strchr(key, ':'))) + goto skippci; + + if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bus %s too big for destination"), key); + goto skippci; + } + + key = nextkey + 1; + if (!(nextkey = strchr(key, '.'))) + goto skippci; + + if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Slot %s too big for destination"), key); + goto skippci; + } + + key = nextkey + 1; + if (strlen(key) != 1) + goto skippci; + + if (virStrncpy(func, key, 1, sizeof(func)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Function %s too big for destination"), key); + goto skippci; + } + + if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) + goto skippci; + if (virStrToLong_i(bus, NULL, 16, &busID) < 0) + goto skippci; + if (virStrToLong_i(slot, NULL, 16, &slotID) < 0) + goto skippci; + if (virStrToLong_i(func, NULL, 16, &funcID) < 0) + goto skippci; + + if (!(hostdev = virDomainHostdevDefAlloc())) + return -1; + + hostdev->managed = false; + hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; + hostdev->source.subsys.u.pci.addr.domain = domainID; + hostdev->source.subsys.u.pci.addr.bus = busID; + hostdev->source.subsys.u.pci.addr.slot = slotID; + hostdev->source.subsys.u.pci.addr.function = funcID; + + if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { + virDomainHostdevDefFree(hostdev); + return -1; + } + + skippci: + list = list->next; + } + } + + return 0; +} virDomainDefPtr xenParseXM(virConfPtr conf, int xendConfigVersion, virCapsPtr caps) @@ -541,7 +637,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, virDomainDiskDefPtr disk = NULL; virDomainNetDefPtr net = NULL; virDomainGraphicsDefPtr graphics = NULL; - virDomainHostdevDefPtr hostdev = NULL; size_t i; unsigned long count; char *script = NULL; @@ -848,98 +943,9 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto cleanup; if (xenParseXMEventsActions(conf, def) < 0) goto cleanup; - - list = virConfGetValue(conf, "pci"); - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - char domain[5]; - char bus[3]; - char slot[3]; - char func[2]; - char *key, *nextkey; - int domainID; - int busID; - int slotID; - int funcID; - - domain[0] = bus[0] = slot[0] = func[0] = '\0'; - - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skippci; - - /* pci=['0000:00:1b.0','0000:00:13.0'] */ - if (!(key = list->str)) - goto skippci; - if (!(nextkey = strchr(key, ':'))) - goto skippci; - - if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain %s too big for destination"), key); - goto skippci; - } - - key = nextkey + 1; - if (!(nextkey = strchr(key, ':'))) - goto skippci; - - if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bus %s too big for destination"), key); - goto skippci; - } - - key = nextkey + 1; - if (!(nextkey = strchr(key, '.'))) - goto skippci; - - if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Slot %s too big for destination"), key); - goto skippci; - } - - key = nextkey + 1; - if (strlen(key) != 1) - goto skippci; - - if (virStrncpy(func, key, 1, sizeof(func)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Function %s too big for destination"), key); - goto skippci; - } - - if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) - goto skippci; - if (virStrToLong_i(bus, NULL, 16, &busID) < 0) - goto skippci; - if (virStrToLong_i(slot, NULL, 16, &slotID) < 0) - goto skippci; - if (virStrToLong_i(func, NULL, 16, &funcID) < 0) - goto skippci; - - if (!(hostdev = virDomainHostdevDefAlloc())) - goto cleanup; - - hostdev->managed = false; - hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; - hostdev->source.subsys.u.pci.addr.domain = domainID; - hostdev->source.subsys.u.pci.addr.bus = busID; - hostdev->source.subsys.u.pci.addr.slot = slotID; - hostdev->source.subsys.u.pci.addr.function = funcID; - - if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { - virDomainHostdevDefFree(hostdev); - goto cleanup; - } - - skippci: - list = list->next; - } - } - - if (hvm) { + if (xenParseXMPCI(conf, def) < 0) + goto cleanup; + if (hvm) { if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) goto cleanup; if (str &&

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce xenParseXMCPUFeatures(.....); This function parses config related to CPU and power management

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 116 ++++++++++++++++++++++++++++------------------------- 1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 7a5b47c..ebeeeb5 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -625,6 +625,66 @@ xenParseXMPCI(virConfPtr conf, virDomainDefPtr def)

return 0; } +static int xenParseXMCPUFeatures(virConfPtr conf, virDomainDefPtr def) +{ + unsigned long count; + const char *str; + int val;

The usual whitespace comments, but otherwise looks good. Regards, Jim

+ if (xenXMConfigGetULong(conf, "vcpus", &count, 1) < 0 || + MAX_VIRT_CPUS < count) + return -1; + def->maxvcpus = count; + if (xenXMConfigGetULong(conf, "vcpu_avail", &count, -1) < 0) + return -1; + def->vcpus = MIN(count_one_bits_l(count), def->maxvcpus); + + if (xenXMConfigGetString(conf, "cpus", &str, NULL) < 0) + return -1; + if (str && (virBitmapParse(str, 0, &def->cpumask, 4096) < 0)) + return -1; + + if (STREQ(def->os.type, "hvm")) { + if (xenXMConfigGetBool(conf, "pae", &val, 0) < 0) + return -1; + else if (val) + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; + if (xenXMConfigGetBool(conf, "acpi", &val, 0) < 0) + return -1; + else if (val) + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; + if (xenXMConfigGetBool(conf, "apic", &val, 0) < 0) + return -1; + else if (val) + def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; + if (xenXMConfigGetBool(conf, "hap", &val, 0) < 0) + return -1; + else if (val) + def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_DOMAIN_FEATURE_STATE_ON; + if (xenXMConfigGetBool(conf, "viridian", &val, 0) < 0) + return -1; + else if (val) + def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_DOMAIN_FEATURE_STATE_ON; + + if (xenXMConfigGetBool(conf, "hpet", &val, -1) < 0) + return -1; + else if (val != -1) { + virDomainTimerDefPtr timer; + + if (VIR_ALLOC_N(def->clock.timers, 1) < 0 || + VIR_ALLOC(timer) < 0) + return -1; + + timer->name = VIR_DOMAIN_TIMER_NAME_HPET; + timer->present = val; + timer->tickpolicy = -1; + + def->clock.ntimers = 1; + def->clock.timers[0] = timer; + } + } + + return 0; +} virDomainDefPtr xenParseXM(virConfPtr conf, int xendConfigVersion, virCapsPtr caps) @@ -638,7 +698,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, virDomainNetDefPtr net = NULL; virDomainGraphicsDefPtr graphics = NULL; size_t i; - unsigned long count; char *script = NULL; char *listenAddr = NULL;

@@ -703,59 +762,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, } if (xenParseXMMem(conf, def) < 0) goto cleanup; - - if (xenXMConfigGetULong(conf, "vcpus", &count, 1) < 0 || - MAX_VIRT_CPUS < count) - goto cleanup; - def->maxvcpus = count; - if (xenXMConfigGetULong(conf, "vcpu_avail", &count, -1) < 0) - goto cleanup; - def->vcpus = MIN(count_one_bits_l(count), def->maxvcpus); - - if (xenXMConfigGetString(conf, "cpus", &str, NULL) < 0) - goto cleanup; - if (str && (virBitmapParse(str, 0, &def->cpumask, 4096) < 0)) - goto cleanup; - - if (hvm) { - if (xenXMConfigGetBool(conf, "pae", &val, 0) < 0) - goto cleanup; - else if (val) - def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; - if (xenXMConfigGetBool(conf, "acpi", &val, 0) < 0) - goto cleanup; - else if (val) - def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; - if (xenXMConfigGetBool(conf, "apic", &val, 0) < 0) - goto cleanup; - else if (val) - def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; - if (xenXMConfigGetBool(conf, "hap", &val, 0) < 0) - goto cleanup; - else if (val) - def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_DOMAIN_FEATURE_STATE_ON; - if (xenXMConfigGetBool(conf, "viridian", &val, 0) < 0) - goto cleanup; - else if (val) - def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_DOMAIN_FEATURE_STATE_ON; - - if (xenXMConfigGetBool(conf, "hpet", &val, -1) < 0) - goto cleanup; - else if (val != -1) { - virDomainTimerDefPtr timer; - - if (VIR_ALLOC_N(def->clock.timers, 1) < 0 || - VIR_ALLOC(timer) < 0) - goto cleanup; - - timer->name = VIR_DOMAIN_TIMER_NAME_HPET; - timer->present = val; - timer->tickpolicy = -1; - - def->clock.ntimers = 1; - def->clock.timers[0] = timer; - } - } if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) goto cleanup;

@@ -945,6 +951,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto cleanup; if (xenParseXMPCI(conf, def) < 0) goto cleanup; + if (xenParseXMCPUFeatures(conf, def) < 0) + goto cleanup; if (hvm) { if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) goto cleanup;

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce the function xenParseXMDisk(..........);

Parses disk config

signed-off-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 204 ++++++++++++++++++++++++++++------------------------- 1 file changed, 108 insertions(+), 96 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index ebeeeb5..80a7941 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -685,87 +685,14 @@ static int xenParseXMCPUFeatures(virConfPtr conf, virDomainDefPtr def)

return 0; } -virDomainDefPtr -xenParseXM(virConfPtr conf, int xendConfigVersion, - virCapsPtr caps) +static int xenParseXMDisk(virConfPtr conf, virDomainDefPtr def, + int xendConfigVersion) { - const char *str; - int hvm = 0; - int val; - virConfValuePtr list; - virDomainDefPtr def = NULL; + const char *str = NULL; virDomainDiskDefPtr disk = NULL; - virDomainNetDefPtr net = NULL; - virDomainGraphicsDefPtr graphics = NULL; - size_t i; - char *script = NULL; - char *listenAddr = NULL; - - if (VIR_ALLOC(def) < 0) - return NULL; - - def->virtType = VIR_DOMAIN_VIRT_XEN; - def->id = -1; - if (xenParseXMGeneral(conf, def, caps) < 0) - goto cleanup; - hvm = (STREQ(def->os.type, "hvm")); - if (hvm) { - const char *boot; - if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0) - goto cleanup; - - if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0) - goto cleanup; + int hvm = STREQ(def->os.type, "hvm"); + virConfValuePtr list = virConfGetValue(conf, "disk");

- for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { - switch (*boot) { - case 'a': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY; - break; - case 'd': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM; - break; - case 'n': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET; - break; - case 'c': - default: - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK; - break; - } - def->os.nBootDevs++; - } - } else { - const char *extra, *root; - - if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) - goto cleanup; - - if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - goto cleanup; - if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) - goto cleanup; - if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) - goto cleanup; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - goto cleanup; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - goto cleanup; - } - } - if (xenParseXMMem(conf, def) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) - goto cleanup; - - list = virConfGetValue(conf, "disk"); if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { @@ -779,7 +706,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, head = list->str;

if (!(disk = virDomainDiskDefNew())) - goto cleanup; + return -1;

/* * Disks have 3 components, SOURCE,DEST-DEVICE,MODE @@ -798,10 +725,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, ignore_value(virDomainDiskSetSource(disk, NULL)); } else { if (VIR_STRNDUP(tmp, head, offset - head) < 0) - goto cleanup; + return -1; if (virDomainDiskSetSource(disk, tmp) < 0) { VIR_FREE(tmp); - goto cleanup; + return -1; } VIR_FREE(tmp); } @@ -815,12 +742,12 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (!(offset = strchr(head, ','))) goto skipdisk; if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) - goto cleanup; + return -1; if (virStrncpy(disk->dst, head, offset - head, (offset - head) + 1) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Dest file %s too big for destination"), head); - goto cleanup; + return -1; } head = offset + 1;

@@ -832,16 +759,16 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if ((tmp = strchr(src, ':')) != NULL) { len = tmp - src; if (VIR_STRNDUP(tmp, src, len) < 0) - goto cleanup; + return -1; if (virDomainDiskSetDriver(disk, tmp) < 0) { VIR_FREE(tmp); - goto cleanup; + return -1; } VIR_FREE(tmp);

/* Strip the prefix we found off the source file name */ if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto cleanup; + return -1; src = virDomainDiskGetSource(disk); }

@@ -854,7 +781,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, len = tmp - src;

if (VIR_STRNDUP(driverType, src, len) < 0) - goto cleanup; + return -1; if (STREQ(driverType, "aio")) virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); else @@ -865,12 +792,12 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown driver type %s"), src); - goto cleanup; + return -1; }

/* Strip the prefix we found off the source file name */ if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto cleanup; + return -1; src = virDomainDiskGetSource(disk); } } @@ -878,7 +805,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, /* No source, or driver name, so fix to phy: */ if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "phy") < 0) - goto cleanup; + return -1;

The problem will all these 'return -1' is that you leak 'disk'. I think it would be good to have cleanup label in this function. I'd suggested a local variable 'ret' initialized to -1, which you set to 0 when the function succeeds. You can then cleanup and return 'ret' in the cleanup label.

/* phy: type indicates a block device */ @@ -912,7 +839,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,

/* Maintain list in sorted order according to target device name */ if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) - goto cleanup; + return -1;

skipdisk: list = list->next;

The next line here is virDomainDiskDefFree(disk); which is not right in the case VIR_APPEND_ELEMENT succeeds (pre-existing bug). 'disk' should be set to NULL if VIR_APPEND_ELEMENT succeeds, so we don't free the disk we've just added to the disk list.

@@ -922,26 +849,108 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,

if (hvm && xendConfigVersion == XEND_CONFIG_VERSION_3_0_2) { if (xenXMConfigGetString(conf, "cdrom", &str, NULL) < 0) - goto cleanup; + return -1; if (str) { if (!(disk = virDomainDiskDefNew())) - goto cleanup; + return -1;

virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; if (virDomainDiskSetDriver(disk, "file") < 0) - goto cleanup; + return -1; if (virDomainDiskSetSource(disk, str) < 0) - goto cleanup; + return -1; if (VIR_STRDUP(disk->dst, "hdc") < 0) - goto cleanup; + return -1; disk->bus = VIR_DOMAIN_DISK_BUS_IDE; disk->src->readonly = true;

if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) + return -1; + } + } + + return 0; +} +virDomainDefPtr +xenParseXM(virConfPtr conf, int xendConfigVersion, + virCapsPtr caps) +{ + const char *str; + int hvm = 0; + int val; + virConfValuePtr list; + virDomainDefPtr def = NULL; + virDomainDiskDefPtr disk = NULL;

'disk' is no longer used in this function and can be removed. Regards, Jim

+ virDomainNetDefPtr net = NULL; + virDomainGraphicsDefPtr graphics = NULL; + size_t i; + char *script = NULL; + char *listenAddr = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->virtType = VIR_DOMAIN_VIRT_XEN; + def->id = -1; + if (xenParseXMGeneral(conf, def, caps) < 0) + goto cleanup; + hvm = (STREQ(def->os.type, "hvm")); + if (hvm) { + const char *boot; + if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0) + goto cleanup; + + if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0) + goto cleanup; + + for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { + switch (*boot) { + case 'a': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY; + break; + case 'd': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM; + break; + case 'n': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET; + break; + case 'c': + default: + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK; + break; + } + def->os.nBootDevs++; + } + } else { + const char *extra, *root; + + if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) + goto cleanup; + + if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) + goto cleanup; + if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) + goto cleanup; + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) + goto cleanup; + + if (root) { + if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) + goto cleanup; + } else { + if (VIR_STRDUP(def->os.cmdline, extra) < 0) goto cleanup; } } + if (xenParseXMMem(conf, def) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) + goto cleanup; if (xenParseXMVif(conf, def) < 0) goto cleanup;

@@ -953,6 +962,9 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto cleanup; if (xenParseXMCPUFeatures(conf, def) < 0) goto cleanup; + if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (hvm) { if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) goto cleanup;

Eric Blake wrote:

On 07/11/2014 01:04 PM, Jim Fehlig wrote:

...

David Kiarie wrote:

...

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce the function xenParseXMDisk(..........);

Parses disk config

signed-off-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 204 ++++++++++++++++++++++++++++------------------------- 1 file changed, 108 insertions(+), 96 deletions(-)

...

...

@@ -912,7 +839,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,

/* Maintain list in sorted order according to target device name */ if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) - goto cleanup; + return -1;

skipdisk: list = list->next;

The next line here is

virDomainDiskDefFree(disk);

which is not right in the case VIR_APPEND_ELEMENT succeeds (pre-existing bug). 'disk' should be set to NULL if VIR_APPEND_ELEMENT succeeds, so we don't free the disk we've just added to the disk list.

VIR_APPEND_ELEMENT(, , disk) sets disk=NULL on success, precisely so you don't have to worry about double-freeing it (if you call the macro, you are guaranteed either transfer semantics and success, or no change on failure).

Ah, should have read the source :). Thanks Eric. That explains my confusion as to why a crash was never reported in this code (or the vif parsing code). Regards, Jim

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

introduce function

xenParseXMVfb(.....);

which parses the graphics config

signed-off-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 247 ++++++++++++++++++++++++++++------------------------- 1 file changed, 131 insertions(+), 116 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 80a7941..89d5739 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -872,125 +872,14 @@ static int xenParseXMDisk(virConfPtr conf, virDomainDefPtr def,

return 0; } -virDomainDefPtr -xenParseXM(virConfPtr conf, int xendConfigVersion, - virCapsPtr caps) +static int xenParseXMVfb(virConfPtr conf, virDomainDefPtr def, + int xendConfigVersion) { - const char *str; - int hvm = 0; + char *listenAddr; int val; - virConfValuePtr list; - virDomainDefPtr def = NULL; - virDomainDiskDefPtr disk = NULL; - virDomainNetDefPtr net = NULL; + virConfValuePtr list = NULL; virDomainGraphicsDefPtr graphics = NULL; - size_t i; - char *script = NULL; - char *listenAddr = NULL; - - if (VIR_ALLOC(def) < 0) - return NULL; - - def->virtType = VIR_DOMAIN_VIRT_XEN; - def->id = -1; - if (xenParseXMGeneral(conf, def, caps) < 0) - goto cleanup; - hvm = (STREQ(def->os.type, "hvm")); - if (hvm) { - const char *boot; - if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0) - goto cleanup; - - if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0) - goto cleanup; - - for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { - switch (*boot) { - case 'a': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY; - break; - case 'd': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM; - break; - case 'n': - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET; - break; - case 'c': - default: - def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK; - break; - } - def->os.nBootDevs++; - } - } else { - const char *extra, *root; - - if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) - goto cleanup; - - if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - goto cleanup; - if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) - goto cleanup; - if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) - goto cleanup; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - goto cleanup; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - goto cleanup; - } - } - if (xenParseXMMem(conf, def) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) - goto cleanup; - if (xenParseXMVif(conf, def) < 0) - goto cleanup; - - if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) - goto cleanup; - if (xenParseXMEventsActions(conf, def) < 0) - goto cleanup; - if (xenParseXMPCI(conf, def) < 0) - goto cleanup; - if (xenParseXMCPUFeatures(conf, def) < 0) - goto cleanup; - if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) - goto cleanup; - - if (hvm) { - if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) - goto cleanup; - if (str && - (STREQ(str, "tablet") || - STREQ(str, "mouse") || - STREQ(str, "keyboard"))) { - virDomainInputDefPtr input; - if (VIR_ALLOC(input) < 0) - goto cleanup; - input->bus = VIR_DOMAIN_INPUT_BUS_USB; - if (STREQ(str, "mouse")) - input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; - else if (STREQ(str, "tablet")) - input->type = VIR_DOMAIN_INPUT_TYPE_TABLET; - else if (STREQ(str, "keyboard")) - input->type = VIR_DOMAIN_INPUT_TYPE_KBD; - if (VIR_ALLOC_N(def->inputs, 1) < 0) { - virDomainInputDefFree(input); - goto cleanup; - } - def->inputs[0] = input; - def->ninputs = 1; - } - } - + int hvm = STREQ(def->os.type, "hvm");

The usual whitespace comments: blank lines between functions, indentation of the function params, and blank line after local variables.

/* HVM guests, or old PV guests use this config format */ if (hvm || xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { if (xenXMConfigGetBool(conf, "vnc", &val, 0) < 0) @@ -1125,6 +1014,132 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, } }

+ return 0; + + cleanup: + virDomainGraphicsDefFree(graphics); + return -1; +} +virDomainDefPtr +xenParseXM(virConfPtr conf, int xendConfigVersion, + virCapsPtr caps) +{ + const char *str; + int hvm = 0; + virConfValuePtr list; + virDomainDefPtr def = NULL; + virDomainDiskDefPtr disk = NULL; + virDomainNetDefPtr net = NULL; + virDomainGraphicsDefPtr graphics = NULL; + size_t i; + char *script = NULL; + char *listenAddr = NULL;

'graphics' and 'listenAddr' are no longer used in this function and can be dropped. Regards, Jim

+ + if (VIR_ALLOC(def) < 0) + return NULL; + + def->virtType = VIR_DOMAIN_VIRT_XEN; + def->id = -1; + if (xenParseXMGeneral(conf, def, caps) < 0) + goto cleanup; + hvm = (STREQ(def->os.type, "hvm")); + if (hvm) { + const char *boot; + if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0) + goto cleanup; + + if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0) + goto cleanup; + + for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { + switch (*boot) { + case 'a': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY; + break; + case 'd': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM; + break; + case 'n': + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET; + break; + case 'c': + default: + def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK; + break; + } + def->os.nBootDevs++; + } + } else { + const char *extra, *root; + + if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) + goto cleanup; + + if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) + goto cleanup; + if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) + goto cleanup; + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) + goto cleanup; + + if (root) { + if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) + goto cleanup; + } else { + if (VIR_STRDUP(def->os.cmdline, extra) < 0) + goto cleanup; + } + } + if (xenParseXMMem(conf, def) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) + goto cleanup; + if (xenParseXMVif(conf, def) < 0) + goto cleanup; + + if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMEventsActions(conf, def) < 0) + goto cleanup; + if (xenParseXMPCI(conf, def) < 0) + goto cleanup; + if (xenParseXMCPUFeatures(conf, def) < 0) + goto cleanup; + if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) + goto cleanup; + + if (hvm) { + if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) + goto cleanup; + if (str && + (STREQ(str, "tablet") || + STREQ(str, "mouse") || + STREQ(str, "keyboard"))) { + virDomainInputDefPtr input; + if (VIR_ALLOC(input) < 0) + goto cleanup; + input->bus = VIR_DOMAIN_INPUT_BUS_USB; + if (STREQ(str, "mouse")) + input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; + else if (STREQ(str, "tablet")) + input->type = VIR_DOMAIN_INPUT_TYPE_TABLET; + else if (STREQ(str, "keyboard")) + input->type = VIR_DOMAIN_INPUT_TYPE_KBD; + if (VIR_ALLOC_N(def->inputs, 1) < 0) { + virDomainInputDefFree(input); + goto cleanup; + } + def->inputs[0] = input; + def->ninputs = 1; + } + } + if (xenParseXMVfb(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (hvm) { virDomainChrDefPtr chr = NULL;

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce functions xenParseXMOS(.....) and xenParseXMMisc(.....) to parse the OS and Miscellaneous devices configs

signed-off-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 146 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 82 insertions(+), 64 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 89d5739..312d890 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1020,36 +1020,21 @@ static int xenParseXMVfb(virConfPtr conf, virDomainDefPtr def, virDomainGraphicsDefFree(graphics); return -1; } -virDomainDefPtr -xenParseXM(virConfPtr conf, int xendConfigVersion, - virCapsPtr caps) +static int xenParseXMOS(virConfPtr conf, virDomainDefPtr def) { - const char *str; - int hvm = 0; - virConfValuePtr list; - virDomainDefPtr def = NULL; - virDomainDiskDefPtr disk = NULL; - virDomainNetDefPtr net = NULL; - virDomainGraphicsDefPtr graphics = NULL; size_t i; - char *script = NULL; - char *listenAddr = NULL; - - if (VIR_ALLOC(def) < 0) - return NULL;

- def->virtType = VIR_DOMAIN_VIRT_XEN; - def->id = -1; - if (xenParseXMGeneral(conf, def, caps) < 0) - goto cleanup; - hvm = (STREQ(def->os.type, "hvm")); - if (hvm) { + if (STREQ(def->os.type, "hvm")) { const char *boot; + + if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) + return -1; + if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0) - goto cleanup; + return -1;

if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0) - goto cleanup; + return -1;

for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { switch (*boot) { @@ -1073,55 +1058,52 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, const char *extra, *root;

if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) - goto cleanup; + return -1; if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) - goto cleanup; + return -1;

if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - goto cleanup; + return -1; if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - goto cleanup; + return -1; if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) - goto cleanup; + return -1; if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) - goto cleanup; + return -1;

if (root) { if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - goto cleanup; + return -1; } else { if (VIR_STRDUP(def->os.cmdline, extra) < 0) - goto cleanup; + return -1; } } - if (xenParseXMMem(conf, def) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) - goto cleanup; - if (xenParseXMVif(conf, def) < 0) - goto cleanup; - - if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) - goto cleanup; - if (xenParseXMEventsActions(conf, def) < 0) - goto cleanup; - if (xenParseXMPCI(conf, def) < 0) - goto cleanup; - if (xenParseXMCPUFeatures(conf, def) < 0) - goto cleanup; - if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) - goto cleanup; - - if (hvm) { + + return 0; +} +static int xenParseXMMisc(virConfPtr conf, virDomainDefPtr def)

Hrm, not sure I'm fond of this name since it could become a dumping ground for any "miscellaneous" stuff in the future. Perhaps xenParseXMInputDevs? Regards, Jim

+{ + const char *str; + + if (STREQ(def->os.type, "hvm")) { + if (xenXMConfigGetString(conf, "soundhw", &str, NULL) < 0) + return -1; + + if (str && + xenParseSxprSound(def, str) < 0) + return -1; + if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0) - goto cleanup; + return -1; + if (str && (STREQ(str, "tablet") || STREQ(str, "mouse") || STREQ(str, "keyboard"))) { virDomainInputDefPtr input; if (VIR_ALLOC(input) < 0) - goto cleanup; + return -1; input->bus = VIR_DOMAIN_INPUT_BUS_USB; if (STREQ(str, "mouse")) input->type = VIR_DOMAIN_INPUT_TYPE_MOUSE; @@ -1131,15 +1113,61 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, input->type = VIR_DOMAIN_INPUT_TYPE_KBD; if (VIR_ALLOC_N(def->inputs, 1) < 0) { virDomainInputDefFree(input); - goto cleanup; + return -1; } def->inputs[0] = input; def->ninputs = 1; } } - if (xenParseXMVfb(conf, def, xendConfigVersion) < 0) + + return 0; +} +virDomainDefPtr +xenParseXM(virConfPtr conf, int xendConfigVersion, + virCapsPtr caps) +{ + const char *str; + int hvm = 0; + virConfValuePtr list; + virDomainDefPtr def = NULL; + virDomainDiskDefPtr disk = NULL; + virDomainNetDefPtr net = NULL; + virDomainGraphicsDefPtr graphics = NULL; + char *script = NULL; + char *listenAddr = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->virtType = VIR_DOMAIN_VIRT_XEN; + def->id = -1; + if (xenParseXMGeneral(conf, def, caps) < 0) goto cleanup;

+ if (xenParseXMOS(conf, def) < 0) + goto cleanup; + + hvm = (STREQ(def->os.type, "hvm")); + if (xenParseXMMem(conf, def) < 0) + goto cleanup; + if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) + goto cleanup; + if (xenParseXMVif(conf, def) < 0) + goto cleanup; + if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMEventsActions(conf, def) < 0) + goto cleanup; + if (xenParseXMPCI(conf, def) < 0) + goto cleanup; + if (xenParseXMCPUFeatures(conf, def) < 0) + goto cleanup; + if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMVfb(conf, def, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMMisc(conf, def) < 0) + goto cleanup; if (hvm) { virDomainChrDefPtr chr = NULL;

@@ -1221,16 +1249,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, def->consoles[0]->target.port = 0; def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } - - if (hvm) { - if (xenXMConfigGetString(conf, "soundhw", &str, NULL) < 0) - goto cleanup; - - if (str && - xenParseSxprSound(def, str) < 0) - goto cleanup; - } - VIR_FREE(script); return def;

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

A couple of miscellaneous fixed and wrap code common code into xenParseConfigCommon(...).I will drop some of the functions later though

signed-off-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 134 ++++++++++++++++++++++++++++------------------------- src/xenxs/xen_xm.h | 3 +- 2 files changed, 73 insertions(+), 64 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 312d890..657dd1c 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -458,7 +458,7 @@ static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def) }

return 0; -cleanup: + cleanup:

This should be in 3/17 to fix 'make syntax-check'.

VIR_FREE(script); return -1; } @@ -1026,7 +1026,8 @@ static int xenParseXMOS(virConfPtr conf, virDomainDefPtr def)

if (STREQ(def->os.type, "hvm")) { const char *boot; - + if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) + return -1;

This replicates the next lines of code. Not needed IMO.

if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) return -1;

@@ -1122,54 +1123,13 @@ static int xenParseXMMisc(virConfPtr conf, virDomainDefPtr def)

return 0; } -virDomainDefPtr -xenParseXM(virConfPtr conf, int xendConfigVersion, - virCapsPtr caps) +static int xenParseXMCharDev(virConfPtr conf, virDomainDefPtr def) { const char *str; - int hvm = 0; - virConfValuePtr list; - virDomainDefPtr def = NULL; - virDomainDiskDefPtr disk = NULL; - virDomainNetDefPtr net = NULL; - virDomainGraphicsDefPtr graphics = NULL; - char *script = NULL; - char *listenAddr = NULL; - - if (VIR_ALLOC(def) < 0) - return NULL; - - def->virtType = VIR_DOMAIN_VIRT_XEN; - def->id = -1; - if (xenParseXMGeneral(conf, def, caps) < 0) - goto cleanup; - - if (xenParseXMOS(conf, def) < 0) - goto cleanup; + virConfValuePtr value = NULL; + virDomainChrDefPtr chr = NULL;

- hvm = (STREQ(def->os.type, "hvm")); - if (xenParseXMMem(conf, def) < 0) - goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0) - goto cleanup; - if (xenParseXMVif(conf, def) < 0) - goto cleanup; - if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) - goto cleanup; - if (xenParseXMEventsActions(conf, def) < 0) - goto cleanup; - if (xenParseXMPCI(conf, def) < 0) - goto cleanup; - if (xenParseXMCPUFeatures(conf, def) < 0) - goto cleanup; - if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) - goto cleanup; - if (xenParseXMVfb(conf, def, xendConfigVersion) < 0) - goto cleanup; - if (xenParseXMMisc(conf, def) < 0) - goto cleanup; - if (hvm) { - virDomainChrDefPtr chr = NULL; + if (STREQ(def->os.type, "hvm")) {

if (xenXMConfigGetString(conf, "parallel", &str, NULL) < 0) goto cleanup; @@ -1179,7 +1139,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,

if (chr) { if (VIR_ALLOC_N(def->parallels, 1) < 0) { - virDomainChrDefFree(chr); goto cleanup; } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; @@ -1190,21 +1149,21 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, }

/* Try to get the list of values to support multiple serial ports */ - list = virConfGetValue(conf, "serial"); - if (list && list->type == VIR_CONF_LIST) { + value = virConfGetValue(conf, "serial"); + if (value && value->type == VIR_CONF_LIST) { int portnum = -1;

- list = list->list; - while (list) { + value = value->list; + while (value) { char *port = NULL;

- if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + if ((value->type != VIR_CONF_STRING) || (value->str == NULL)) goto cleanup;

- port = list->str; + port = value->str; portnum++; if (STREQ(port, "none")) { - list = list->next; + value = value->next; continue; }

@@ -1215,11 +1174,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, chr->target.port = portnum;

if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) { - virDomainChrDefFree(chr); goto cleanup; }

- list = list->next; + value = value->next; } } else { /* If domain is not using multiple serial ports we parse data old way */ @@ -1249,16 +1207,66 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, def->consoles[0]->target.port = 0; def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } - VIR_FREE(script); + + return 0; + + cleanup: + virDomainChrDefFree(chr); + return -1; +}

I think introducing xenParseXMCharDev and xenParseConfigCommon should be done in separate patches.

+int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, + virCapsPtr caps, int xendConfigVersion ) +{ + + if (xenParseXMGeneral(conf, def, caps) < 0) + return -1; + + if (xenParseXMOS(conf, def) < 0) + return -1; + + if (xenParseXMMem(conf, def) < 0) + return -1; + if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0) + return -1; + if (xenParseXMEventsActions(conf, def) < 0) + return -1; + if (xenParseXMPCI(conf, def) < 0) + return -1; + if (xenParseXMCPUFeatures(conf, def) < 0) + return -1; + + if (xenParseXMMisc(conf, def) < 0) + return -1; + if (xenParseXMCharDev(conf, def) < 0) + return -1; + + if (xenParseXMVfb(conf, def, xendConfigVersion) < 0) + return -1; + + return 0; +} +virDomainDefPtr +xenParseXM(virConfPtr conf, int xendConfigVersion, + virCapsPtr caps) +{ + virDomainDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->virtType = VIR_DOMAIN_VIRT_XEN; + def->id = -1; + if (xenParseConfigCommon(conf, def, caps, xendConfigVersion) < 0) + goto cleanup; + if (xenParseXMVif(conf, def) < 0) + goto cleanup; + if (xenParseXMDisk(conf, def, xendConfigVersion) < 0) + goto cleanup; + return def;

cleanup: - virDomainGraphicsDefFree(graphics); - virDomainNetDefFree(net); - virDomainDiskDefFree(disk); virDomainDefFree(def); - VIR_FREE(script); - VIR_FREE(listenAddr);

All of these *Free calls should have been removed in the respective patches that removed the code from xenParseXM.

return NULL; }

diff --git a/src/xenxs/xen_xm.h b/src/xenxs/xen_xm.h index 629a4b3..621cbe1 100644 --- a/src/xenxs/xen_xm.h +++ b/src/xenxs/xen_xm.h @@ -29,7 +29,8 @@ # include "internal.h" # include "virconf.h" # include "domain_conf.h" - +int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def, + virCapsPtr caps, int xendConfigVersion);

Blank line between these function declarations. Regards, Jim

virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion);

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

refactor code parsing uuid, memory and name options

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 657dd1c..a907252 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1609,6 +1609,31 @@ xenFormatXMPCI(virConfPtr conf, either 32, or 64 on a platform where long is big enough. */ verify(MAX_VIRT_CPUS <= sizeof(1UL) * CHAR_BIT);

+static int xenFormatXMGeneral(virConfPtr conf, virDomainDefPtr def)

Similar comment here as in 1/17 regarding the name xenFormatXMGeneral. I'd suggest something like xenFormatXMGeneralMeta.

+{ + char uuid[VIR_UUID_STRING_BUFLEN]; + + if (xenXMConfigSetString(conf, "name", def->name) < 0) + return -1; + + virUUIDFormat(def->uuid, uuid); + if (xenXMConfigSetString(conf, "uuid", uuid) < 0) + return -1; + + return 0; +} +static int xenFormatXMMem(virConfPtr conf, virDomainDefPtr def) +{ + if (xenXMConfigSetInt(conf, "maxmem", + VIR_DIV_UP(def->mem.max_balloon, 1024)) < 0) + return -1; + + if (xenXMConfigSetInt(conf, "memory", + VIR_DIV_UP(def->mem.cur_balloon, 1024)) < 0) + return -1; + + return 0; +}

You split the general and mem functions in separate patches on the parse side. I think that is a good approach on the format side too. Regards, Jim

virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion) @@ -1618,27 +1643,16 @@ virConfPtr xenFormatXM(virConnectPtr conn, size_t i; char *cpus = NULL; const char *lifecycle; - char uuid[VIR_UUID_STRING_BUFLEN]; virConfValuePtr diskVal = NULL; virConfValuePtr netVal = NULL;

if (!(conf = virConfNew())) goto cleanup;

- - if (xenXMConfigSetString(conf, "name", def->name) < 0) - goto cleanup; - - virUUIDFormat(def->uuid, uuid); - if (xenXMConfigSetString(conf, "uuid", uuid) < 0) + if (xenFormatXMGeneral(conf, def) < 0) goto cleanup;

- if (xenXMConfigSetInt(conf, "maxmem", - VIR_DIV_UP(def->mem.max_balloon, 1024)) < 0) - goto cleanup; - - if (xenXMConfigSetInt(conf, "memory", - VIR_DIV_UP(def->mem.cur_balloon, 1024)) < 0) + if (xenFormatXMMem(conf, def) < 0) goto cleanup;

if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0)

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce function xenFormatXMTimeOffset(......) which formats virtual time config

Ah, on the format side you split TimeOffset and EventActions as I requested on the parse side. Looks good with the exception of the usual whitespace comments. Regards, Jim

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 149 ++++++++++++++++++++++++++++------------------------- 1 file changed, 78 insertions(+), 71 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index a907252..c91fa73 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1634,12 +1634,88 @@ static int xenFormatXMMem(virConfPtr conf, virDomainDefPtr def)

return 0; } +static int xenFormatXMTimeOffset(virConfPtr conf, virDomainDefPtr def, + int xendConfigVersion) +{ + int vmlocaltime; + if (xendConfigVersion < XEND_CONFIG_VERSION_3_1_0) { + /* <3.1: UTC and LOCALTIME */ + switch (def->clock.offset) { + case VIR_DOMAIN_CLOCK_OFFSET_UTC: + vmlocaltime = 0; + break; + case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: + vmlocaltime = 1; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported clock offset='%s'"), + virDomainClockOffsetTypeToString(def->clock.offset)); + return -1; + } + } else { + if (STREQ(def->os.type, "hvm")) { + /* >=3.1 HV: VARIABLE */ + int rtc_timeoffset; + switch (def->clock.offset) { + case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: + vmlocaltime = (int)def->clock.data.variable.basis; + rtc_timeoffset = def->clock.data.variable.adjustment; + break; + case VIR_DOMAIN_CLOCK_OFFSET_UTC: + if (def->clock.data.utc_reset) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported clock adjustment='reset'")); + return -1; + } + vmlocaltime = 0; + rtc_timeoffset = 0; + break; + case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: + if (def->clock.data.utc_reset) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupported clock adjustment='reset'")); + return -1; + } + vmlocaltime = 1; + rtc_timeoffset = 0; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported clock offset='%s'"), + virDomainClockOffsetTypeToString(def->clock.offset)); + return -1; + } + if (xenXMConfigSetInt(conf, "rtc_timeoffset", rtc_timeoffset) < 0) + return -1; + } else { + /* >=3.1 PV: UTC and LOCALTIME */ + switch (def->clock.offset) { + case VIR_DOMAIN_CLOCK_OFFSET_UTC: + vmlocaltime = 0; + break; + case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: + vmlocaltime = 1; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported clock offset='%s'"), + virDomainClockOffsetTypeToString(def->clock.offset)); + return -1; + } + } /* !hvm */ + } + if (xenXMConfigSetInt(conf, "localtime", vmlocaltime) < 0) + return -1; + + return 0; +} virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion) { virConfPtr conf = NULL; - int hvm = 0, vmlocaltime = 0; + int hvm = 0; size_t i; char *cpus = NULL; const char *lifecycle; @@ -1778,78 +1854,9 @@ virConfPtr xenFormatXM(virConnectPtr conn, goto cleanup; } /* !hvm */

- - if (xendConfigVersion < XEND_CONFIG_VERSION_3_1_0) { - /* <3.1: UTC and LOCALTIME */ - switch (def->clock.offset) { - case VIR_DOMAIN_CLOCK_OFFSET_UTC: - vmlocaltime = 0; - break; - case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: - vmlocaltime = 1; - break; - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported clock offset='%s'"), - virDomainClockOffsetTypeToString(def->clock.offset)); - goto cleanup; - } - } else { - if (hvm) { - /* >=3.1 HV: VARIABLE */ - int rtc_timeoffset; - switch (def->clock.offset) { - case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: - vmlocaltime = (int)def->clock.data.variable.basis; - rtc_timeoffset = def->clock.data.variable.adjustment; - break; - case VIR_DOMAIN_CLOCK_OFFSET_UTC: - if (def->clock.data.utc_reset) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unsupported clock adjustment='reset'")); - goto cleanup; - } - vmlocaltime = 0; - rtc_timeoffset = 0; - break; - case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: - if (def->clock.data.utc_reset) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unsupported clock adjustment='reset'")); - goto cleanup; - } - vmlocaltime = 1; - rtc_timeoffset = 0; - break; - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported clock offset='%s'"), - virDomainClockOffsetTypeToString(def->clock.offset)); - goto cleanup; - } - if (xenXMConfigSetInt(conf, "rtc_timeoffset", rtc_timeoffset) < 0) - goto cleanup; - } else { - /* >=3.1 PV: UTC and LOCALTIME */ - switch (def->clock.offset) { - case VIR_DOMAIN_CLOCK_OFFSET_UTC: - vmlocaltime = 0; - break; - case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: - vmlocaltime = 1; - break; - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported clock offset='%s'"), - virDomainClockOffsetTypeToString(def->clock.offset)); - goto cleanup; - } - } /* !hvm */ - } - if (xenXMConfigSetInt(conf, "localtime", vmlocaltime) < 0) + if (xenFormatXMTimeOffset(conf, def, xendConfigVersion) < 0) goto cleanup;

- if (!(lifecycle = virDomainLifecycleTypeToString(def->onPoweroff))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected lifecycle action %d"), def->onPoweroff);

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce function xenFormatXMEventActions(.....) which formats actions following certain events

Looks good with exception of my tiring whitespace comments :-). Regards, Jim

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 60 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index c91fa73..367a8cd 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1710,6 +1710,38 @@ static int xenFormatXMTimeOffset(virConfPtr conf, virDomainDefPtr def,

return 0; } +static int xenFormatXMEventActions(virConfPtr conf, virDomainDefPtr def) +{ + const char *lifecycle = NULL; + + if (!(lifecycle = virDomainLifecycleTypeToString(def->onPoweroff))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected lifecycle action %d"), def->onPoweroff); + return -1; + } + if (xenXMConfigSetString(conf, "on_poweroff", lifecycle) < 0) + return -1; + + + if (!(lifecycle = virDomainLifecycleTypeToString(def->onReboot))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected lifecycle action %d"), def->onReboot); + return -1; + } + if (xenXMConfigSetString(conf, "on_reboot", lifecycle) < 0) + return -1; + + + if (!(lifecycle = virDomainLifecycleCrashTypeToString(def->onCrash))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected lifecycle action %d"), def->onCrash); + return -1; + } + if (xenXMConfigSetString(conf, "on_crash", lifecycle) < 0) + return -1; + + return 0; +} virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion) @@ -1718,7 +1750,6 @@ virConfPtr xenFormatXM(virConnectPtr conn, int hvm = 0; size_t i; char *cpus = NULL; - const char *lifecycle; virConfValuePtr diskVal = NULL; virConfValuePtr netVal = NULL;

@@ -1857,33 +1888,8 @@ virConfPtr xenFormatXM(virConnectPtr conn, if (xenFormatXMTimeOffset(conf, def, xendConfigVersion) < 0) goto cleanup;

- if (!(lifecycle = virDomainLifecycleTypeToString(def->onPoweroff))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected lifecycle action %d"), def->onPoweroff); - goto cleanup; - } - if (xenXMConfigSetString(conf, "on_poweroff", lifecycle) < 0) - goto cleanup; - - - if (!(lifecycle = virDomainLifecycleTypeToString(def->onReboot))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected lifecycle action %d"), def->onReboot); - goto cleanup; - } - if (xenXMConfigSetString(conf, "on_reboot", lifecycle) < 0) - goto cleanup; - - - if (!(lifecycle = virDomainLifecycleCrashTypeToString(def->onCrash))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected lifecycle action %d"), def->onCrash); + if (xenFormatXMEventActions(conf, def) < 0) goto cleanup; - } - if (xenXMConfigSetString(conf, "on_crash", lifecycle) < 0) - goto cleanup; - -

if (hvm) { if (def->emulator &&

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce function xenFormatXMCharDev(....); which formats char devices config

signed-off-by:David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 155 ++++++++++++++++++++++++++++------------------------- 1 file changed, 82 insertions(+), 73 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 367a8cd..ee5dc19 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1742,6 +1742,85 @@ static int xenFormatXMEventActions(virConfPtr conf, virDomainDefPtr def)

return 0; } +static int xenFormatXMCharDev(virConfPtr conf, virDomainDefPtr def) +{ + size_t i; + if (STREQ(def->os.type, "hvm")) { + if (def->nparallels) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *str; + int ret; + + ret = xenFormatSxprChr(def->parallels[0], &buf); + str = virBufferContentAndReset(&buf); + if (ret == 0) + ret = xenXMConfigSetString(conf, "parallel", str); + VIR_FREE(str); + if (ret < 0) + goto cleanup; + } else { + if (xenXMConfigSetString(conf, "parallel", "none") < 0) + goto cleanup; + } + + if (def->nserials) { + if ((def->nserials == 1) && (def->serials[0]->target.port == 0)) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *str; + int ret; + + ret = xenFormatSxprChr(def->serials[0], &buf); + str = virBufferContentAndReset(&buf); + if (ret == 0) + ret = xenXMConfigSetString(conf, "serial", str); + VIR_FREE(str); + if (ret < 0) + goto cleanup; + } else { + size_t j = 0; + int maxport = -1, port; + virConfValuePtr serialVal = NULL; + + if (VIR_ALLOC(serialVal) < 0) + goto cleanup; + serialVal->type = VIR_CONF_LIST; + serialVal->list = NULL; + + for (i = 0; i < def->nserials; i++) + if (def->serials[i]->target.port > maxport) + maxport = def->serials[i]->target.port; + + for (port = 0; port <= maxport; port++) { + virDomainChrDefPtr chr = NULL; + for (j = 0; j < def->nserials; j++) { + if (def->serials[j]->target.port == port) { + chr = def->serials[j]; + break; + } + } + if (xenFormatXMSerial(serialVal, chr) < 0) { + virConfFreeValue(serialVal); + goto cleanup; + } + } + + if (serialVal->list != NULL) { + int ret = virConfSetValue(conf, "serial", serialVal); + serialVal = NULL; + if (ret < 0) + goto cleanup; + } + VIR_FREE(serialVal); + } + } else { + if (xenXMConfigSetString(conf, "serial", "none") < 0) + goto cleanup; + } + } + return 0; +cleanup: + return -1; +} virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion) @@ -2067,79 +2146,10 @@ virConfPtr xenFormatXM(virConnectPtr conn, if (xenFormatXMPCI(conf, def) < 0) goto cleanup;

- if (hvm) { - if (def->nparallels) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *str; - int ret; - - ret = xenFormatSxprChr(def->parallels[0], &buf); - str = virBufferContentAndReset(&buf); - if (ret == 0) - ret = xenXMConfigSetString(conf, "parallel", str); - VIR_FREE(str); - if (ret < 0) - goto cleanup; - } else { - if (xenXMConfigSetString(conf, "parallel", "none") < 0) - goto cleanup; - } - - if (def->nserials) { - if ((def->nserials == 1) && (def->serials[0]->target.port == 0)) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *str; - int ret; - - ret = xenFormatSxprChr(def->serials[0], &buf); - str = virBufferContentAndReset(&buf); - if (ret == 0) - ret = xenXMConfigSetString(conf, "serial", str); - VIR_FREE(str); - if (ret < 0) - goto cleanup; - } else { - size_t j = 0; - int maxport = -1, port; - virConfValuePtr serialVal = NULL; - - if (VIR_ALLOC(serialVal) < 0) - goto cleanup; - serialVal->type = VIR_CONF_LIST; - serialVal->list = NULL; - - for (i = 0; i < def->nserials; i++) - if (def->serials[i]->target.port > maxport) - maxport = def->serials[i]->target.port; - - for (port = 0; port <= maxport; port++) { - virDomainChrDefPtr chr = NULL; - for (j = 0; j < def->nserials; j++) { - if (def->serials[j]->target.port == port) { - chr = def->serials[j]; - break; - } - } - if (xenFormatXMSerial(serialVal, chr) < 0) { - virConfFreeValue(serialVal); - goto cleanup; - } - } - - if (serialVal->list != NULL) { - int ret = virConfSetValue(conf, "serial", serialVal); - serialVal = NULL; - if (ret < 0) - goto cleanup; - } - VIR_FREE(serialVal); - } - } else { - if (xenXMConfigSetString(conf, "serial", "none") < 0) - goto cleanup; - } - + if (xenFormatXMCharDev(conf, def) < 0) + goto cleanup;

+ if (hvm) { if (def->sounds) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *str = NULL; @@ -2153,7 +2163,6 @@ virConfPtr xenFormatXM(virConnectPtr conn, goto cleanup; } } -

Spurious whitespace change. Regards, Jim

return conf;

cleanup:

David Kiarie wrote:

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce the function xenFormatXMDomainNet(......)

On the parsing side, you called this xenParseXMVif. To be consistent, this should be xenFormatXMVif.

I think this could be done in a cleaner way

signed-of-by: David Kiarie<davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index ee5dc19..8dd2823 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1821,6 +1821,36 @@ static int xenFormatXMCharDev(virConfPtr conf, virDomainDefPtr def) cleanup: return -1; } +static int xenFormatXMDomainNet(virConfPtr conf, virConnectPtr conn, + virDomainDefPtr def, int xendConfigVersion) +{ + virConfValuePtr netVal = NULL; + size_t i; + + int hvm = STREQ(def->os.type, "hvm"); + + if (VIR_ALLOC(netVal) < 0) + goto cleanup; + netVal->type = VIR_CONF_LIST; + netVal->list = NULL; + + for (i = 0; i < def->nnets; i++) { + if (xenFormatXMNet(conn, netVal, def->nets[i], + hvm, xendConfigVersion) < 0)

Ah, I see xenFormatXMNet already exists. So maybe xenFormatXMVifs for the list of VIFs and xenFormatXMVif for each individual VIF is clearer. Regards, Jim

+ return -1; + } + if (netVal->list != NULL) { + int ret = virConfSetValue(conf, "vif", netVal); + netVal = NULL; + if (ret < 0) + return -1; + } + VIR_FREE(netVal); + return 0; + cleanup: + VIR_FREE(netVal); + return -1; +} virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def, int xendConfigVersion) @@ -2124,25 +2154,8 @@ virConfPtr xenFormatXM(virConnectPtr conn, goto cleanup; } VIR_FREE(diskVal); - - if (VIR_ALLOC(netVal) < 0) + if (xenFormatXMDomainNet(conf, conn, def, xendConfigVersion) < 0) goto cleanup; - netVal->type = VIR_CONF_LIST; - netVal->list = NULL; - - for (i = 0; i < def->nnets; i++) { - if (xenFormatXMNet(conn, netVal, def->nets[i], - hvm, xendConfigVersion) < 0) - goto cleanup; - } - if (netVal->list != NULL) { - int ret = virConfSetValue(conf, "vif", netVal); - netVal = NULL; - if (ret < 0) - goto cleanup; - } - VIR_FREE(netVal); - if (xenFormatXMPCI(conf, def) < 0) goto cleanup;

Eric Blake wrote:

On 07/10/2014 07:42 AM, David Kiarie wrote:

...

From: Kiarie Kahurani <davidkiarie4@gmail.com>

Introduce function xenParseXMGeneral(virConfPtr conf, ...); This function parses the xm general options such as uuid and name

[meta-comment]:

When posting a series, it's useful to post a cover letter (0/17) to which all patches reply to, rather than having 2-17 reply to 1/17 with no lead-in on the series as a whole.

David, you can add '--cover-letter --annotate' to your git send-email command to have git generate the cover letter and let you edit it before sending.

...

signed-off-by: David Kiarie <davidkiarie4@gmail.com> --- src/xenxs/xen_xm.c | 65 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 25a042d..1953a85 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -253,44 +253,25 @@ xenXMConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) * Turn a config record into a lump of XML describing the * domain, suitable for later feeding for virDomainCreateXML */ -virDomainDefPtr -xenParseXM(virConfPtr conf, int xendConfigVersion, - virCapsPtr caps) +static int xenParseXMGeneral(virConfPtr conf, virDomainDefPtr def, + virCapsPtr caps)

Indentation is off; per our style, this should be:

static int xenParseXMGeneral(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)

...

+} +virDomainDefPtr

2 blank lines between functions.

...

+xenParseXM(virConfPtr conf, int xendConfigVersion, + virCapsPtr caps)

Indentation is off; the second line should line up to the character after the ( of the first line.

Otherwise, this looks like it is just a refactoring of splitting out a helper function for later reuse.

David is looking into supporting Xen's xl config format in domxml-{from,to}-native. For the most part, xm is a subset of xl, so a lot of the xm parsing code can be used by the xl parser. David, this type of info would be useful in a cover letter, so reviewers know what all the code motion is about :-). Regards, Jim