Re: Re: [PATCH 13/33] qemu: Drop qemuDomainSetSCSIControllerModel()

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

 



On Thu, Jan 25, 2024 at 10:16:55 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 11:17:46AM +0100, Peter Krempa wrote:
> > On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
> > > On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
> > > > @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> > > >      switch (cont->type) {
> > > >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > > >          /* Set the default SCSI controller model if not already set */
> > > > -        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
> > > > +        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > > > +
> > > > +        if (cont->model < 0)
> > > >              return -1;
> > >
> > > This is not future proof for the case when 'model' gets turned into a
> > > proper enum.
> >
> > Based on further patches, it seems to be a problem in more places.
> 
> I'm not sure I understand. Can you please explain what "a proper
> enum" means in this context? Is that connected to some condition that

If you turn the 'model' member from 'int' to the appropriate enum type
the '< 0' check will no longer work if the enum has no negative members.
This is due to the fact that the compiler then treats the enum as
'unsigned'

> could present itself in the current code, or as a consequence of a
> potential future refactoring? Based on the fact that you ultimately
> ACKed the patch, I assume the latter.

There are multiple other instances of exactly the same problem for this
enum and additionally this one has a native -1 member, so my point was
moot mostly.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[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]

  Powered by Linux