Bug 518139

Summary: Hot adding any unsupported nic model will cause qemu-kvm exit.
Product: Red Hat Enterprise Linux 5 Reporter: Miya Chen <michen>
Component: kvmAssignee: Luiz Capitulino <lcapitulino>
Status: CLOSED WONTFIX QA Contact: Lawrence Lim <llim>
Severity: medium Docs Contact:
Priority: low    
Version: 5.5CC: armbru, lcapitulino, lihuang, ovirt-maint, tburke, tools-bugs, virt-maint, ykaul
Target Milestone: rc   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-11-15 15:20:57 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Attachments:
Description Flags
Patch submitted upstream none

Description Miya Chen 2009-08-19 05:49:56 UTC
Description of problem:
Monitor command "pci_add pci_addr=auto nic model=None" cause qemu-kvm exit.

Version-Release number of selected component (if applicable):
83-106

How reproducible:
100%

Steps to Reproduce:
1.boot guest by:
/usr/libexec/qemu-kvm -no-hpet -rtc-td-hack -usbdevice tablet -drive file=RHEL-Server-5.4-64-virtio.qcow2,if=ide -cpu qemu64,+sse2 -m 4G -smp 4 -net nic,macaddr=20:20:20:90:22:32,model=virtio,vlan=1 -net tap,script=/etc/qemu-ifup,vlan=1 -vnc :2
2.execute "pci_add pci_addr=auto nic model=None" in qemu monitor.
  
Actual results:
check with gdb, got:
qemu: Unsupported NIC model: None
qemu: Supported NIC models: ne2k_pci,i82551,i82557b,i82559er,rtl8139,e1000,pcnet,virtio
[Thread 0x44fff940 (LWP 10796) exited]
[Thread 0x445fe940 (LWP 10789) exited]
[Thread 0x43bfd940 (LWP 10788) exited]
[Thread 0x41679940 (LWP 10786) exited]
[Thread 0x427fb940 (LWP 10785) exited]
[Thread 0x431fc940 (LWP 10787) exited]
[Thread 0x40aec940 (LWP 10791) exited]

Program exited with code 01.


Expected results:


Additional info:

Comment 1 Miya Chen 2009-08-19 05:58:36 UTC
related code:
void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
                              const char *default_model)
{
   int i, exit_status = 0;

   if (!nd->model)
       nd->model = strdup(default_model);

   if (strcmp(nd->model, "?") != 0) {
       for (i = 0 ; models[i]; i++)
           if (strcmp(nd->model, models[i]) == 0)
               return;

       fprintf(stderr, "qemu: Unsupported NIC model: %s\n", nd->model);
       exit_status = 1;
   }

   fprintf(stderr, "qemu: Supported NIC models: ");
   for (i = 0 ; models[i]; i++)
       fprintf(stderr, "%s%c", models[i], models[i+1] ? ',' : '\n');

   exit(exit_status);
}

Comment 2 Luiz Capitulino 2009-09-24 14:21:57 UTC
I've fixed this and submitted a patch upstream, as the bug also exists there.

I'll wait for upstream review and merge before submitting this for inclusion in RHEL, though, as I'm not sure my fix is the best one.

This might take some time, because maintainers are attending to Linux Plumbers.

Comment 3 Luiz Capitulino 2009-09-24 14:23:11 UTC
Created attachment 362504 [details]
Patch submitted upstream

Comment 4 Luiz Capitulino 2009-09-25 18:00:23 UTC
Markus has submitted a better fix upstream, but it's a 6 patches series:

http://lists.gnu.org/archive/html/qemu-devel/2009-09/msg01585.html

Markus, what do you think it's the best thing to do here? My simple fix or your series?

Comment 5 Markus Armbruster 2009-09-29 14:45:16 UTC
I guess that depends on how invasive a backport of my series turns out to be.

We need only patches 2-4, the other three are just cleanup.  Note that patch 2 fixes a closely related bug "with pci_add BAD-ADDR storage...", which is not covered by your simple fix.

Comment 6 Luiz Capitulino 2009-09-29 18:02:33 UTC
(In reply to comment #5)
> I guess that depends on how invasive a backport of my series turns out to be.

What's your opinion?

> We need only patches 2-4, the other three are just cleanup.  Note that patch 2
> fixes a closely related bug "with pci_add BAD-ADDR storage...", which is not
> covered by your simple fix.  

Hm, I thought you've said on irc that we didn't have this problem in the internal tree, maybe I misread?

Comment 7 Markus Armbruster 2009-09-29 18:21:04 UTC
Luiz, you're right, that bug exists only upstream.  I checked that, told you on IRC, and promptly forgot everything about it %-)

I don't have an opinion on a backport, because I haven't even looked at rejects, yet.

Comment 8 Luiz Capitulino 2009-09-29 22:14:50 UTC
Okay, the original issue is not so important anyway, but you be nice if you have a look at it as we already have the fix.

Comment 9 Markus Armbruster 2009-10-08 22:11:12 UTC
Backport looks feasible --- diffstat for an untested sketch is 15 files changed, 62 insertions(+), 40 deletions(-) --- but it depends on qemu_error().  I doubt it makes sense without it.

Comment 10 Luiz Capitulino 2009-10-09 13:02:40 UTC
Well, I'd say this diffstat is too big for a stable branch considering the bug is not that serious.

But it's not up to me to decide. :)

Comment 11 Markus Armbruster 2009-10-09 14:07:59 UTC
The number of files may look a bit scary, but it's actually harmless: most uses of pci_nic_init() get replaced by pci_nic_init_nofail(), which behaves like pci_nic_init() behaved before the patch.  Without that, we're down to 4 files changed, 50 insertions(+), 27 deletions(-).

Comment 12 Dor Laor 2009-11-15 15:20:57 UTC
It does not seems important and our monitor is consumed by libvirt/vdsm only