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

Re: [libvirt] [PATCH 1/4] Add an API for comparing the ABI of two guest configurations



On Tue, May 31, 2011 at 11:52:20AM +0200, Matthias Bolte wrote:
> 2011/5/27 Daniel P. Berrange <berrange redhat com>:
> > To allow a client app to pass in custom XML during migration
> > of a guest it is neccessary to ensure the guest ABI remains
> > unchanged. The virDomainDefCheckABIStablity method accepts
> > two virDomainDefPtr structs and compares everything in them
> > that could impact the guest machine ABI
> >
> > * src/conf/domain_conf.c, src/conf/domain_conf.h,
> >  src/libvirt_private.syms: Add virDomainDefCheckABIStablity
> > * src/conf/cpu_conf.c, src/conf/cpu_conf.h: Add virCPUDefIsEqual
> > * src/util/sysinfo.c, src/util/sysinfo.h: Add virSysinfoIsEqual
> > ---
> >  src/conf/cpu_conf.c      |   91 +++++
> >  src/conf/cpu_conf.h      |    9 +-
> >  src/conf/domain_conf.c   |  881 +++++++++++++++++++++++++++++++++++++++++++++-
> >  src/conf/domain_conf.h   |   11 +-
> >  src/libvirt_private.syms |    1 +
> >  src/util/sysinfo.c       |   60 +++-
> >  src/util/sysinfo.h       |   11 +-
> >  7 files changed, 1051 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> > index 98d598a..77d0976 100644
> > --- a/src/conf/cpu_conf.c
> > +++ b/src/conf/cpu_conf.c
> 
> > @@ -446,3 +449,91 @@ no_memory:
> >     virReportOOMError();
> >     return -1;
> >  }
> > +
> > +bool
> > +virCPUDefIsEqual(virCPUDefPtr src,
> > +                 virCPUDefPtr dst)
> > +{
> > +    bool identical = false;
> > +    int i;
> 
> > +    for (i = 0 ; i < src->nfeatures ; i++) {
> > +        if (STRNEQ(src->features[i].name, dst->features[i].name)) {
> > +            virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                              _("Target CPU feature %s does not match source %s"),
> > +                              dst->features[i].name, src->features[i].name);
> > +            goto cleanup;
> > +        }
> 
> I think you need to compare features[i].policy here too.
> 
> > +    }
> > +
> > +    identical = true;
> > +
> > +cleanup:
> > +    return identical;
> > +}
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 8ff155b..a9a4655 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> 
> > +static bool virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
> > +                                             virDomainNetDefPtr dst)
> > +{
> > +    bool identical = false;
> > +
> > +    if (memcmp(src->mac, dst->mac, VIR_MAC_BUFLEN) != 0) {
> > +        virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                             _("Target network card mac %02x:%02x:%02x:%02x:%02x:%02x"
> > +                               "does not match source %02x:%02x:%02x:%02x:%02x:%02x"),
> > +                             dst->mac[0], dst->mac[2], dst->mac[2],
> 
> Shouldn't this be dst->mac[0], dst->mac[1], dst->mac[2]?
> 
> > +                             dst->mac[3], dst->mac[4], dst->mac[5],
> > +                             src->mac[0], src->mac[2], src->mac[2],
> 
> Copy-n-paste error here.
> 
> > diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c
> > index d929073..70da532 100644
> > --- a/src/util/sysinfo.c
> > +++ b/src/util/sysinfo.c
> 
> > @@ -326,4 +328,56 @@ virSysinfoFormat(virSysinfoDefPtr def, const char *prefix)
> >     return virBufferContentAndReset(&buf);
> >  }
> >
> > +bool virSysinfoIsEqual(virSysinfoDefPtr src,
> > +                       virSysinfoDefPtr dst)
> > +{
> > +    bool identical = false;
> > +
> > +    if (!src && !dst)
> > +        return true;
> > +
> > +    if ((src && !dst) || (!src && dst)) {
> > +        virSmbiosReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                             _("Target sysinfo does not match source"));
> > +        goto cleanup;
> > +    }
> > +
> > +    if (src->type != dst->type) {
> > +        virSmbiosReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                             _("Target sysinfo %s does not match source %s"),
> > +                             virSysinfoTypeToString(dst->type),
> > +                             virSysinfoTypeToString(src->type));
> > +        goto cleanup;
> > +    }
> > +
> > +#define CHECK_FIELD(name, desc)                                         \
> 
> This needs to be '# define CHECK_FIELD(name, desc)' to please syntax-check.
> 
> > +    do {                                                                \
> > +        if (STRNEQ_NULLABLE(src->name, dst->name)) {                    \
> > +            virSmbiosReportError(VIR_ERR_CONFIG_UNSUPPORTED,            \
> > +                                 _("Target sysinfo " desc " %s does not match source %s"), \
> 
> Are you sure that this insertion of dest into the string in this way
> will work properly with gettext? I think
> 
>   _("Target sysinfo %s %s does not match source %s"), desc
> 
> will work better.
> 
> > +                                 src->name, dst->name);                 \
> 
> You used STRNEQ_NULLABLE as src->name, dst->name could be NULL, so you
> need to use
> 
>   NULLSTR(src->name), NULLSTR(dst->name)
> 
> here too.
> 
> > +        }                                                               \
> > +    } while (0)
> > +
> > +    CHECK_FIELD(bios_vendor, "BIOS vendor");
> > +    CHECK_FIELD(bios_version, "BIOS version");
> > +    CHECK_FIELD(bios_date, "BIOS date");
> > +    CHECK_FIELD(bios_release, "BIOS release");
> > +
> > +    CHECK_FIELD(system_manufacturer, "system vendor");
> > +    CHECK_FIELD(system_product, "system product");
> > +    CHECK_FIELD(system_version, "system version");
> > +    CHECK_FIELD(system_serial, "system serial");
> > +    CHECK_FIELD(system_uuid, "system uuid");
> > +    CHECK_FIELD(system_sku, "system sku");
> > +    CHECK_FIELD(system_family, "system family");
> > +
> > +#undef CHECK_FIELD
> 
> s/#undef CHECK_FIELD/# undef CHECK_FIELD/
> 
> > +
> > +    identical = true;
> > +
> > +cleanup:
> > +    return identical;
> > +}
> > +
> >  #endif /* !WIN32 */
> 
> ACK with those nits fixed.

Thanks, I've pushed this series with those fixes

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 :|


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