Re: [libvirt] [PATCH 07/16] hyperv: implement ability to send xml soap requests

On 08/09/2016 08:39 AM, Jason Miesionczek wrote: This should be a #define somewhere I think Again seems like some sort of #define for all the syntax Long line - try 80 chars Odd construct (*(tab_props+1)) why not tab_props[0]->name = "..." and tab_props[1]->name = "..." Those names could be constants too. More of the same with (*params) long line - put selector); on it's own line Long line 2.3.0 at the earliest FWIW: It "feels" as if the hunks above should be separate from the hunks below. I believe above depends on below and below could be any patch before above. Although I assume this is what Matthias was referring to in his comments from patch 2. If that's the case, then this could be closer to patch 2 if it's kept. Overkill on the extra lines... Could put the "%s", on the same line as ReportError Could go with "if (!(*xmlDocRoot = ws_xml_create_doc(...))) {" Likewise Similar if construct Likewise Long line - split args over multiple lines Use 'size_t' not 'int' for loop variables Typically we use loop variables of i, j, k... I think taking this method tricks the checker though. So I guess we're guaranteed this is a never ending loop by some other means? My question being - how is cimClasses constructed such that we know 'i' won't go beyond the number of elements in the array? BTW: Check out STREQ_NULLABLE s/)){/)) {/ - syntax-check s/)){/)) {/ - syntax-check Similar comment to above regarding how do we know 'y' will never go beyond the bounds such that cimTypesPtr[y] isn't valid? And of course STREQ_NULLABLE applies here as well break? so we never get to i++ once we find a matching className?, but if we don't find a matching className we can go for a while can't we... Consider "if (!(xmlNodeParam = ws*(...))) {" It's a long line, but should be split across 2 in any case. Just return -1 just return 0; Removing pointless cleanup since there is no cleanup if (!(options = wsmc*())) { s/client,root/client, root/ (from syntax-check) Brackets - syntax-check One less empty line brackets - syntax-check Long line above, consider combining if stmt. Repeats numerous times below Comments should be : /* * text line 1 * text line 2 */ and they should be in line with the indention as well as be less the 80 chars on the line s/Node,(/Node, (/ - syntax-check > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not add child to xml parent node")); > + goto cleanup; > + } > + > + if (xmlAddChild((xmlNodePtr) xmlNodeParam, xmlNodeAdrPtr) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not add child to xml parent node")); > + goto cleanup; > + } > + > + if (xmlAddChild((xmlNodePtr) xmlNodeParam, xmlNodeRefPtr) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not add child to xml parent node")); > + goto cleanup; > + > + cleanup: > + if (options != NULL) { > + wsmc_options_destroy(options); > + } > + if (filter != NULL) { > + filter_destroy(filter); > + } Both - brackets - syntax-check > + ws_xml_destroy_doc(xmlDocResponse); > + VIR_FREE(enumContext); > + VIR_FREE(query_string); > + > + return result; > +} > + > + > +/* Adding an Embedded Instance node to a parent node given in parameter */ > +static int > +hypervAddEmbeddedParam(properties_t *prop_t, int nbProps, const char *paramName, > + const char *instanceName, const char *classURI, WsXmlNodeH *parentNode) > +{ > + > + int result = -1; > + WsXmlNodeH xmlNodeInstance = NULL; > + WsXmlNodeH xmlNodeProperty = NULL; > + WsXmlNodeH xmlNodeParam = NULL; > + WsXmlNodeH xmlNodeArray = NULL; > + WsXmlDocH xmlDocTemp = NULL; > + WsXmlDocH xmlDocCdata = NULL; > + xmlBufferPtr xmlBufferNode = NULL; > + const xmlChar *xmlCharCdataContent = NULL; > + xmlNodePtr xmlNodeCdata = NULL; > + const char *type = NULL; > + bool isArray = false; > + int len = 0; > + int i = 0; > + > + /* Add child to given parent node*/ > + xmlNodeParam = ws_xml_add_child(*parentNode, classURI, paramName, NULL); > + if (xmlNodeParam == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not add child node to xmlNodeParam")); > + goto cleanup; > + } > + > + /* Create temp Xml doc */ > + /* INSTANCE node */ > + xmlDocTemp = ws_xml_create_doc(NULL, "INSTANCE"); > + if (xmlDocTemp == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not create temporary Xml doc")); > + goto cleanup; > + } > + > + xmlNodeInstance = xml_parser_get_root(xmlDocTemp); > + if (xmlNodeInstance == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get root of temporary Xml doc")); > + goto cleanup; > + } > + > + /* Add CLASSNAME node to INSTANCE node */ > + if (ws_xml_add_node_attr(xmlNodeInstance, NULL, "CLASSNAME", instanceName) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add attribute to node ")); > + goto cleanup; > + } > + > + /* Property nodes */ > + while (i < nbProps) { > + > + if (prop_t[i].name == NULL && prop_t[i].val == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get properties from array")); > + goto cleanup; > + } > + > + if (hypervGetPropType(instanceName, prop_t[i].name, &type, &isArray) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get properties from array")); > + goto cleanup; > + } > + > + xmlNodeProperty = ws_xml_add_child(xmlNodeInstance, NULL, > + isArray ? "PROPERTY.ARRAY" : "PROPERTY", NULL); > + if (xmlNodeProperty == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add child to node")); > + goto cleanup; > + } > + > + if (ws_xml_add_node_attr(xmlNodeProperty, NULL, "NAME", prop_t[i].name) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add attribute to node")); > + goto cleanup; > + } > + > + if (ws_xml_add_node_attr(xmlNodeProperty, NULL, "TYPE", type) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add attribute to node")); > + goto cleanup; > + } > + > + /* Add the node VALUE.ARRAY if the attribute is an array */ > + if (isArray) { > + xmlNodeArray = ws_xml_add_child(xmlNodeProperty, NULL, "VALUE.ARRAY", NULL); > + if (xmlNodeArray == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add child to node")); > + goto cleanup; > + } > + } > + > + if (ws_xml_add_child(isArray ? xmlNodeArray : xmlNodeProperty, NULL, "VALUE", prop_t[i].val) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not add child to node")); > + goto cleanup; > + } > + > + xmlNodeArray = NULL; > + xmlNodeProperty = NULL; > + i++; > + } > + > + /* Create CDATA node */ > + xmlBufferNode = xmlBufferCreate(); > + if (xmlNodeDump(xmlBufferNode, (xmlDocPtr) xmlDocTemp->parserDoc, (xmlNodePtr) xmlNodeInstance, 0, 0) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get root of temporary Xml doc")); > + goto cleanup; > + } > + > + len = xmlBufferLength(xmlBufferNode); > + xmlCharCdataContent = xmlBufferContent(xmlBufferNode); > + xmlNodeCdata = xmlNewCDataBlock((xmlDocPtr) xmlDocCdata, xmlCharCdataContent, len); > + if (xmlNodeCdata == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get root of temporary Xml doc")); > + goto cleanup; > + } > + /* Add CDATA node child to the root node of the main doc given */ > + if (xmlAddChild((xmlNodePtr) xmlNodeParam, xmlNodeCdata) == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", > + _("Could not get root of temporary Xml doc")); > + goto cleanup; > + > + cleanup: > + ws_xml_destroy_doc(xmlDocCdata); > + ws_xml_destroy_doc(xmlDocTemp); > + if (xmlBufferNode != NULL) > + xmlBufferFree(xmlBufferNode); Syntax-check says "if (xmlBufferNode != NULL)" is unecessary brackets - syntax-check Is there a reason for the usleep? Might be good to know why... That 100 ms microsleep may work on a non busy system, but what about a busy one? I'm just pointing an area ripe for some sort of timing issue. move the "else if" after the closing }; otherwise, syntax-check declares both sides of if-then-else-if need open/close braces > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Invocation of %s returned an error: %s (%d)"), > + "RequestStateChange", hypervReturnCodeToString(returnCode), > + returnCode); > + goto cleanup; > + > + cleanup: > + if (options != NULL) > + wsmc_options_destroy(options); > + if (response != NULL) > + ws_xml_destroy_doc(response); > + VIR_FREE(returnValue); > + VIR_FREE(instanceID); > + VIR_FREE(xpath_expr_string); > + hypervFreeObject(priv, (hypervObject *) concreteJob); > + virBufferFreeAndReset(&query); > + virBufferFreeAndReset(&xpath_expr_buff); > + > + return result; > +} > + > + > +/* Calls the invoke method by passing provided parameters as an XML tree */ > +int > +hypervInvokeMethod(hypervPrivate *priv, invokeXmlParam *param_t, int nbParameters, > + const char* methodName, const char* providerURI, const char *selector) > +{ > + int result = -1; > + WsXmlDocH doc = NULL; > + WsXmlNodeH methodNode = NULL; > + simpleParam *simple; > + eprParam *epr; > + embeddedParam *embedded; > + int i =0; s/int/size_t - syntax-check s/=0/= 0 - syntax-check s/,/, / syntax-check (3 instances) s/( i/(i - syntax-check s/name,simple/name, simple - syntax-check The "%s", could be on previous line > + _("Error during invocation action")); > + goto cleanup; > + > + cleanup: > + if (doc != NULL) > + ws_xml_destroy_doc(doc); > + > + return result; > +} > + > > > #include "hyperv_wmi.generated.c" > diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h > index 5fbbbac..c14d229 100644 > --- a/src/hyperv/hyperv_wmi.h > +++ b/src/hyperv/hyperv_wmi.h > @@ -29,7 +29,11 @@ > # include "hyperv_wmi_classes.h" > # include "openwsman.h" > > +#define ROOT_CIMV2 \ s/#define/# define/ - syntax-check s/#define/# define/ - syntax-check Using a <tab> instead of spaces - syntax-check Using <tab> instead of spaces <tab> All 3 have <tab> instead of spaces John