Refactor node_has_curve25519_onion_key() to use node_get_curve25519_onion_key()

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac easy in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac points::0.2 in Legacy / Trac priority::medium in Legacy / Trac refactor in Legacy / Trac resolution::implemented in Legacy / Trac review-group-28 in Legacy / Trac reviewer::teor in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::enhancement in Legacy / Trac labels

Remove the parent and spurious keywords.

Trac:
Keywords: single-onion, prop224, tor-hs, ipv6 deleted, N/A added
Parent: legacy/trac#23577 (moved) to N/A

could you give more information?

Trac:
Username: aruna1234

Replying to aruna1234:

could you give more information?

These functions have code that's almost identical. We can implement node_has… by calling node_get… and checking the result. The result is valid if it passes the existing check in node_has….

This might be a good ticket for you to focus on, because it is a small first-time change for you to learn C and learn our processes.

But I still didn't understand what does the bug demand?

Trac:
Username: aruna1234

Maybe this is not the best bug for you then,

Replying to teor:

Replying to aruna1234:

could you give more information?

These functions have code that's almost identical. We can implement node_has… by calling node_get… and checking the result. The result is valid if it passes the existing check in node_has….

This might be a good ticket for you to focus on, because it is a small first-time change for you to learn C and learn our processes.

Does that mean that we can replace all calls to function node_has... by calling node_get... without any ambiguity i.e if it passes all tests.

Trac:
Username: aruna1234

Replying to teor:

In legacy/trac#23577 (moved), we will create node_get_curve25519_onion_key(). We should use it to implement node_has_curve25519_onion_key(), so they give consistent results.

Or as this says, we should create node_get_curve25519_onion_key() and then call node_has_curve25519_onion_key() from this function so they give consistent results?

Trac:
Username: aruna1234

Both functions already exist. This ticket is about rewriting node_has… so that it uses node_get… and a comparison,

So, have thought of something:

All the calls to node_has.. will be replaced by node_get.. .Instead we will call node_get..() and check if the value that is returned is NULL or not. If it is not NULL then we'll call routerinfo_has_curve25519_onion_key() in routerlist.c, otherwise its zero.

Trac:
Username: aruna1234

Not quite, but you're very close.

When I read node_get_curve25519_onion_key() and node_has_curve25519_onion_key(), I see that they both have a similar pattern:

check if something exists, and return something
check if something else exists, and return something
check if something else exists, and return something

But we don't like duplicate code like this: it makes it easy to make mistakes.

So, what we want to do instead is:

leave node_get_curve25519_onion_key() as it is
make node_has_curve25519_onion_key() call node_get_curve25519_onion_key(), and check if the result is not NULL.

That way, we remove the duplicate code. And we only need to change one function, node_has_curve25519_onion_key().

Trac:
Username: aruna1234

0003-Refactored-node_has_curve25519_onion_key-to-use-node.patch

Trac:
Username: aruna1234

That's not quite what we wanted - we want node_has_curve25519_onion_key() to become a very simple function.

I'll walk you through the process.

We start with two functions that look like this:

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  if (!node)
    return 0;

  if (node->ri)
    return routerinfo_has_curve25519_onion_key(node->ri);
  else if (node->md)
    return microdesc_has_curve25519_onion_key(node->md);
  else
    return 0;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (node->ri)
    return node->ri->onion_curve25519_pkey;
  else if (node->md)
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

And we want to end up with node_has_curve25519_onion_key() calling node_get_curve25519_onion_key(), and checking if the result is NULL:

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  return node_get_curve25519_onion_key(node) != NULL;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (node->ri)
    return node->ri->onion_curve25519_pkey;
  else if (node->md)
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

But there were some good checks in node_has_curve25519_onion_key(), so we want to move them to node_get_curve25519_onion_key():

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  return node_get_curve25519_onion_key(node) != NULL;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (!node)
    return NULL;

  if (routerinfo_has_curve25519_onion_key(node->ri))
    return node->ri->onion_curve25519_pkey;
  else if (microdesc_has_curve25519_onion_key(node->md))
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

Can you test that the code works (we use the "make check" command) and then turn it into a patch?

Edit: oops, I mixed up the last two steps.

Trac:
Status: new to needs_revision

Trac:
Username: aruna1234

0004-node_has_curve25519_onion_key-is-refactored-and-dupl.patch

Trac:
Username: aruna1234

Did you compile this and run the unit tests using "make check"?

Please fix the indenting here - we indent code blocks, so this will confuse people:

   if(!node)
   return NULL;

I did run the make check command. It is failing at line 1638 that is the one with the first if condition that returns NULL. I tried running without the if condition, but it's still failing at the same line.

Trac:
Username: aruna1234

0005-Indentation-error-fixed.patch

Trac:
Username: aruna1234

Hi, please provide one patch file that includes all your changes.

Trac:
Username: aruna1234

0001-node_has_curve25519_onion_key-is-refactored-and-dupl.patch

Trac:
Username: aruna1234

Hi, please provide one patch file that applies to the Tor master branch. This one doesn't.

Hey! I added the changes in a new branch with only one commit-> [https://github.com/ArunaMaurya221B/tor/commit/01efbe3bd3d7e7d3bd372c04531ea2825ac9f706]

Trac:
Username: aruna1234

Thanks for creating a branch on master. That's much easier for us! This patch is ready to go, just one more final review before we merge.

I added a changes file to aruna1234's patch. Please see my branch Bug-23966 on https://github.com/teor2345/tor.git

Trac:
Reviewer: N/A to teor
Status: needs_revision to merge_ready
Type: defect to enhancement

Thanks for the patience. Took quite some time:)

Trac:
Username: aruna1234

Trac:
Keywords: N/A deleted, review-group-28 added

merging!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 1h 36m

mentioned in issue legacy/trac#23975 (moved)

moved from legacy/trac#23966 (moved)

added Feature First Contribution labels and removed 1 deleted label

added 1 deleted label and removed 1 deleted label

added Technical Debt label and removed 1 deleted label

Refactor node_has_curve25519_onion_key() to use node_get_curve25519_onion_key()

Child items ...

Activity