Avoid (when possible) calling sys_*getxattr with NULL size

[mykaul]

In several cases in the code [for example, posix_cs_set_state() ], we have this code convention that we do some getxattr without a buffer to get the buffer we need, then we CALLOC (we could MALLOC, btw) the buffer and do another fetch with the size.

In other cases, we are more efficient. We have some buffer, and we either use it, or dynamically realloc if the first getxattr fails. This is better as it removes an xattr in case the value fits the buffer.

Bad example:

       xattrsize = sys_fgetxattr(*fd, GF_CS_OBJECT_REMOTE, NULL, 0);
        if (xattrsize != -1) {
            value = GF_CALLOC(1, xattrsize + 1, gf_posix_mt_char);
            if (!value) {
                gf_msg(this->name, GF_LOG_ERROR, 0, 0, "no memory for value");
                ret = -1;
                goto out;
            }
            /* TODO: Add check for ENODATA */
            xattrsize = sys_fgetxattr(*fd, GF_CS_OBJECT_REMOTE, value,
                                      xattrsize + 1);

Slightly better example: (but still suffers from extra memory copy which can be removed)

    if (filler->real_path)
        xattr_size = sys_lgetxattr(filler->real_path, key, val_buf,
                                   sizeof(val_buf) - 1);
    else
        xattr_size = sys_fgetxattr(filler->fdnum, key, val_buf,
                                   sizeof(val_buf) - 1);

    if (xattr_size >= 0) {
        have_val = _gf_true;
    } else if (xattr_size == -1 && errno != ERANGE) {
        ret = -1;
        goto out;
    }

    if (have_val) {
        /*No need to do getxattr*/
    } else if (filler->real_path) {
        xattr_size = sys_lgetxattr(filler->real_path, key, NULL, 0);
    } else {
        xattr_size = sys_fgetxattr(filler->fdnum, key, NULL, 0);
    }

    if (xattr_size != -1) {
        value = GF_MALLOC(xattr_size + 1, gf_posix_mt_char);
        if (!value)
            goto out;

        if (have_val) {
            memcpy(value, val_buf, xattr_size);
        } else {
            bzero(value, xattr_size + 1);
            if (filler->real_path) {
                xattr_size = sys_lgetxattr(filler->real_path, key, value,
                                           xattr_size);

The places I found the bad implementation:

• posix_fetch_mdata_xattr()
• posix_cs_set_state()
• posix_cs_check_status()
• _posix_xattr_get_set_from_backend
• _get_list_xattr() - really bad (called many times)
• posix_fgetxattr

[atinmu]

@pranithk @raghavendrabhat ^^

[spalai]

Will send a fix for this.

[mykaul]

https://review.gluster.org/#/c/glusterfs/+/23452/ is an attempt to do that for _posix_xattr_get_set_from_backend()

[mykaul]

_get_list_xattr is fixed via https://review.gluster.org/#/c/glusterfs/+/23714/

[mykaul]

@amarts - I hope to complete all of this (or most) for Gluster 8.

https://review.gluster.org/#/c/glusterfs/+/23714/ is in the works.
https://review.gluster.org/#/c/glusterfs/+/23823/ is another one in the list for these, ready for review.

[mykaul]

@amarts - I hope to complete all of this (or most) for Gluster 8.

https://review.gluster.org/#/c/glusterfs/+/23714/ is in the works.
https://review.gluster.org/#/c/glusterfs/+/23823/ is another one in the list for these, ready for review.

I'm not going to pursue these for Gluster anymore - patches abandoned .