[Libvirt-cim] [PATCH 03/47] Add basic operations for reading data from xml node
Boris Fiuczynski
fiuczy at linux.vnet.ibm.com
Tue Oct 15 14:40:45 UTC 2013
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
On 10/08/2013 08:13 AM, Xu Wang wrote:
> Signed-off-by: Xu Wang <gesaint at linux.vnet.ibm.com>
> ---
> libxkutil/device_parsing.c | 200 +++++++++++++++++++++++++++++++++++++++++++-
> libxkutil/device_parsing.h | 21 +++++
> 2 files changed, 219 insertions(+), 2 deletions(-)
>
> diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
> index ea84d07..09dab97 100644
> --- a/libxkutil/device_parsing.c
> +++ b/libxkutil/device_parsing.c
> @@ -52,7 +52,7 @@
> /* Device parse function */
> typedef int (*dev_parse_func_t)(xmlNode *, struct virt_device **);
>
> -static void cleanup_node_of_others(struct others *others)
> +void cleanup_node_of_others(struct others *others)
> {
> if (others == NULL) {
> return;
> @@ -73,7 +73,7 @@ static void cleanup_node_of_others(struct others *others)
> free(others);
> }
>
> -static void cleanup_others(struct others *others)
> +void cleanup_others(struct others *others)
> {
> struct others *head = others;
>
> @@ -254,6 +254,202 @@ char *get_node_content(xmlNode *node)
> return buf;
> }
>
> +void print_others(struct others *head)
> +{
> + while (head) {
> + CU_DEBUG("---------------------------");
> + CU_DEBUG("- others name: %s", head->name);
> + CU_DEBUG("- others value: %s", head->value);
> + CU_DEBUG("- others type: %d", head->type);
> + CU_DEBUG("- others parent: %s", head->parent);
> + CU_DEBUG("---------------------------");
> + head = head->next;
> + }
> +}
Is the method above ever used?
> +
> +struct others *add_others(struct others *head,
> + xmlNode *node,
> + const xmlChar *name,
> + enum others_type type,
> + const xmlChar *parent)
> +{
> + struct others *new = NULL;
> +
> + new = calloc(1, sizeof(*new));
> + if (new == NULL) {
> + CU_DEBUG("calloc space failed.");
> + return NULL;
> + }
> +
> + new->name = strdup((char *)name);
> + if (parent) {
> + new->parent = strdup((char *)parent);
> + }
> + new->type = type;
> + if (type == TYPE_PROP) {
> + new->value = get_attr_value(node, (char *)name);
> + } else if (type == TYPE_NODE) {
> + new->value = get_node_content(node);
> + }
> + new->next = NULL;
> +
> + if (head == NULL) {
> + head = new;
> + } else {
> + new->next = head;
> + head = new;
> + }
Suggestion for the 8 lines above:
new->next = head;
head = new;
> +
> + return head;
> +}
> +
> +bool compare_parent(const char *a, const char *b)
> +{
> + if ((a == NULL) && (b == NULL)) {
> + return true;
> + }
> +
> + if ((a != NULL) && (b != NULL) &&
> + STREQ(a, b)) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> +char *fetch_from_others(struct others **head,
You have an add method, a fetch method and a seek method...
Fetch and seek methods both have the side effect to remove the entries
if found. To me the name should therefore be more like add/remove or
put/remove
> + char *name,
> + enum others_type type,
> + char *parent)
I am just wondering: Is the parent as a simple string contain uniqueness
that tags are placed correctly? Isn't in some cases the order of tags
within the domain important? would that be maintained? Looking at the
add and fetch/seek methods seems like it would reverse the order.
> +{
> + struct others *tmp = *head;
> + struct others *last = NULL;
> + char *value = NULL;
> +
> + while (tmp) {
> + if (STREQ(tmp->name, name) &&
> + compare_parent(tmp->parent, parent) &&
> + tmp->type == type) {
> + value = strdup(tmp->value);
> + if (tmp == *head) {
> + /* The first node is we needed. */
> + if ((*head)->next) {
> + /* There are more than two nodes in others. */
> + *head = (*head)->next;
> + } else {
> + *head = NULL;
> + }
Aren't the above 6 lines the same as:
*head = (*head)->next;
> + } else {
> + last->next = tmp->next;
> + }
> + cleanup_node_of_others(tmp);
> + return value;
> + }
> + last = tmp;
> + tmp = tmp->next;
> + }
> +
> + return NULL;
> +}
> +
> +static bool seek_in_others(struct others **head,
Naming? See above.
> + char *name,
> + enum others_type type,
> + char *parent)
> +{
> + struct others *tmp = *head;
> + struct others *last = NULL;
> +
> + while (tmp) {
> + if (STREQ(tmp->name, name) &&
> + compare_parent(tmp->parent, parent) &&
> + tmp->type == type) {
> + if (tmp == *head) {
> + if (tmp->next) {
> + *head = (*head)->next;
> + } else {
> + *head = NULL;
> + }
> + } else {
> + last->next = tmp->next;
> + }
> + cleanup_node_of_others(tmp);
> +
> + return true;
> + }
> + last = tmp;
> + tmp = tmp->next;
> + }
> +
> + return false;
> +}
To me it looks like this method could reuse fetch_from_others.
> +
> +struct others *combine_others(struct others *head1,
> + struct others *head2)
> +{
> + struct others *tail1 = head1;
> +
> + if (tail1 == NULL) {
> + return head2;
> + }
remove curly brackets?
Suggestion:
if (head2 == NULL)
return head1;
> +
> + while (tail1->next) {
> + tail1 = tail1->next;
> + }
remove curly brackets?
> +
> + tail1->next = head2;
> + return head1;
> +}
> +
> +static struct others *parse_data_to_others(xmlNode *node, const xmlChar *parent)
> +{
> + xmlNode *child = NULL;
> + xmlAttrPtr attrPtr = NULL;
> + struct others *head = NULL;
> + struct others *head2 = NULL;
> +
> + /* If name of node is "text", all operations will skip */
> + if (XSTREQ(node->name, "text")) {
> + return NULL;
> + }
remove curly brackets?
> +
> + head = add_others(head,
> + node,
> + node->name,
> + TYPE_NODE,
> + parent);
> +
> + if (head == NULL) {
> + goto err;
> + }
remove curly brackets?
> +
> + /* Get properties of node */
> + attrPtr = node->properties;
> + while (attrPtr) {
> + head = add_others(head,
> + node,
> + attrPtr->name,
> + TYPE_PROP,
> + node->name);
> + if (head == NULL) {
> + goto err;
> + }
> +
> + attrPtr = attrPtr->next;
> + }
> +
> + for (child = node->children; child != NULL; child = child->next) {
> + /* Recursion to restore child's properties or child if have */
> + head2 = parse_data_to_others(child, node->name);
> + head = combine_others(head, head2);
wouldn't it make things easier if instead of combining/flattening xml
subtags into a single list the xml structure would be replicated on the
others structure e.g. by adding a child pointer into the others structure?
> + }
> +
> + return head;
> +err:
> + CU_DEBUG("add_others failed.");
> + return NULL;
> +}
> +
> static int parse_fs_device(xmlNode *dnode, struct virt_device **vdevs)
> {
> struct virt_device *vdev = NULL;
> diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h
> index 027dd21..166f305 100644
> --- a/libxkutil/device_parsing.h
> +++ b/libxkutil/device_parsing.h
> @@ -258,6 +258,27 @@ int change_device(virDomainPtr dom, struct virt_device *dev);
>
> bool has_kvm_domain_type(xmlNodePtr node);
>
> +struct others *add_others(struct others *head,
> + xmlNode *node,
> + const xmlChar *name,
> + enum others_type type,
> + const xmlChar *parent);
> +
> +char *fetch_from_others(struct others **head,
> + char *name,
> + enum others_type type,
> + char *parent);
> +
> +void cleanup_node_of_others(struct others *others);
> +
> +bool compare_parent(const char *a, const char *b);
> +
> +void print_others(struct others *head);
> +
> +void cleanup_others(struct others *others);
> +
> +struct others *combine_others(struct others *head1, struct others *head2);
> +
> #define XSTREQ(x, y) (STREQ((char *)x, y))
> #define STRPROP(d, p, n) (d->p = get_node_content(n))
>
More information about the Libvirt-cim
mailing list