Hi, Daniel
Thank you for your review.
On Fri, 24 Jun 2011 15:21:36 +0100
"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Tue, Jun 21, 2011 at 01:19:17PM +0900, Minoru Usui wrote:
> > [Cleanup] Separate BIOSInfo and SystemInfo part from virSysinfoRead()
> >
> >
> > Signed-off-by: Minoru Usui <usui@xxxxxxxxxxxxxxxxx>
> > ---
> > src/util/sysinfo.c | 210 ++++++++++++++++++++++++++++++++--------------------
> > 1 files changed, 130 insertions(+), 80 deletions(-)
> >
> > diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c
> > index 40ec2e3..53122f7 100644
> > --- a/src/util/sysinfo.c
> > +++ b/src/util/sysinfo.c
> > @@ -96,37 +96,10 @@ virSysinfoRead(void) {
> >
> > #else /* !WIN32 */
> >
> > -virSysinfoDefPtr
> > -virSysinfoRead(void) {
> > - char *path, *cur, *eol, *base;
> > - virSysinfoDefPtr ret = NULL;
> > - char *outbuf = NULL;
> > - virCommandPtr cmd;
> > -
> > - path = virFindFileInPath(SYSINFO_SMBIOS_DECODER);
> > - if (path == NULL) {
> > - virSmbiosReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Failed to find path for %s binary"),
> > - SYSINFO_SMBIOS_DECODER);
> > - return NULL;
> > - }
> > -
> > - cmd = virCommandNewArgList(path, "-q", "-t", "0,1", NULL);
> > - VIR_FREE(path);
> > - virCommandSetOutputBuffer(cmd, &outbuf);
> > - if (virCommandRun(cmd, NULL) < 0) {
> > - virSmbiosReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Failed to execute command %s"),
> > - path);
> > - goto cleanup;
> > - }
> > -
> > - if (VIR_ALLOC(ret) < 0)
> > - goto no_memory;
> > -
> > - ret->type = VIR_SYSINFO_SMBIOS;
> > -
> > - base = outbuf;
> > +static char *
> > +parseBIOSInfo(char *base, virSysinfoDefPtr ret)
>
> Can you make sure this method, and all other ones added
> to this file have the "virSysinfo" prefix on their name
Because of static functions, I didn't add prefixes.
I will add prefixes to all functions.
> > +static char *
> > +parseSystemInfo(char *base, virSysinfoDefPtr ret)
> > +{
> > +static void
> > +BIOSInfoFormat(virSysinfoDefPtr def, const char *prefix, virBufferPtr buf)
>
>
> > +static void
> > +SystemInfoFormat(virSysinfoDefPtr def, const char *prefix, virBufferPtr buf)
>
>
> > +{
> > +/**
> > + * virSysinfoFormat:
> > + * @def: structure to convert to xml string
> > + * @prefix: string to prefix before each line of xml
> > + *
> > + * This returns the XML description of the sysinfo, or NULL after
> > + * generating an error message.
> > + */
> > +char *
> > +virSysinfoFormat(virSysinfoDefPtr def, const char *prefix)
> > +{
> > + const char *type = virSysinfoTypeToString(def->type);
> > + virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +
> > + if (!type) {
> > + virSmbiosReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("unexpected sysinfo type model %d"),
> > + def->type);
> > + return NULL;
> > }
> >
> > + virBufferAsprintf(&buf, "%s<sysinfo type='%s'>\n", prefix, type);
> > +
> > + BIOSInfoFormat(def, prefix, &buf);
> > + SystemInfoFormat(def, prefix, &buf);
> > +
> > virBufferAsprintf(&buf, "%s</sysinfo>\n", prefix);
> >
> > return virBufferContentAndReset(&buf);
>
> An existing bug here that needs fixing. There should be a call to
>
> if (virBufferError(&buf)) {
> virReportOOMError();
> return NULL;
> }
>
> just before the virBufferContentAndReset() usage.
OK.
I'll fix it.
>
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
Minoru Usui <usui@xxxxxxxxxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list