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

Re: [libvirt] [PATCH 4/6] conf: Move virDomainPCIAddressAsString() to device_conf



On Fri, Aug 31, 2018 at 04:25:25PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-31 at 15:05 +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 31, 2018 at 04:00:45PM +0200, Andrea Bolognani wrote:
> > > Unfortunately, even after this change functions
> > > handling virPCIDeviceAddress are split pretty much
> > > evenly between conf/device_conf and utils/virpci:
> > > ideally everything would be moved to the former,
> > > including the struct declaration itself, and all the
> > > names would be changed to be consistent with the rest
> > > of the virDomainDevice*Address, but that's a cleanup
> > > for another day.
> [...]
> > > +char *
> > > +virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> > > +{
> > > +    char *str;
> > > +
> > > +    ignore_value(virAsprintf(&str, "%.4x:%.2x:%.2x.%.1x",
> > > +                             addr->domain,
> > > +                             addr->bus,
> > > +                             addr->slot,
> > > +                             addr->function));
> > > +    return str;
> > > +}
> > 
> > This should really be in src/util/virpci.{c,h}, since that's where the
> > virPCIDeviceAddressPtr struct is declared. There's nothing XML related
> > about this string conversion, so doesn't belong in src/conf at all.
> 
> See the commit message :)
> 
> I can move this to util/virpci instead of conf/device_conf for
> the time being if you prefer, but at some point we're going to
> have to pick a place for *all* functions related to PCI addresses
> and conf/device_conf is the most sensible option IMHO, seeing as
> all other address types and related functions are defined there.

The device_conf file is dealing with domain device addresses. The
virPCIDeviceAddressPtr struct is independant of domain device
addresses. It is used across domain, node device, network and
storage drivers.

For that matter device_conf should probably be domain_device_conf
as a name.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


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