[cdatapack] fix error handling for getdeltachain

Summary:
1. `delta_chain_t` is a pretty small structure, so I'm modifying the code to return this struct on the stack.  This removes the allocation and the error checking needs for it.
2. add a `code` field to indicate the status.

Test Plan:
make local.

unfortunately, we don't have any good tooling to introduce memory allocation failures, so this is mostly visual.

Reviewers: #fastmanifest, durham

Reviewed By: durham

Subscribers: durham, mitrandir, mjpieters

Differential Revision: https://phabricator.intern.facebook.com/D3786447

Tasks: 12932864

Signature: t1:3786447:1472514697:c09956480b8a5456ffc3f2f955d6a2d2e816769f

remotefilelog/cdatapack/cdatapack.c

@@ -440,20 +440,24 @@ const uint8_t *getdeltachainlink(
   return ptr;
 }
 
-delta_chain_t *getdeltachain(
+delta_chain_t getdeltachain(
     const datapack_handle_t *handle,
     const uint8_t node[NODE_SZ]) {
   pack_chain_t *pack_chain = build_pack_chain(handle, node);
 
   if (pack_chain == NULL) {
-    return NULL;
+    // TODO: build_pack_chain needs to return a more detailed error code.
+    return (delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND };
   }
 
-  delta_chain_t *delta_chain = malloc(sizeof(delta_chain_t));
+  delta_chain_t result;
-  delta_chain->links_count = pack_chain->links_idx;
+  result.links_count = pack_chain->links_idx;
-  delta_chain->delta_chain_links = malloc(
+  result.delta_chain_links = malloc(
-      delta_chain->links_count * sizeof(delta_chain_link_t));
+      result.links_count * sizeof(delta_chain_link_t));
-  // TODO: error handling
+  if (result.delta_chain_links == NULL) {
+    result.code = GET_DELTA_CHAIN_OOM;
+    goto error_cleanup;
+  }
 
 
   for (int ix = 0; ix < pack_chain->links_idx; ix ++) {
@@ -462,12 +466,13 @@ delta_chain_t *getdeltachain(
     const uint8_t *end = ptr +
         pack_chain->pack_chain_links[ix].data_sz;
 
-    delta_chain_link_t *link = &delta_chain->delta_chain_links[ix];
+    delta_chain_link_t *link = &result.delta_chain_links[ix];
 
     ptr = getdeltachainlink(ptr, link);
 
     if (ptr > end) {
-      abort();
+      result.code = GET_DELTA_CHAIN_CORRUPT;
+      goto error_cleanup;
     }
   }
 
@@ -480,19 +485,25 @@ delta_chain_t *getdeltachain(
     platform_madvise_away((void *) ptr, end - ptr);
   }
 
+  result.code = GET_DELTA_CHAIN_OK;
+
+  goto cleanup;
+
+error_cleanup:
+  free(result.delta_chain_links);
+
+cleanup:
+
   // free pack chain.
-  if (pack_chain != NULL) {
   free(pack_chain->pack_chain_links);
   free(pack_chain);
+
+  return result;
 }
 
-  return delta_chain;
+void freedeltachain(delta_chain_t chain) {
+  for (size_t ix = 0; ix < chain.links_count; ix ++) {
+    free((void *) chain.delta_chain_links[ix].delta);
   }
-
+  free(chain.delta_chain_links);
-void freedeltachain(delta_chain_t *chain) {
-  for (size_t ix = 0; ix < chain->links_count; ix ++) {
-    free((void *) chain->delta_chain_links[ix].delta);
-  }
-  free(chain->delta_chain_links);
-  free(chain);
 }

remotefilelog/cdatapack/cdatapack.h

@@ -75,10 +75,18 @@ typedef struct _delta_chain_link_t {
   const uint8_t *delta;
 } delta_chain_link_t;
 
+typedef enum {
+  GET_DELTA_CHAIN_OK,
+  GET_DELTA_CHAIN_OOM,
+  GET_DELTA_CHAIN_NOT_FOUND,
+  GET_DELTA_CHAIN_CORRUPT,
+} get_delta_chain_code_t;
+
 /**
  * This represents an entire delta chain.
  */
 typedef struct _delta_chain_t {
+  get_delta_chain_code_t code;
   delta_chain_link_t *delta_chain_links;
   size_t links_count;
 } delta_chain_t;
@@ -110,11 +118,11 @@ bool find(
 /**
  * Retrieves a delta chain for a given node.
  */
-extern delta_chain_t *getdeltachain(
+extern delta_chain_t getdeltachain(
     const datapack_handle_t *handle,
     const uint8_t node[NODE_SZ]);
 
-extern void freedeltachain(delta_chain_t *chain);
+extern void freedeltachain(delta_chain_t chain);
 
 // this should really be private, but we need it for the cdatapack_dump tool.
 extern const uint8_t *getdeltachainlink(

remotefilelog/cdatapack/cdatapack_get.c

@@ -41,7 +41,12 @@ int main(int argc, char *argv[]) {
 
   unhexlify(argv[2], NODE_SZ * 2, binhash);
 
-  delta_chain_t *chain = getdeltachain(handle, binhash);
+  delta_chain_t chain = getdeltachain(handle, binhash);
+
+  if (chain.code != GET_DELTA_CHAIN_OK) {
+    fprintf(stderr, "error retrieving delta chain (code=%d)\n", chain.code);
+    return 1;
+  }
 
   const char *last_filename = NULL;
   uint16_t last_filename_sz = 0;
@@ -52,8 +57,8 @@ int main(int argc, char *argv[]) {
   char deltabase_buffer[NODE_SZ * 2];
   char sha_buffer[NODE_SZ * 2];
 
-  for (int ix = 0; ix < chain->links_count; ix ++) {
+  for (int ix = 0; ix < chain.links_count; ix ++) {
-    delta_chain_link_t *link = &chain->delta_chain_links[ix];
+    delta_chain_link_t *link = &chain.delta_chain_links[ix];
 
     SHA_CTX ctx;
     SHA1_Init(&ctx);

remotefilelog/cdatapack/py-cdatapack.c

@@ -198,19 +198,30 @@ static PyObject *cdatapack_getdeltachain(
     return NULL;
   }
 
-  delta_chain_t *chain = getdeltachain(self->handle, (const uint8_t *) node);
+  delta_chain_t chain = getdeltachain(self->handle, (const uint8_t *) node);
-  if (chain == NULL) {
+  if (chain.code == GET_DELTA_CHAIN_OOM) {
+    PyErr_NoMemory();
+    return NULL;
+  } else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {
     Py_RETURN_NONE;
+  } else if (chain.code != GET_DELTA_CHAIN_OK) {
+    // corrupt, etc.
+    PyErr_Format(
+        PyExc_ValueError,
+        "unknown error reading node %.*s", (int) node_sz, node);
+    return NULL;
+  }
+  PyObject *result = PyList_New(chain.links_count);
+  if (result == NULL) {
+    goto err_cleanup;
   }
-  PyObject *result = PyList_New(chain->links_count);
-  // TODO: error checking
 
-  for (int ix = 0; ix < chain->links_count; ix ++) {
+  for (int ix = 0; ix < chain.links_count; ix ++) {
     PyObject *tuple = NULL;
     PyObject *name = NULL, *retnode = NULL, *deltabasenode = NULL, *delta =
         NULL;
 
-    delta_chain_link_t *link = &chain->delta_chain_links[ix];
+    delta_chain_link_t *link = &chain.delta_chain_links[ix];
 
     name = PyString_FromStringAndSize(link->filename, link->filename_sz);
     retnode = PyString_FromStringAndSize((const char *) link->node, NODE_SZ);