Re: [PATCH 1/3] Cleanup virSysinfoRead()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]