[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/3] datatypes: avoid redundant __FUNCTION__



2010/5/12 Eric Blake <eblake redhat com>:
> virLibConnError already includes __FUNCTION__ in its output, so we
> were redundant.  Furthermore, clang warns that __FUNCTION__ is not
> a string literal (at least __FUNCTION__ will never contain %, so
> it was not a security risk).
>
> * src/datatypes.c: Replace __FUNCTION__ with a descriptive string.
> ---
>  src/datatypes.c |   36 ++++++++++++++++++------------------
>  1 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/src/datatypes.c b/src/datatypes.c
> index 25962a6..cea8765 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -287,7 +287,7 @@ virUnrefConnect(virConnectPtr conn) {
>     int refs;
>
>     if ((!VIR_IS_CONNECT(conn))) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

As Chris pointed out a while ago:

  https://www.redhat.com/archives/libvir-list/2010-April/msg01241.html

There are some error codes that have string representations attached
which imply a certain usage style. VIR_ERR_INVALID_ARG is one of
those. See virErrorMsg:

        case VIR_ERR_INVALID_ARG:
            if (info == NULL)
                errmsg = _("invalid argument in");
            else
                errmsg = _("invalid argument in %s");
            break;

This implies that you pass the function name as additional
information. But I must admit that I never used VIR_ERR_INVALID_ARG as
it is implied, because I wasn't aware of that until recently.

The VIR_ERR_XML_ERROR is an even worse example:

        case VIR_ERR_XML_ERROR:
            if (info == NULL)
                errmsg = _("XML description not well formed or invalid");
            else
                errmsg = _("XML description for %s is not well formed
or invalid");
            break;

Unrelated to your patch I suggest that we unify the string
representations for error codes to a common style:

        case VIR_ERR_INVALID_ARG:
            if (info == NULL)
                errmsg = _("invalid argument");
            else
                errmsg = _("invalid argument: %s");
            break;

        case VIR_ERR_XML_ERROR:
            if (info == NULL)
                errmsg = _("XML description not well formed or invalid");
            else
                errmsg = _("XML description not well formed or invalid: %s");
            break;

And adapt the callers.

>         return(-1);
>     }
>     virMutexLock(&conn->lock);
> @@ -345,7 +345,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>     virDomainPtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -455,7 +455,7 @@ virUnrefDomain(virDomainPtr domain) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

VIR_IS_CONNECTED_DOMAIN is false if domain isn't a domain, or if it's
connection object isn't a connection. Therefore "no connection" is
incomplete.

>         return(-1);
>     }
>     virMutexLock(&domain->conn->lock);
> @@ -490,7 +490,7 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>     virNetworkPtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -592,7 +592,7 @@ virUnrefNetwork(virNetworkPtr network) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_NETWORK(network)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

As with VIR_IS_CONNECTED_DOMAIN above "no connection" is incomplete here.

>         return(-1);
>     }
>     virMutexLock(&network->conn->lock);
> @@ -629,7 +629,7 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
>     virInterfacePtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/2 of the possibilities to get into this block.

>         return(NULL);
>     }
>
> @@ -774,7 +774,7 @@ virUnrefInterface(virInterfacePtr iface) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_INTERFACE(iface)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

The same as with VIR_IS_CONNECTED_DOMAIN and VIR_IS_CONNECTED_NETWORK above.

>         return(-1);
>     }
>     virMutexLock(&iface->conn->lock);
> @@ -810,7 +810,7 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui
>     virStoragePoolPtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -913,7 +913,7 @@ virUnrefStoragePool(virStoragePoolPtr pool) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is incomplete here too.

>         return(-1);
>     }
>     virMutexLock(&pool->conn->lock);
> @@ -950,7 +950,7 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c
>     virStorageVolPtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (key == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -1062,7 +1062,7 @@ virUnrefStorageVol(virStorageVolPtr vol) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

And another incomplete message.

>         return(-1);
>     }
>     virMutexLock(&vol->conn->lock);
> @@ -1098,7 +1098,7 @@ virGetNodeDevice(virConnectPtr conn, const char *name)
>     virNodeDevicePtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/2 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -1230,7 +1230,7 @@ virGetSecret(virConnectPtr conn, const unsigned char *uuid,
>     char uuidstr[VIR_UUID_STRING_BUFLEN];
>
>     if (!VIR_IS_CONNECT(conn) || uuid == NULL || usageID == NULL) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return NULL;
>     }
>     virMutexLock(&conn->lock);
> @@ -1329,7 +1329,7 @@ virUnrefSecret(virSecretPtr secret) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_SECRET(secret)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

And another incomplete message.

>         return -1;
>     }
>     virMutexLock(&secret->conn->lock);
> @@ -1423,7 +1423,7 @@ virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid)
>     virNWFilterPtr ret = NULL;
>
>     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

"no connection" is only true in 1/3 of the possibilities to get into this block.

>         return(NULL);
>     }
>     virMutexLock(&conn->lock);
> @@ -1526,7 +1526,7 @@ virUnrefNWFilter(virNWFilterPtr pool) {
>     int refs;
>
>     if (!VIR_IS_CONNECTED_NWFILTER(pool)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("no connection"));

And another incomplete message.

>         return -1;
>     }
>     virMutexLock(&pool->conn->lock);
> @@ -1550,7 +1550,7 @@ virGetDomainSnapshot(virDomainPtr domain, const char *name)
>     virDomainSnapshotPtr ret = NULL;
>
>     if ((!VIR_IS_DOMAIN(domain)) || (name == NULL)) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, _("not a snapshot"));

This block is entered if domain is no domain or name is NULL,
therefore "not a snapshot" is a misleading error message here.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]