On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> ---
> src/conf/capabilities.c | 30 +++++--------
Reviewed earlier.
> src/conf/cpu_conf.c | 19 ++++----
> src/conf/domain_conf.c | 105 ++++++++++++++------------------------------
> src/conf/domain_event.c | 39 ++++++++--------
> src/conf/node_device_conf.c | 29 ++++++------
> src/conf/nwfilter_conf.c | 17 ++-----
> src/conf/nwfilter_params.c | 30 ++++---------
> src/conf/snapshot_conf.c | 11 ++---
> src/conf/storage_conf.c | 13 ++----
> src/conf/virchrdev.c | 12 ++---
Part two of the review:
> +++ b/src/conf/cpu_conf.c
> @@ -99,10 +99,10 @@ virCPUDefCopyModel(virCPUDefPtr dst,
> {
> unsigned int i;
>
> - if ((src->model && !(dst->model = strdup(src->model)))
> - || (src->vendor && !(dst->vendor = strdup(src->vendor)))
> - || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id)))
> - || VIR_ALLOC_N(dst->features, src->nfeatures) < 0)
> + if ((src->model && VIR_STRDUP(dst->model, src->model) < 0) ||
> + (src->vendor && VIR_STRDUP(dst->vendor, src->vendor) < 0) ||
> + (src->vendor_id && VIR_STRDUP(dst->vendor_id, src->vendor_id) < 0) ||
> + VIR_ALLOC_N(dst->features, src->nfeatures) < 0)
> goto no_memory;
Double OOM, but you also have a VIR_ALLOC, so this will be cleaned up in
a later pass.
> dst->nfeatures_max = dst->nfeatures = src->nfeatures;
>
> @@ -118,7 +118,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,
> dst->features[i].policy = src->features[i].policy;
> }
>
> - if (!(dst->features[i].name = strdup(src->features[i].name)))
> + if (VIR_STRDUP(dst->features[i].name, src->features[i].name) < 0)
> goto no_memory;
Double OOM reporting; I'd just inline 'return -1' here instead of using
goto.
> @@ -167,8 +167,8 @@ virCPUDefCopy(const virCPUDefPtr cpu)
> if (!copy->cells[i].cpumask)
> goto no_memory;
>
> - if (!(copy->cells[i].cpustr = strdup(cpu->cells[i].cpustr)))
> - goto no_memory;
> + if (VIR_STRDUP(copy->cells[i].cpustr, cpu->cells[i].cpustr) < 0)
> + goto error;
Yay - clean conversion!
> @@ -694,8 +694,8 @@ virCPUDefAddFeature(virCPUDefPtr def,
> if (def->type == VIR_CPU_TYPE_HOST)
> policy = -1;
>
> - if (!(def->features[def->nfeatures].name = strdup(name)))
> - goto no_memory;
> + if (VIR_STRDUP(def->features[def->nfeatures].name, name) < 0)
> + goto error;
>
> def->features[def->nfeatures].policy = policy;
> def->nfeatures++;
> @@ -704,6 +704,7 @@ virCPUDefAddFeature(virCPUDefPtr def,
>
> no_memory:
> virReportOOMError();
> +error:
> return -1;
You could drop the error label and just inline the return -1.
> +++ b/src/conf/domain_conf.c
> @@ -1352,59 +1352,42 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
> case VIR_DOMAIN_CHR_TYPE_FILE:
> case VIR_DOMAIN_CHR_TYPE_PIPE:
> if (src->data.file.path &&
> - !(dest->data.file.path = strdup(src->data.file.path))) {
> - virReportOOMError();
> + VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0)
> return -1;
More instances where allowing a NULL source would simplify callers.
> @@ -6133,7 +6106,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> addrtype = virXPathString("string(./source/address/@type)", ctxt);
> /* if not explicitly stated, source/vendor implies usb device */
> if (!addrtype && virXPathNode("./source/vendor", ctxt) &&
> - ((addrtype = strdup("usb")) == NULL)) {
> + VIR_STRDUP(addrtype, "usb") < 0) {
> virReportOOMError();
Drop this double OOM.
> @@ -9679,9 +9648,7 @@ virDomainDefGetDefaultEmulator(virDomainDefPtr def,
> return NULL;
> }
>
> - if (!(retemu = strdup(emulator)))
> - virReportOOMError();
> -
> + ignore_value(VIR_STRDUP(retemu, emulator));
> return retemu;
This one caught my eye, but it looks correct after all.
> @@ -10857,8 +10821,8 @@ virDomainDefParseXML(xmlDocPtr xml,
> def->os.arch,
> virDomainVirtTypeToString(def->virtType));
> if (defaultMachine != NULL) {
> - if (!(def->os.machine = strdup(defaultMachine))) {
> - goto no_memory;
> + if (VIR_STRDUP(def->os.machine, defaultMachine) < 0) {
> + goto error;
> }
Another case that could be simplified by taking a NULL source.
> @@ -16490,7 +16455,7 @@ virDomainObjListCopyInactiveNames(void *payload,
>
> virObjectLock(obj);
> if (!virDomainObjIsActive(obj) && data->numnames < data->maxnames) {
> - if (!(data->names[data->numnames] = strdup(obj->def->name)))
> + if (VIR_STRDUP(data->names[data->numnames], obj->def->name) < 0)
> data->oom = 1;
This change is fine, but incomplete. You need to fix
virDomainObjListGetInactiveNames a few lines later to quit reporting OOM
when it sees data->oom set.
> @@ -17425,7 +17384,7 @@ virDomainDefGenSecurityLabelDef(const char *model)
> virSecurityLabelDefPtr seclabel = NULL;
>
> if (VIR_ALLOC(seclabel) < 0 ||
> - (model && !(seclabel->model = strdup(model)))) {
> + (model && VIR_STRDUP(seclabel->model, model) < 0)) {
> virReportOOMError();
double-oom is excusable because of VIR_ALLOC
> @@ -17440,7 +17399,7 @@ virDomainDiskDefGenSecurityLabelDef(const char *model)
> virSecurityDeviceLabelDefPtr seclabel = NULL;
>
> if (VIR_ALLOC(seclabel) < 0 ||
> - (model && !(seclabel->model = strdup(model)))) {
> + (model && VIR_STRDUP(seclabel->model, model) < 0)) {
> virReportOOMError();
and again
> +++ b/src/conf/domain_event.c
> @@ -791,9 +791,9 @@ static virDomainEventPtr virDomainEventIOErrorNewFromDomImpl(int event,
>
> if (ev) {
> ev->data.ioError.action = action;
> - if (!(ev->data.ioError.srcPath = strdup(srcPath)) ||
> - !(ev->data.ioError.devAlias = strdup(devAlias)) ||
> - (reason && !(ev->data.ioError.reason = strdup(reason)))) {
> + if (VIR_STRDUP(ev->data.ioError.srcPath, srcPath) < 0 ||
> + VIR_STRDUP(ev->data.ioError.devAlias, devAlias) < 0 ||
> + (reason && VIR_STRDUP(ev->data.ioError.reason, reason) < 0)) {
> virDomainEventFree(ev);
> ev = NULL;
Changed from silent to noisy. Only caller appears to be
remote_driver.c:remoteDomainBuildEventIOError, where failure is ignored
(the event is basically discarded). Ouch - that code has a pre-existing
bug - a NULL return here is eventually passed on to
virDomainEventQueuePush, which queues up a NULL, and later servicing of
the queue will do a NULL deref in virDomainEventDispatchMatchCallback.
Looks like we need to fix that, and now's as good a time as any,
possibly by fixing virDomainEventStateQueue to ignore a NULL event push,
or possibly by fixing remote_driver.c.
> @@ -815,9 +815,9 @@ static virDomainEventPtr virDomainEventIOErrorNewFromObjImpl(int event,
>
> if (ev) {
> ev->data.ioError.action = action;
> - if (!(ev->data.ioError.srcPath = strdup(srcPath)) ||
> - !(ev->data.ioError.devAlias = strdup(devAlias)) ||
> - (reason && !(ev->data.ioError.reason = strdup(reason)))) {
> + if (VIR_STRDUP(ev->data.ioError.srcPath, srcPath) < 0 ||
> + VIR_STRDUP(ev->data.ioError.devAlias, devAlias) < 0 ||
> + (reason && VIR_STRDUP(ev->data.ioError.reason, reason) < 0)) {
> virDomainEventFree(ev);
> ev = NULL;
Changed from silent to noisy. Only caller appears to be
qemu_process.c:qemuProcessHandleIOError, but at least this one properly
discards the error without trying to queue up NULL.
I'm wondering if these two particular functions should use the _QUIET
variant, so we aren't polluting the thread-local error object in a case
where the caller is fine discarding the error if it could not be created.
> @@ -882,7 +882,7 @@ virDomainEventPtr virDomainEventGraphicsNewFromDom(virDomainPtr dom,
>
> if (ev) {
> ev->data.graphics.phase = phase;
> - if (!(ev->data.graphics.authScheme = strdup(authScheme))) {
> + if (VIR_STRDUP(ev->data.graphics.authScheme, authScheme) < 0) {
> virDomainEventFree(ev);
> return NULL;
silent->noisy. I did not audit this one for callers, but suspect it
will be similar to IOError events: we probably have a bug in
remote_driver.c for trying to queue NULL, and since all callers probably
ignore an event object that could not be queued, using _QUIET here may
be appropriate.
Hmm, a bit more thinking: all events are asynchronous push, rather than
synchronous replies to the client's request; which explains why we
ignore OOM in creating an event - we aren't under obligation to send a
reply. But I'm wondering if we should have code that tries to deliver
an OOM event (of course, pre-created at the time the connection is first
established, so that actually sending the OOM event doesn't involve any
use of malloc), where hitting OOM and discarding any normal event then
falls back to trying to fire the OOM event. Then again, OOM is such a
rare corner case, and tends to be somewhat persistent as well; once we
hit OOM trying to generate an error, we will probably also hit OOM on
the next synchronous call from the client, so it won't be too much
longer before they learn about it normally, without us having to worry
about a special OOM event. So ignore this paragraph if you think I'm
over-engineering things.
> @@ -907,7 +907,7 @@ virDomainEventPtr virDomainEventGraphicsNewFromObj(virDomainObjPtr obj,
>
> if (ev) {
> ev->data.graphics.phase = phase;
> - if (!(ev->data.graphics.authScheme = strdup(authScheme))) {
> + if (VIR_STRDUP(ev->data.graphics.authScheme, authScheme) < 0) {
> virDomainEventFree(ev);
> return NULL;
> }
> @@ -928,8 +928,7 @@ virDomainEventBlockJobNew(int id, const char *name, unsigned char *uuid,
> id, name, uuid);
>
> if (ev) {
> - if (!(ev->data.blockJob.path = strdup(path))) {
> - virReportOOMError();
> + if (VIR_STRDUP(ev->data.blockJob.path, path) < 0) {
> virDomainEventFree(ev);
> return NULL;
Hmm, we weren't very consistent on which events were already noisy on
creation failure. I guess that argues that using _QUIET variants is
overkill, because we already have cases where an OOM will set the
thread-local error object even if it event is discarded.
> +++ b/src/conf/node_device_conf.c
> @@ -1284,14 +1280,19 @@ virNodeDeviceGetWWNs(virNodeDeviceDefPtr def,
> char **wwpn)
> {
> virNodeDevCapsDefPtr cap = NULL;
> - int ret = 0;
> + int ret = -1;
>
> cap = def->caps;
> while (cap != NULL) {
> if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST &&
> cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> - *wwnn = strdup(cap->data.scsi_host.wwnn);
> - *wwpn = strdup(cap->data.scsi_host.wwpn);
> + if (VIR_STRDUP(*wwnn, cap->data.scsi_host.wwnn) < 0 ||
> + VIR_STRDUP(*wwpn, cap->data.scsi_host.wwpn) < 0) {
> + /* Free the other one, if allocated... */
> + VIR_FREE(*wwnn);
> + VIR_FREE(*wwpn);
Technically, *wwpn is already NULL if we get here, so you can drop this
second VIR_FREE.
> +++ b/src/conf/nwfilter_params.c
> @@ -80,8 +80,7 @@ virNWFilterVarValueCopy(const virNWFilterVarValuePtr val)
> switch (res->valType) {
> case NWFILTER_VALUE_TYPE_SIMPLE:
> if (val->u.simple.value) {
> - res->u.simple.value = strdup(val->u.simple.value);
> - if (!res->u.simple.value)
> + if (VIR_STRDUP(res->u.simple.value, val->u.simple.value) < 0)
> goto err_exit;
double OOM here, but okay because VIR_ALLOC also uses the label.
> @@ -90,8 +89,7 @@ virNWFilterVarValueCopy(const virNWFilterVarValuePtr val)
> goto err_exit;
> res->u.array.nValues = val->u.array.nValues;
> for (i = 0; i < val->u.array.nValues; i++) {
> - str = strdup(val->u.array.values[i]);
> - if (!str)
> + if (VIR_STRDUP(str, val->u.array.values[i]) < 0)
> goto err_exit;
Again, VIR_ALLOC cleanup will cover this case of double oom
> @@ -654,17 +650,15 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table,
> {
> if (!virHashLookup(table->hashTable, name)) {
> if (copyName) {
> - name = strdup(name);
> - if (!name) {
> - virReportOOMError();
> + char *newName;
> + if (VIR_STRDUP(newName, name) < 0)
> return -1;
> - }
>
> if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) {
> VIR_FREE(name);
> return -1;
Hmm, this code was missing oom reporting; but we'll clean that up on the
VIR_ALLOC pass.
> +++ b/src/conf/virchrdev.c
> @@ -52,7 +52,7 @@ typedef struct _virChrdevStreamInfo virChrdevStreamInfo;
> typedef virChrdevStreamInfo *virChrdevStreamInfoPtr;
> struct _virChrdevStreamInfo {
> virChrdevsPtr devs;
> - const char *path;
> + char *path;
> };
Interesting. But looks correct.
The majority of this patch looks reasonable. I already split
capabilities.c into a separate review. domain_event.c is the only
remaining file out of this lot that might warrant a split into its own
patch, if you want to tackle making all event clients deal sanely with
consistent semantics on what happens if you hit OOM when trying to
report an event (and again, I'm not sure if the sanest is to clear the
thread-local error, leave it polluted, or switch to _QUIET variants
since we know that all events are best-effort only).
--
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