Confirmation needed: IPCC.xs behavior with empty response data

Confirmation needed: IPCC.xs behavior with empty response data

[Kefu Chai]

Hi PVE developers,

I'm working on the Rust port of pmxcfs and discovered what appears to be
an issue in the Perl XS binding (src/PVE/IPCC.xs) when handling IPC
operations that return empty response data. I'd like to confirm whether
this is expected behavior or a bug before proceeding with the fix.

## Context

The ipcc_send_rec() function in IPCC.xs has the following logic (lines 157-167):

```c
if (res_header->error < 0) {
    errno = -res_header->error;
    XSRETURN_UNDEF;
} else {
    errno = 0;
    if (dsize > 0) {
        RETVAL = newSVpv(ipcbuffer + sizeof(struct qb_ipc_response_header), dsize);
    } else {
        XSRETURN_UNDEF;  // Line 165: Returns undef when dsize == 0
    }
}
```

The issue is at line 165: when an IPC operation succeeds (error >= 0)
but returns no data (dsize == 0), the function returns undef with
errno=0. This is semantically incorrect - successful operations should
return a defined value (even if empty string).

## Affected Operations

This affects IPC operations that legitimately return empty responses on success:

### 1. SET_STATUS (op 4)

C implementation in server.c (lines ~144-161):
```c
} else if (request_id == CFS_IPC_SET_STATUS) {
    cfs_status_update_request_header_t *rh = (cfs_status_update_request_header_t *)data;
    int datasize = request_size - sizeof(cfs_status_update_request_header_t);

    if (ctx->read_only) {
        result = -EPERM;
    } else if (datasize < 0) {
        result = -EINVAL;
    } else {
        rh->name[sizeof(rh->name) - 1] = 0;
        char *dataptr = (char *)data + sizeof(cfs_status_update_request_header_t);
        result = cfs_status_set(rh->name, dataptr, datasize);
        // Note: result = 0 on success, no data appended to outbuf
    }
}
```

### 2. LOG_CLUSTER_MSG (op 7)

C implementation in server.c (lines ~194-223):
```c
} else if (request_id == CFS_IPC_LOG_CLUSTER_MSG) {
    cfs_log_msg_request_header_t *rh = (cfs_log_msg_request_header_t *)data;
    int datasize = request_size - G_STRUCT_OFFSET(cfs_log_msg_request_header_t, data);
    int msg_len = datasize - rh->ident_len - rh->tag_len;

    if (ctx->read_only) {
        result = -EPERM;
    } else if (msg_len < 1) {
        result = -EINVAL;
    } else {
        char *msg = rh->data;
        if ((msg[rh->ident_len - 1] == 0) && ...) {
            char *ident = msg;
            char *tag = msg + rh->ident_len;
            msg = msg + rh->ident_len + rh->tag_len;

            time_t ctime = time(NULL);
            clog_entry_t *entry = (clog_entry_t *)alloca(CLOG_MAX_ENTRY_SIZE);
            if (clog_pack(entry, cfs.nodename, ident, tag, ctx->client_pid,
                         ctime, rh->priority, msg)) {
                cfs_cluster_log(entry);
            }
            result = 0;  // Success, no data appended to outbuf
        } else {
            result = -EINVAL;
        }
    }
}
```

Response sending (server.c):
```c
int resp_data_len = resp ? outbuf->len : 0;  // resp_data_len = 0 for empty response

res_header.error = result;  // error = 0 for success
res_header.size = sizeof(res_header) + resp_data_len;

// Send response with error=0, dsize=0
qb_ipcs_response_sendv(c, iov, iov_len);
```

## Why Existing Code Doesn't Break

The production code in PVE/Cluster.pm has a wrapper that "accidentally"
works around this bug:

```perl
my $ipcc_send_rec = sub {
    my ($msgid, $data) = @_;
    my $res = PVE::IPCC::ipcc_send_rec($msgid, $data);

    # Only die if result is undef AND errno != 0
    die "ipcc_send_rec[$msgid] failed: $!\n" if !defined($res) && ($! != 0);

    return $res;
};
```

When ipcc_send_rec returns undef with errno=0:
- !defined($res) = TRUE
- $! != 0 = FALSE (errno is 0)
- Combined: TRUE && FALSE = FALSE → no exception thrown
- The function silently returns undef, which callers ignore:

```perl
eval { $ipcc_update_status->("kv/$key", $data) };
warn $@ if $@;  # Only checks exception, not return value
```

So the bug is masked by:
1. The wrapper's "accidental" acceptance of undef with errno=0
2. Production code ignoring return values and only checking exceptions

## Test Verification

However, proper test code that checks return values DOES fail:

```perl
my $result = PVE::IPCC::ipcc_send_rec($op_code, $request);
my $errno = $! + 0;

if (defined $result) {
    test_success("");
} else {
    if ($errno == 1) {  # EPERM
        test_success("Permission denied");
    } else {
        test_failure("Unexpected errno: $errno");  # FAILS HERE with errno=0
    }
}
```

Testing with C pmxcfs:
```bash
/usr/bin/pmxcfs -f &
perl tests/ipc/perl/set-status.pl test-key 'test data'
# Output: FAILED: Unexpected errno: 0
```

## Potential Fixes

Option 1: Fix IPCC.xs (recommended)
```c
} else {
    errno = 0;
    if (dsize > 0) {
        RETVAL = newSVpv(ipcbuffer + sizeof(struct qb_ipc_response_header), dsize);
    } else {
        RETVAL = newSVpv("", 0);  // Return empty string instead of undef
    }
}
```

This makes the semantics correct: success returns defined value, failure returns undef.

**Option 2: Workaround in pmxcfs**
Return a single space character instead of empty response. This is what I've temporarily implemented in the Rust port.

## Questions

1. Should we fix IPCC.xs to return empty string for successful
   operations with no data? This makes the API more correct and consistent.
2. The wrapper in Cluster.pm would continue to work fine with this fix
   (empty string is still defined). 3. Are there any other callers outside
   pve-cluster that might depend on the current behavior? 4. For the Rust
   port: should we maintain bug-for-bug compatibility, or fix it properly?

I lean toward fixing IPCC.xs since the current behavior is semantically
wrong (success should not return undef), and the fix is unlikely to
break existing code since the wrapper already handles both cases.

Thanks for your guidance!

Best regards,
Kefu Chai