Skip to content

Commit

Permalink
Rename 'ret' variables passed from allocation to return.
Browse files Browse the repository at this point in the history 
I mentioned recently (in commit 9e7d4c5) message that I'm no
longer fond of the variable name 'ret', because it's used in two quite
different contexts: it's the return value from a subroutine you just
called (e.g. 'int ret = read(fd, buf, len);' and then check for error
or EOF), or it's the value you're preparing to return from the
_containing_ routine (maybe by assigning it a default value and then
conditionally modifying it, or by starting at NULL and reallocating,
or setting it just before using the 'goto out' cleanup idiom). In the
past I've occasionally made mistakes by forgetting which meaning the
variable had, or accidentally conflating both uses.

If all else fails, I now prefer 'retd' (short for 'returned') in the
former situation, and 'toret' (obviously, the value 'to return') in
the latter case. But even better is to pick a name that actually says
something more specific about what the thing actually is.

One particular bad habit throughout this codebase is to have a set of
functions that deal with some object type (say 'Foo'), all *but one*
of which take a 'Foo *foo' parameter, but the foo_new() function
starts with 'Foo *ret = snew(Foo)'. If all the rest of them think the
canonical name for the ambient Foo is 'foo', so should foo_new()!

So here's a no-brainer start on cutting down on the uses of 'ret': I
looked for all the cases where it was being assigned the result of an
allocation, and renamed the variable to be a description of the thing
being allocated. In the case of a new() function belonging to a
family, I picked the same name as the rest of the functions in its own
family, for consistency. In other cases I picked something sensible.

One case where it _does_ make sense not to use your usual name for the
variable type is when you're cloning an existing object. In that case,
_neither_ of the Foo objects involved should be called 'foo', because
it's ambiguous! They should be named so you can see which is which. In
the two cases I found here, I've called them 'orig' and 'copy'.

As in the previous refactoring, many thanks to clang-rename for the
help.
  • Loading branch information
@sgtatham
sgtatham committed Sep 14, 2022
1 parent 6cf6682 commit 20f818a
Show file tree
Hide file tree
Showing 19 changed files with 513 additions and 516 deletions.
8 changes: 4 additions & 4 deletions crypto/rsa.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,11 @@ char *rsa_ssh1_fingerprint(RSAKey *key)
*/
char **rsa_ssh1_fake_all_fingerprints(RSAKey *key)
{
char **ret = snewn(SSH_N_FPTYPES, char *);
char **fingerprints = snewn(SSH_N_FPTYPES, char *);
for (unsigned i = 0; i < SSH_N_FPTYPES; i++)
ret[i] = NULL;
ret[SSH_FPTYPE_MD5] = rsa_ssh1_fingerprint(key);
return ret;
fingerprints[i] = NULL;
fingerprints[SSH_FPTYPE_MD5] = rsa_ssh1_fingerprint(key);
return fingerprints;
}

/*
Expand Down
14 changes: 7 additions & 7 deletions dialog.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ int ctrl_path_compare(const char *p1, const char *p2)

struct controlbox *ctrl_new_box(void)
{
struct controlbox *ret = snew(struct controlbox);
struct controlbox *b = snew(struct controlbox);

ret->nctrlsets = ret->ctrlsetsize = 0;
ret->ctrlsets = NULL;
ret->nfrees = ret->freesize = 0;
ret->frees = NULL;
ret->freefuncs = NULL;
b->nctrlsets = b->ctrlsetsize = 0;
b->ctrlsets = NULL;
b->nfrees = b->freesize = 0;
b->frees = NULL;
b->freefuncs = NULL;

return ret;
return b;
}

void ctrl_free_box(struct controlbox *b)
Expand Down
116 changes: 58 additions & 58 deletions import.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,16 +339,16 @@ static void BinarySink_put_mp_ssh2_from_string(BinarySink *bs, ptrlen str)
static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
const char **errmsg_p)
{
struct openssh_pem_key *ret;
struct openssh_pem_key *key;
char *line = NULL;
const char *errmsg;
char *p;
bool headers_done;
char base64_bit[4];
int base64_chars = 0;

ret = snew(struct openssh_pem_key);
ret->keyblob = strbuf_new_nm();
key = snew(struct openssh_pem_key);
key->keyblob = strbuf_new_nm();

if (!(line = bsgetline(src))) {
errmsg = "unexpected end of file";
Expand All @@ -366,11 +366,11 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
* base64.
*/
if (!strcmp(line, "-----BEGIN RSA PRIVATE KEY-----")) {
ret->keytype = OP_RSA;
key->keytype = OP_RSA;
} else if (!strcmp(line, "-----BEGIN DSA PRIVATE KEY-----")) {
ret->keytype = OP_DSA;
key->keytype = OP_DSA;
} else if (!strcmp(line, "-----BEGIN EC PRIVATE KEY-----")) {
ret->keytype = OP_ECDSA;
key->keytype = OP_ECDSA;
} else if (!strcmp(line, "-----BEGIN OPENSSH PRIVATE KEY-----")) {
errmsg = "this is a new-style OpenSSH key";
goto error;
Expand All @@ -382,8 +382,8 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
sfree(line);
line = NULL;

ret->encrypted = false;
memset(ret->iv, 0, sizeof(ret->iv));
key->encrypted = false;
memset(key->iv, 0, sizeof(key->iv));

headers_done = false;
while (1) {
Expand Down Expand Up @@ -411,15 +411,15 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
}
p += 2;
if (!strcmp(p, "ENCRYPTED"))
ret->encrypted = true;
key->encrypted = true;
} else if (!strcmp(line, "DEK-Info")) {
int i, ivlen;

if (!strncmp(p, "DES-EDE3-CBC,", 13)) {
ret->encryption = OP_E_3DES;
key->encryption = OP_E_3DES;
ivlen = 8;
} else if (!strncmp(p, "AES-128-CBC,", 12)) {
ret->encryption = OP_E_AES;
key->encryption = OP_E_AES;
ivlen = 16;
} else {
errmsg = "unsupported cipher";
Expand All @@ -432,7 +432,7 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
errmsg = "expected more iv data in DEK-Info";
goto error;
}
ret->iv[i] = j;
key->iv[i] = j;
p += 2;
}
if (*p) {
Expand All @@ -459,7 +459,7 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
goto error;
}

put_data(ret->keyblob, out, len);
put_data(key->keyblob, out, len);

smemclr(out, sizeof(out));
}
Expand All @@ -472,20 +472,20 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
line = NULL;
}

if (!ret->keyblob || ret->keyblob->len == 0) {
if (!key->keyblob || key->keyblob->len == 0) {
errmsg = "key body not present";
goto error;
}

if (ret->encrypted && ret->keyblob->len % 8 != 0) {
if (key->encrypted && key->keyblob->len % 8 != 0) {
errmsg = "encrypted key blob is not a multiple of "
"cipher block size";
goto error;
}

smemclr(base64_bit, sizeof(base64_bit));
if (errmsg_p) *errmsg_p = NULL;
return ret;
return key;

error:
if (line) {
Expand All @@ -494,11 +494,11 @@ static struct openssh_pem_key *load_openssh_pem_key(BinarySource *src,
line = NULL;
}
smemclr(base64_bit, sizeof(base64_bit));
if (ret) {
if (ret->keyblob)
strbuf_free(ret->keyblob);
smemclr(ret, sizeof(*ret));
sfree(ret);
if (key) {
if (key->keyblob)
strbuf_free(key->keyblob);
smemclr(key, sizeof(*key));
sfree(key);
}
if (errmsg_p) *errmsg_p = errmsg;
return NULL;
Expand Down Expand Up @@ -1119,7 +1119,7 @@ struct openssh_new_key {
static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
const char **errmsg_p)
{
struct openssh_new_key *ret;
struct openssh_new_key *key;
char *line = NULL;
const char *errmsg;
char *p;
Expand All @@ -1129,8 +1129,8 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
ptrlen str;
unsigned key_index;

ret = snew(struct openssh_new_key);
ret->keyblob = strbuf_new_nm();
key = snew(struct openssh_new_key);
key->keyblob = strbuf_new_nm();

if (!(line = bsgetline(filesrc))) {
errmsg = "unexpected end of file";
Expand Down Expand Up @@ -1171,7 +1171,7 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
goto error;
}

put_data(ret->keyblob, out, len);
put_data(key->keyblob, out, len);

smemclr(out, sizeof(out));
}
Expand All @@ -1183,12 +1183,12 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
line = NULL;
}

if (ret->keyblob->len == 0) {
if (key->keyblob->len == 0) {
errmsg = "key body not present";
goto error;
}

BinarySource_BARE_INIT_PL(src, ptrlen_from_strbuf(ret->keyblob));
BinarySource_BARE_INIT_PL(src, ptrlen_from_strbuf(key->keyblob));

if (strcmp(get_asciz(src), "openssh-key-v1") != 0) {
errmsg = "new-style OpenSSH magic number missing\n";
Expand All @@ -1198,11 +1198,11 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
/* Cipher name */
str = get_string(src);
if (ptrlen_eq_string(str, "none")) {
ret->cipher = ON_E_NONE;
key->cipher = ON_E_NONE;
} else if (ptrlen_eq_string(str, "aes256-cbc")) {
ret->cipher = ON_E_AES256CBC;
key->cipher = ON_E_AES256CBC;
} else if (ptrlen_eq_string(str, "aes256-ctr")) {
ret->cipher = ON_E_AES256CTR;
key->cipher = ON_E_AES256CTR;
} else {
errmsg = get_err(src) ? "no cipher name found" :
"unrecognised cipher name\n";
Expand All @@ -1212,9 +1212,9 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
/* Key derivation function name */
str = get_string(src);
if (ptrlen_eq_string(str, "none")) {
ret->kdf = ON_K_NONE;
key->kdf = ON_K_NONE;
} else if (ptrlen_eq_string(str, "bcrypt")) {
ret->kdf = ON_K_BCRYPT;
key->kdf = ON_K_BCRYPT;
} else {
errmsg = get_err(src) ? "no kdf name found" :
"unrecognised kdf name\n";
Expand All @@ -1223,7 +1223,7 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,

/* KDF extra options */
str = get_string(src);
switch (ret->kdf) {
switch (key->kdf) {
case ON_K_NONE:
if (str.len != 0) {
errmsg = "expected empty options string for 'none' kdf";
Expand All @@ -1234,8 +1234,8 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
BinarySource opts[1];

BinarySource_BARE_INIT_PL(opts, str);
ret->kdfopts.bcrypt.salt = get_string(opts);
ret->kdfopts.bcrypt.rounds = get_uint32(opts);
key->kdfopts.bcrypt.salt = get_string(opts);
key->kdfopts.bcrypt.rounds = get_uint32(opts);

if (get_err(opts)) {
errmsg = "failed to parse bcrypt options string";
Expand All @@ -1257,23 +1257,23 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
* 'key_wanted' field is set to a value in the range [0,
* nkeys) by some mechanism.
*/
ret->nkeys = toint(get_uint32(src));
if (ret->nkeys != 1) {
key->nkeys = toint(get_uint32(src));
if (key->nkeys != 1) {
errmsg = get_err(src) ? "no key count found" :
"multiple keys in new-style OpenSSH key file not supported\n";
goto error;
}
ret->key_wanted = 0;
key->key_wanted = 0;

/* Read and ignore a string per public key. */
for (key_index = 0; key_index < ret->nkeys; key_index++)
for (key_index = 0; key_index < key->nkeys; key_index++)
str = get_string(src);

/*
* Now we expect a string containing the encrypted part of the
* key file.
*/
ret->private = get_string(src);
key->private = get_string(src);
if (get_err(src)) {
errmsg = "no private key container string found\n";
goto error;
Expand All @@ -1285,7 +1285,7 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,

smemclr(base64_bit, sizeof(base64_bit));
if (errmsg_p) *errmsg_p = NULL;
return ret;
return key;

error:
if (line) {
Expand All @@ -1294,10 +1294,10 @@ static struct openssh_new_key *load_openssh_new_key(BinarySource *filesrc,
line = NULL;
}
smemclr(base64_bit, sizeof(base64_bit));
if (ret) {
strbuf_free(ret->keyblob);
smemclr(ret, sizeof(*ret));
sfree(ret);
if (key) {
strbuf_free(key->keyblob);
smemclr(key, sizeof(*key));
sfree(key);
}
if (errmsg_p) *errmsg_p = errmsg;
return NULL;
Expand Down Expand Up @@ -1725,7 +1725,7 @@ struct sshcom_key {
static struct sshcom_key *load_sshcom_key(BinarySource *src,
const char **errmsg_p)
{
struct sshcom_key *ret;
struct sshcom_key *key;
char *line = NULL;
int hdrstart, len;
const char *errmsg;
Expand All @@ -1734,9 +1734,9 @@ static struct sshcom_key *load_sshcom_key(BinarySource *src,
char base64_bit[4];
int base64_chars = 0;

ret = snew(struct sshcom_key);
ret->comment[0] = '\0';
ret->keyblob = strbuf_new_nm();
key = snew(struct sshcom_key);
key->comment[0] = '\0';
key->keyblob = strbuf_new_nm();

if (!(line = bsgetline(src))) {
errmsg = "unexpected end of file";
Expand Down Expand Up @@ -1803,8 +1803,8 @@ static struct sshcom_key *load_sshcom_key(BinarySource *src,
p++;
p[strlen(p)-1] = '\0';
}
strncpy(ret->comment, p, sizeof(ret->comment));
ret->comment[sizeof(ret->comment)-1] = '\0';
strncpy(key->comment, p, sizeof(key->comment));
key->comment[sizeof(key->comment)-1] = '\0';
}
} else {
headers_done = true;
Expand All @@ -1824,7 +1824,7 @@ static struct sshcom_key *load_sshcom_key(BinarySource *src,
goto error;
}

put_data(ret->keyblob, out, len);
put_data(key->keyblob, out, len);
}

p++;
Expand All @@ -1835,24 +1835,24 @@ static struct sshcom_key *load_sshcom_key(BinarySource *src,
line = NULL;
}

if (ret->keyblob->len == 0) {
if (key->keyblob->len == 0) {
errmsg = "key body not present";
goto error;
}

if (errmsg_p) *errmsg_p = NULL;
return ret;
return key;

error:
if (line) {
smemclr(line, strlen(line));
sfree(line);
line = NULL;
}
if (ret) {
strbuf_free(ret->keyblob);
smemclr(ret, sizeof(*ret));
sfree(ret);
if (key) {
strbuf_free(key->keyblob);
smemclr(key, sizeof(*key));
sfree(key);
}
if (errmsg_p) *errmsg_p = errmsg;
return NULL;
Expand Down
Loading

0 comments on commit 20f818a

Please sign in to comment.