(1) Long subject line. I'd suggest hyperv: add implementation for virConnectGetCapabilities Done by adding the Win32_ComputerSystemProduct class.
"Done" is not the appropriate word; adding Win32_ComputerSystemProduct class was just a requirement to make the implementation. (2) Dead 'if'; virObjectUnref(NULL) is safe Stands also for patch #20 with xmlopt. (3) Why do you need a forward declaration of a static function? Just because the new code was "grouped" near the end of the file. It's not a problem to move the block of functions hypervLookupHostSystemBiosUuid & hypervCapsInit just before function hypervConnectOpen and remove this forward declaration. (4) Is VIR_ERR_NO_DOMAIN really the right error to be using here? No; it's a mistake; VIR_ERR_INTERNAL_ERROR error should be used instead. (5) Libvirt style is two blank lines between functions, and function name starting in column 1 of a line separate from the return type: In the current version of the hyperv libvirt driver, functions are actually separated by three blank lines. (6) Might be nice to explain why this is commented out. /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 }); */ Has been commented because virCapabilitiesSetMacPrefix does not exist. Is that something that a generic function that has been removed from the libvirt? BTY. the Mac Prefix for hyperv is 00-15-0D (7) Only need two blank lines In the current version of the hyperv libvirt driver, functions are actually separated by three blank lines. All the other functions must be reviewed then... -----Message d'origine----- De : Eric Blake [mailto:ebl...@redhat.com] Envoyé : jeudi 9 octobre 2014 00:34 À : Yves Vinter; libvir-list@redhat.com Objet : Re: [libvirt] [PATCH 03/21] Added basic implementation for virConnectGetCapabilities (required Win32_ComputerSystemProduct class) On 10/08/2014 06:33 AM, Yves Vinter wrote: > From: yvinter <yves.vin...@bull.net> > Long subject line. I'd suggest: hyperv: add implementation for virConnectGetCapabilities Done by adding the Win32_ComputerSystemProduct class. > --- > src/hyperv/hyperv_driver.c | 112 > ++++++++++++++++++++++++++++++++++ > src/hyperv/hyperv_private.h | 2 + > src/hyperv/hyperv_wmi_generator.input | 12 ++++ > 3 files changed, 126 insertions(+) > > diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c > index f2017c3..dd56fb0 100644 > --- a/src/hyperv/hyperv_driver.c > +++ b/src/hyperv/hyperv_driver.c > @@ -58,12 +58,19 @@ hypervFreePrivate(hypervPrivate **priv) > wsmc_release((*priv)->client); > } > > + if ((*priv)->caps != NULL) > + virObjectUnref((*priv)->caps); Dead 'if'; virObjectUnref(NULL) is safe. > > +/* Forward declaration of hypervCapsInit */ static virCapsPtr > +hypervCapsInit(hypervPrivate *priv); Why do you need a forward declaration of a static function? If it's not recursive, just put the whole function body here (that may mean moving more than one function, to keep things topologically sorted; if you need to do code motion of existing functions, do it in a separate patch from where you add new code, to ease review). > > +/* Retrieves host system UUID */ > +static int > +hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char > +*uuid) { > + Win32_ComputerSystemProduct *computerSystem = NULL; > + virBuffer query = VIR_BUFFER_INITIALIZER; > + int result = -1; > + > + virBufferAddLit(&query, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT); > + > + if (hypervGetWin32ComputerSystemProductList(priv, &query, > &computerSystem) < 0) { > + goto cleanup; > + } > + > + if (computerSystem == NULL) { > + virReportError(VIR_ERR_NO_DOMAIN, Is VIR_ERR_NO_DOMAIN really the right error to be using here? > + _("Unable to get Win32_ComputerSystemProduct")); > + goto cleanup; > + } > + > + if (virUUIDParse(computerSystem->data->UUID, uuid) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not parse UUID from string '%s'"), > + computerSystem->data->UUID); > + goto cleanup; > + } > + > + result = 0; > + > + cleanup: > + hypervFreeObject(priv, (hypervObject *)computerSystem); > + virBufferFreeAndReset(&query); This virBufferFreeAndReset is not needed if you take my alternate patch for 1/21. > + > + return result; > +} > + > + > + > +static virCapsPtr hypervCapsInit(hypervPrivate *priv) Libvirt style is two blank lines between functions, and function name starting in column 1 of a line separate from the return type: static virCapsPtr hypervCapsInit(hypervPrivate *priv) > +{ > + virCapsPtr caps = NULL; > + virCapsGuestPtr guest = NULL; > + > + caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1); > + > + if (caps == NULL) { > + virReportOOMError(); > + return NULL; > + } > + > + /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, > + 0x0c, 0x29 }); */ Might be nice to explain why this is commented out. > +} > + > + > + > +static char* Only need two blank lines. > +hypervConnectGetCapabilities(virConnectPtr conn) { > + hypervPrivate *priv = conn->privateData; > + char *xml = virCapabilitiesFormatXML(priv->caps); virCapabilitiesFormatXML can only return NULL on error, and it already outputs a decent error message; also, error is not always due to OOM... > + > + if (xml == NULL) { > + virReportOOMError(); ...so this virReportOOMError() is bogus, because it overwrites the decent message. > + return NULL; > + } Once you realize that, you can simplify this function to: static char* hypervConnectGetCapabilities(virConnectPtr conn) { hypervPrivate *priv = conn->privateData; return virCapabilitiesFormatXML(priv->caps); } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list