Merge branch 'refs/heads/upstream-HEAD' into repo-HEAD

Author: Delphix Engineering

Makefile.am

@@ -33,6 +33,7 @@ dist_noinst_DATA = \
 	Doxyfile \
 	NEWS \
 	README.md \
+	attributes.md \
 	objects.md \
 	threads.md
 

NEWS

@@ -1,5 +1,8 @@
 next
 ----
+  * Incompatible API changes:
+    - kdump_get_typed_attr(): parameters and type mismatch behaviour
+    - kdump_attr_ref_get(): result must be discarded
   * Support flattened ELF dump files.
   * Support partially rearranged makedumpfile split files.
   * Parse QEMU CPU state ELF notes.

attributes.md

@@ -0,0 +1,101 @@
+Attributes								{#attributes}
+==========
+
+All meta-information about a dump file is stored in attributes. All attributes
+form a tree-like dictionary, where every attribute is placed according to its
+path from the root node. The textual representation of this path, where path
+elements are delimited by dots, is called _attribute key_.
+
+Some attributes are present in every [kdump_ctx_t] object, although not
+necessarily set. They are called _global attributes_. Other attributes may be
+created by the library as needed, e.g. CPU registers. It is not possible to
+create a new attribute with an arbitrary path through the public API.
+
+Every attribute has a type and an optional value. The type and value can be
+queried with `kdump_get_attr`. If the attribute is unset (i.e. it does not
+have a value), this function returns `KDUMP_ERR_NODATA`. Attribute value can
+be set (or unset) with `kdump_set_attr`, but the type of an attribute cannot
+be changed.
+
+Lifetime
+--------
+
+Trivial types do not implement reference counting. The attribute value is
+always copied. Following types are trivial:
+- `KDUMP_NUMBER`
+- `KDUMP_ADDRESS`
+- `KDUMP_STRING`
+
+Other attribute types refer to objects, and reference counting is used to make
+sure that the object does not unexpectedly disappear if the attribute value is
+(directly or indirectly) changed. Following types are reference-counted:
+- `KDUMP_BITMAP`
+- `KDUMP_BLOB`
+
+For historical reasons, the simple attribute API (`kdump_get_attr`,
+`kdump_get_attr` and friends) does not increase the reference count of the
+returned data. User of this API must not modify the context object while
+making use of the returned attribute value. On the other hand, they don't have
+to do anything when they are finished.
+
+When a value is returned by the reference-based API (`kdump_attr_ref_get`),
+reference count is increased (or a new dynamic string is allocated). Users of
+the attribute reference API should release the underlying resources with a
+call to `kdump_attr_discard`.
+
+When a new value is set with `kdump_set_attr`, attributes with a trivial type
+make a copy of the new value, and attributes with a reference-counted type
+take ownership of the reference from the caller.
+
+Well-known Attributes
+---------------------
+
+Some attribute keys are available as macros. The intention is to spot mistakes
+earlier: A typo in a macro name is detected at build time, whereas a typo in a
+key string is not detected until run time.
+
+Implementation
+--------------
+
+Internally, there are two types of attributes:
+
+- global, and
+- allocated.
+
+Global attributes are allocated at dictionary initialization time, i.e. they
+exist in every dictionary object (albeit possibly unset). They can be looked
+up directly using a value from [enum global_keyidx] (identifiers with a `GKI_`
+prefix) as an index into the `global_attrs` fixed-size array in
+[struct attr_dict]. They can also be looked up through the hash table, like
+any other attribute, but that's less efficient, of course.
+
+The values of some global attributes are assumed to be always available and
+valid. These values are in fact stored in [struct kdump_shared] and can be
+accessed from here with zero overhead by inline helper functions. These
+attributes are called _static_ and are used for frequently accessed values,
+such as `arch.byte_order` or `file.pagemap`.
+
+It is possible to associate attribute operations with static attributes.
+However, the `revalidate` hook is not called when the value is accessed
+internally by the library using the `get_` or `sget_` helper functions.
+It is usually a bug to set `flags.invalid` for a static attribute.
+
+All non-global attributes are dynamically allocated by `new_attr`, and there
+is nothing special about them.
+
+Volatile and Persistent Attributes
+----------------------------------
+
+Volatile attributes are cleared when a file format probe returns an error.
+Persistent attributes are kept. All attributes set through the public API are
+persistent. This implementation detail allows to set attribute values while
+probing a file format and remove those attributes automatically if it turns
+out to be a different file type.
+
+The difference between volatile and persistent attributes is not visible in
+the public API.
+
+[kdump_ctx_t]: @ref kdump_ctx_t
+[struct kdump_shared]: @ref kdump_shared
+[enum global_keyidx]: @ref global_keyidx
+[struct attr_dict]: @ref attr_dict

examples/dumpattr.c

@@ -134,6 +134,7 @@ show_attr(kdump_ctx_t *ctx, kdump_attr_ref_t *ref, int indent, const char *key)
 		printf("<unknown>\n");
 	}
 
+	kdump_attr_discard(ctx, &attr);
 	return 0;
 }
 

include/libkdumpfile/kdumpfile.h.in

@@ -732,29 +732,38 @@ kdump_clear_attr(kdump_ctx_t *ctx, const char *key)
  * @param valp Value (filled on successful return).
  * @returns    Error status.
  *
- * Note that the caller does not hold a reference to the attribute, so
- * it is not generally safe to use this function in a multi-threaded
- * program, or across another library call which modifies the attribute
- * (explicitly or implicitly).
+ * This function is useful only when the caller can fully control the
+ * lifetime of the returned attribute value. In that case, the value
+ * is valid only as long as no changes are made to the containing dump
+ * file object.
+ *
+ * Note that attribute data (or the attribute hierarchy itself) may
+ * change as a side effect of another operation. Merely reading dump
+ * data or attributes will never invalidate the attribute value, i.e.
+ * it is safe to dereference pointers. However, the returned value may
+ * become stale, for example cache statistics.
+ *
+ * A more robust API is provided by @ref kdump_attr_ref_get.
  */
 kdump_status kdump_get_attr(kdump_ctx_t *ctx, const char *key,
 			    kdump_attr_t *valp);
 
-/**  Get a dump file attribute, checking its type.
+/**  Get a dump file attribute with a given type.
  * @param ctx  Dump file object.
  * @param key  Attribute key.
+ * @param type Expected attribute type.
  * @param valp Value (updated on return).
  * @returns    Error status.
  *
- * The @c type field in @c valp must be set to the expected attribute
- * type by the caller. It is an error if the attribute is of a different
- * type, but @c valp is updated to its value anyway.
+ * If the attribute is of a different type, this function returns
+ * KDUMP_ERR_INVALID and does not change @p valp.
  *
- * Note that the caller does not hold a reference to the attribute. See
- * the description of @ref kdump_get_attr for limitations.
+ * The caller must ensure the lifetime of the returned data is valid. See
+ * the description of @ref kdump_get_attr for more details.
  */
 kdump_status kdump_get_typed_attr(kdump_ctx_t *ctx, const char *key,
-				  kdump_attr_t *valp);
+				  kdump_attr_type_t type,
+				  kdump_attr_value_t *valp);
 
 /** Get a numeric attribute.
  *
@@ -766,14 +775,8 @@ kdump_status kdump_get_typed_attr(kdump_ctx_t *ctx, const char *key,
 static inline kdump_status
 kdump_get_number_attr(kdump_ctx_t *ctx, const char *key, kdump_num_t *num)
 {
-	kdump_attr_t attr;
-	kdump_status ret;
-
-	attr.type = KDUMP_NUMBER;
-	ret = kdump_get_typed_attr(ctx, key, &attr);
-	if (ret == KDUMP_OK)
-		*num = attr.val.number;
-	return ret;
+	return kdump_get_typed_attr(ctx, key, KDUMP_NUMBER,
+				    (kdump_attr_value_t *)num);
 }
 
 /** Get an address attribute.
@@ -786,14 +789,8 @@ kdump_get_number_attr(kdump_ctx_t *ctx, const char *key, kdump_num_t *num)
 static inline kdump_status
 kdump_get_address_attr(kdump_ctx_t *ctx, const char *key, kdump_addr_t *addr)
 {
-	kdump_attr_t attr;
-	kdump_status ret;
-
-	attr.type = KDUMP_ADDRESS;
-	ret = kdump_get_typed_attr(ctx, key, &attr);
-	if (ret == KDUMP_OK)
-		*addr = attr.val.address;
-	return ret;
+	return kdump_get_typed_attr(ctx, key, KDUMP_ADDRESS,
+				    (kdump_attr_value_t *)addr);
 }
 
 /** Get a string attribute.
@@ -803,20 +800,14 @@ kdump_get_address_attr(kdump_ctx_t *ctx, const char *key, kdump_addr_t *addr)
  * @param str[out]  Filled with the attribute value on successful return.
  * @returns         Error status.
  *
- * Note that the caller does not hold a reference to the string. See
- * the description of @ref kdump_get_attr for limitations.
+ * The caller must ensure the lifetime of the returned data is valid. See
+ * the description of @ref kdump_get_attr for more details.
  */
 static inline kdump_status
 kdump_get_string_attr(kdump_ctx_t *ctx, const char *key, const char **str)
 {
-	kdump_attr_t attr;
-	kdump_status ret;
-
-	attr.type = KDUMP_STRING;
-	ret = kdump_get_typed_attr(ctx, key, &attr);
-	if (ret == KDUMP_OK)
-		*str = attr.val.string;
-	return ret;
+	return kdump_get_typed_attr(ctx, key, KDUMP_STRING,
+				    (kdump_attr_value_t *)str);
 }
 
 /** Get a reference to an attribute
@@ -863,14 +854,42 @@ int kdump_attr_ref_isset(kdump_attr_ref_t *ref);
 /**  Get attribute data by reference.
  * @param      ctx   Dump file object.
  * @param[in]  ref   Attribute reference.
- * @param[out] valp  Attribute value (filled on successful return).
+ * @param[out] valp  Attribute value (filled on return).
+ * @returns          Error status.
  *
- * This works just like @ref kdump_get_attr, except that the attribute
- * is denoted by a reference rather than by its key path.
+ * This works similarly to @ref kdump_get_attr, except:
+ * - the attribute is denoted by a reference rather than by path,
+ * - the returned data stays valid even if changes are made to the dump
+ *   object.
+ *
+ * The returned value may be a dynamically allocated copy of the attribute
+ * value and/or it may hold an extra reference to the underlying objects.
+ * You should free up these resources with @ref kdump_attr_discard when
+ * the data is no longer needed.
+ *
+ * If the function returns an error, it is not necessary to discard the
+ * result. However, the type of @p valp is set to @c KDUMP_NIL, so it is
+ * always safe to call @ref kdump_attr_discard on the result. The goal of
+ * this feature is to simplify code flow in callers.
  */
 kdump_status kdump_attr_ref_get(kdump_ctx_t *ctx, const kdump_attr_ref_t *ref,
 				kdump_attr_t *valp);
 
+/** Discard attribute data value.
+ * @param ctx   Dump file object.
+ * @param attr  Attribute data.
+ *
+ * Perform any actions necessary to release resources by the attribute data.
+ * This function may decrement object references, free dynamically allocated
+ * memory, or even do nothing, depending on the data type.
+ *
+ * Call this function when the data returned by @ref kdump_attr_ref_get is no
+ * longer needed. Do *NOT* call this function on attribute data returned by
+ * @ref kdump_get_attr.
+ */
+void
+kdump_attr_discard(kdump_ctx_t *ctx, kdump_attr_t *attr);
+
 /**  Set attribute data by reference.
  * @param      ctx   Dump file object.
  * @param[in]  ref   Attribute reference.

python/kdumpfile.c

@@ -199,26 +199,37 @@ static PyObject *kdumpfile_read (PyObject *_self, PyObject *args, PyObject *kw)
 static PyObject *
 attr_new(kdumpfile_object *kdumpfile, kdump_attr_ref_t *ref, kdump_attr_t *attr)
 {
+	PyObject *obj = NULL;
+
 	if (attr->type != KDUMP_DIRECTORY)
 		kdump_attr_unref(kdumpfile->ctx, ref);
 
 	switch (attr->type) {
 		case KDUMP_NUMBER:
-			return PyLong_FromUnsignedLong(attr->val.number);
+			obj = PyLong_FromUnsignedLong(attr->val.number);
+			break;
 		case KDUMP_ADDRESS:
-			return PyLong_FromUnsignedLong(attr->val.address);
+			obj = PyLong_FromUnsignedLong(attr->val.address);
+			break;
 		case KDUMP_STRING:
-			return PyString_FromString(attr->val.string);
+			obj = PyString_FromString(attr->val.string);
+			break;
 		case KDUMP_DIRECTORY:
-			return attr_dir_new(kdumpfile, ref);
+			obj= attr_dir_new(kdumpfile, ref);
+			break;
 		case KDUMP_BITMAP:
-			return bmp_new(attr->val.bitmap);
+			obj = bmp_new(attr->val.bitmap);
+			break;
 		case KDUMP_BLOB:
-			return blob_new(attr->val.blob);
+			obj = blob_new(attr->val.blob);
+			break;
 		default:
 			PyErr_SetString(PyExc_RuntimeError, "Unhandled attr type");
-			return NULL;
 	}
+
+	kdump_attr_discard(kdumpfile->ctx, attr);
+
+	return obj;
 }
 
 PyDoc_STRVAR(get_addrxlat_ctx__doc__,
@@ -1604,14 +1615,15 @@ attr_iteritem_next(PyObject *_self)
 
 	result = PyTuple_New(2);
 	if (result == NULL)
-		return NULL;
+		goto err_attr;
 	key = PyString_FromString(self->iter.key);
 	if (!key)
 		goto err_result;
 	value = attr_new(self->kdumpfile, &self->iter.pos, &attr);
 	if (!value)
 		goto err_key;
 
+	kdump_attr_discard(self->kdumpfile->ctx, &attr);
 	PyTuple_SET_ITEM(result, 0, key);
 	PyTuple_SET_ITEM(result, 1, value);
 	return attr_iter_advance(self, result);
@@ -1620,6 +1632,8 @@ attr_iteritem_next(PyObject *_self)
 	Py_DECREF(key);
  err_result:
 	Py_DECREF(result);
+ err_attr:
+	kdump_attr_discard(self->kdumpfile->ctx, &attr);
 	return NULL;
 }
 

src/kdumpfile/Makefile.am

@@ -64,7 +64,7 @@ libkdumpfile_la_LIBADD = \
 	$(SNAPPY_LIBS)	\
 	$(ZSTD_LIBS)
 
-libkdumpfile_la_LDFLAGS = -version-info 11:0:1
+libkdumpfile_la_LDFLAGS = -version-info 12:0:0
 
 if HAVE_LD_VERSION_SCRIPT
 libkdumpfile_la_LDFLAGS += -Wl,--version-script=$(srcdir)/libkdumpfile.map