[ovs-dev] [PATCH 09/15] datapath-windows: Add macros in Debug.h

Samuel Ghinet sghinet at cloudbasesolutions.com
Thu Aug 21 19:40:04 UTC 2014


Hello Nithin,

[QUOTE]
I don't think we need two separate definitions. If ASSERT() is defined debug only, then it would be a no-op in 'OVS_USE_ASSERTS'.
[/QUOTE]
The more complex assert-based macros would be useful if we would want to do a more 'proper' cleanup in release mode, for a bug.

e.g.
if we have, say, within a function (I give some generic names)
pObj1 = LookupObject(container, cond1);
ASSERT(pObj1); //normally, pObj1 must be there, otherwise it's a bug
if (!pObj1) {
LOG_ERROR(err1)
return error_x;
}

pObj2 = LookupObject(container, cond2);
ASSERT(pObj2); //normally, pObj2 must be there, otherwise it's a bug
if (!pObj2) {
LOG_ERROR(err2)
return error_x;
}

...

A cleaner and more concise solution would be to do:
pObj1 = LookupObject(container, cond1);
OVS_CHECK_XXY(pObj1, err1);

pObj2 = LookupObject(container, cond2);
OVS_CHECK_XXY(pObj2, err2);

I think this patch was useful if the vport patch was added. Anyway, it's not important now.

Sam
________________________________________
From: Nithin Raju [nithin at vmware.com]
Sent: Saturday, August 16, 2014 9:47 AM
To: Samuel Ghinet
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH 09/15] datapath-windows: Add macros in Debug.h

Sam,
There are lot of assumptions here about how code should be structured and written. eg. there's an implicit assumption that 'Cleanup' should be defined.

Let's table this for sometime till we get the current code starts working with netlink.

If you can point me to some code what is using these macros, I might be able to comment better.

> +//OVS_USE_ASSERTS is not #define-d on release mode
> +#if OVS_USE_ASSERTS
> +#define OVS_CHECK(x) ASSERT(x)
> +#define OVS_CHECK_OR(x, expr) { if (!(x)) { ASSERT(0); expr; } }
> +#define OVS_CHECK_BREAK(x) { if (!(x)) { ASSERT(0); break; } }
> +#define OVS_CHECK_RET(x, value) { if (!(x)) { ASSERT(0); return value; } }
> +#define OVS_CHECK_GC(x) { if (!(x)) { ASSERT(0); goto Cleanup; } }
> +#else
> +#define OVS_CHECK(x)
> +#define OVS_CHECK_OR(x, expr) { if (!(x)) expr; }
> +#define OVS_CHECK_BREAK(x) { if (!(x)) break; }
> +#define OVS_CHECK_RET(x, value) { if (!(x)) return value; }
> +#define OVS_CHECK_GC(x) { if (!(x)) goto Cleanup; }
> +#endif //OVS_USE_ASSERTS

I don't think we need two separate definitions. If ASSERT() is defined debug only, then it would be a no-op in 'OVS_USE_ASSERTS'.

thanks,
Nithin



More information about the dev mailing list