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

Re: [libvirt] [PATCH v3 6/9] qemu_command: Honour chardev alias assignment with a function



On Wed, Jul 10, 2013 at 02:57:48PM +0200, Michal Privoznik wrote:
> On 03.07.2013 17:29, Daniel P. Berrange wrote:
> > On Tue, Jul 02, 2013 at 05:53:05PM +0200, Michal Privoznik wrote:
> >> The chardev alias assignment is going to be needed in a separate
> >> places, so it should be moved into a separate function rather
> >> than copying code randomly around.
> >> ---
> >>  src/qemu/qemu_command.c | 75 +++++++++++++++++++++++++++++++++++++++++++------
> >>  src/qemu/qemu_command.h |  3 ++
> >>  2 files changed, 70 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >> index ba93233..903839f 100644
> >> --- a/src/qemu/qemu_command.c
> >> +++ b/src/qemu/qemu_command.c
> >> @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
> >>      return 0;
> >>  }
> >>  
> >> +int
> >> +qemuAssignDeviceChrAlias(virDomainDefPtr def,
> >> +                         virDomainChrDefPtr chr,
> >> +                         ssize_t idx)
> >> +{
> >> +    const char *prefix = NULL;
> >> +    const char *prefix2 = NULL;
> >> +
> >> +    switch ((enum virDomainChrDeviceType) chr->deviceType) {
> >> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> >> +        prefix = "parallel";
> >> +        break;
> >> +
> >> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
> >> +        prefix = "serial";
> >> +        break;
> >> +
> >> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> >> +        prefix = "console";
> >> +        prefix2 = "serial";
> >> +        break;
> >> +
> >> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
> >> +        prefix = "channel";
> >> +        break;
> >> +
> >> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (idx == -1) {
> >> +        virDomainChrDefPtr **arrPtr;
> >> +        size_t *cntPtr;
> >> +        size_t i;
> >> +        idx = 0;
> >> +
> >> +        virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr);
> >> +
> >> +        for (i = 0; i < *cntPtr; i++) {
> >> +            int thisidx;
> >> +            if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) &&
> >> +                (prefix2 &&
> >> +                 (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) {
> >> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +                               _("Unable to determine device index for character device"));
> >> +                return -1;
> >> +            }
> >> +            if (thisidx >= idx)
> >> +                idx = thisidx + 1;
> >> +        }
> >> +    }
> > 
> > The commit message describes this as a simple refactoring, but this
> > if (idx== -1) {...}  is all new functionality compared to what is
> > being replaced. I'm not too sure that this logic is correct
> > either when dealing with <console> with a 'serialXX' alias.
> 
> This function just imitates other functions we've already:
> qemuAssignDeviceRedirdevAlias, qemuAssignDeviceHostdevAlias being
> examples. One can find even more.

Sure, that's not what I was complaining about though. The issue is that
you're mixing plain refactoring of code, with the inclusion of extra
functionality.  Nothing in this patch ever passes a value 'idx == -1'
so code to handle that scenario is not related to refactoring. It can
be introduced in whatever patch actually needs that code.

> And regarding <console> I've sent 2 patches, none of them was accepted.
> So I had to workaround the problem in my patch. What's the solution
> you're suggesting? Just to refresh our memory: for <console> the alias
> can be either 'consoleX' or 'serialX' depending if the daemon was
> restarted or not.

I described what I think needs fixing here:

https://www.redhat.com/archives/libvir-list/2013-July/msg00107.html

>   IIUC, this patch is intended to change things so that after libvirtd is
>   restarted, we get:
>
>      def->seriales[0]->info == "serial0"
>      def->consoles[0]->info == "console0"
>
>   but this is fixing the wrong thing. There is only one physical device
>   emulated in the guest, which is a serial port with id==serial0, and
>   this is reflected correctly in the XML we generate. Only the internal
>   struct is different.
>
>   So what needs fixing is the code which populated def->consoles[0]->info
>   with "console0" instead of the correct "serial0" string at VM startup.



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]