| Message ID | 1448358898-2350-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > detailed error info could show user about how to use it. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/pci-bridge/xio3130_downstream.c | 5 +++++ > 1 file changed, 5 insertions(+) > > When I learned to use root/upstream/downstream port like this: > qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx... > downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly. > > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index b3a6479..86b7970 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -23,6 +23,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/pcie.h" > #include "xio3130_downstream.h" > +#include "qemu/error-report.h" > > #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ > #define XIO3130_REVISION 0x1 > @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d) > pcie_chassis_create(s->chassis); > rc = pcie_chassis_add_slot(s); > if (rc < 0) { > + error_report("Slot number is reduplicate(chassis = %d, slot = %d). " > + "Slot #(default 0) is unique within a chassis, chassis #" > + "(default 0) is unique in the system. Please change chassis # " > + "or slot #.", s->chassis, s->slot); > goto err_pcie_cap; > } > pcie_cap_arifwd_init(d); > -- > 2.1.0 This covers one out of six error paths, and it doesn't work for QMP. Please convert the device to realize() instead. For an example, check out commit 28b07e7.
Hi Markus On 11/24/2015 06:08 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> detailed error info could show user about how to use it. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/pci-bridge/xio3130_downstream.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> When I learned to use root/upstream/downstream port like this: >> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx... >> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly. >> >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >> index b3a6479..86b7970 100644 >> --- a/hw/pci-bridge/xio3130_downstream.c >> +++ b/hw/pci-bridge/xio3130_downstream.c >> @@ -23,6 +23,7 @@ >> #include "hw/pci/msi.h" >> #include "hw/pci/pcie.h" >> #include "xio3130_downstream.h" >> +#include "qemu/error-report.h" >> >> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ >> #define XIO3130_REVISION 0x1 >> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d) >> pcie_chassis_create(s->chassis); >> rc = pcie_chassis_add_slot(s); >> if (rc < 0) { >> + error_report("Slot number is reduplicate(chassis = %d, slot = %d). " >> + "Slot #(default 0) is unique within a chassis, chassis #" >> + "(default 0) is unique in the system. Please change chassis # " >> + "or slot #.", s->chassis, s->slot); >> goto err_pcie_cap; >> } >> pcie_cap_arifwd_init(d); >> -- >> 2.1.0 > > This covers one out of six error paths, and it doesn't work for QMP. > > Please convert the device to realize() instead. For an example, check > out commit 28b07e7. > Thanks for your quick review:)OK, will do the investigation right away
Hi, Markus On 11/24/2015 06:08 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> detailed error info could show user about how to use it. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/pci-bridge/xio3130_downstream.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> When I learned to use root/upstream/downstream port like this: >> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx... >> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly. >> >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >> index b3a6479..86b7970 100644 >> --- a/hw/pci-bridge/xio3130_downstream.c >> +++ b/hw/pci-bridge/xio3130_downstream.c >> @@ -23,6 +23,7 @@ >> #include "hw/pci/msi.h" >> #include "hw/pci/pcie.h" >> #include "xio3130_downstream.h" >> +#include "qemu/error-report.h" >> >> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ >> #define XIO3130_REVISION 0x1 >> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d) >> pcie_chassis_create(s->chassis); >> rc = pcie_chassis_add_slot(s); >> if (rc < 0) { >> + error_report("Slot number is reduplicate(chassis = %d, slot = %d). " >> + "Slot #(default 0) is unique within a chassis, chassis #" >> + "(default 0) is unique in the system. Please change chassis # " >> + "or slot #.", s->chassis, s->slot); >> goto err_pcie_cap; >> } >> pcie_cap_arifwd_init(d); >> -- >> 2.1.0 > > This covers one out of six error paths, and it doesn't work for QMP. > I looked around in the code, and only find there are monitor & QMP may need be covered. Could give more hints about the "six error paths"? > Please convert the device to realize() instead. For an example, check > out commit 28b07e7. > and this will cover to output to the monitor, right?
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > Hi, Markus > > On 11/24/2015 06:08 PM, Markus Armbruster wrote: >> Cao jin <caoj.fnst@cn.fujitsu.com> writes: >> >>> detailed error info could show user about how to use it. >>> >>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >>> --- >>> hw/pci-bridge/xio3130_downstream.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> When I learned to use root/upstream/downstream port like this: >>> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx... >>> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly. >>> >>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >>> index b3a6479..86b7970 100644 >>> --- a/hw/pci-bridge/xio3130_downstream.c >>> +++ b/hw/pci-bridge/xio3130_downstream.c >>> @@ -23,6 +23,7 @@ >>> #include "hw/pci/msi.h" >>> #include "hw/pci/pcie.h" >>> #include "xio3130_downstream.h" >>> +#include "qemu/error-report.h" >>> >>> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ >>> #define XIO3130_REVISION 0x1 >>> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d) >>> pcie_chassis_create(s->chassis); >>> rc = pcie_chassis_add_slot(s); >>> if (rc < 0) { >>> + error_report("Slot number is reduplicate(chassis = %d, slot = %d). " >>> + "Slot #(default 0) is unique within a chassis, chassis #" >>> + "(default 0) is unique in the system. Please change chassis # " >>> + "or slot #.", s->chassis, s->slot); >>> goto err_pcie_cap; >>> } >>> pcie_cap_arifwd_init(d); >>> -- >>> 2.1.0 >> >> This covers one out of six error paths, and it doesn't work for QMP. >> > > I looked around in the code, and only find there are monitor & QMP may > need be covered. Could give more hints about the "six error paths"? > >> Please convert the device to realize() instead. For an example, check >> out commit 28b07e7. >> > > and this will cover to output to the monitor, right? The device still implements the old PCIDeviceClass.init() instead of the new .realize(). All devices need to be converted to .realize(). You can help by converting this one. .init() reports errors with error_report() and returns 0 on success, -1 on failure. .realize() passes errors to its callers via its errp argument. Commit 28b07e7 can serve as example of how to convert a device to realize(). There are many more. The ones I did usually have "Convert to realize()" in the commit message's first line. Regarding six error paths: I counted six places that return -1 directly or goto err or similar. Your patch adds a suitable error message to one of them. The other five need one, too. You'll have to examine the called functions to find out whether they report anything. If they do, they need to be first converted to pass an Error to their callers instead.
On 11/27/2015 10:22 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> Hi, Markus >> >> On 11/24/2015 06:08 PM, Markus Armbruster wrote: [...] >> >> and this will cover to output to the monitor, right? > > The device still implements the old PCIDeviceClass.init() instead of the > new .realize(). All devices need to be converted to .realize(). You > can help by converting this one. > > .init() reports errors with error_report() and returns 0 on success, -1 > on failure. > > .realize() passes errors to its callers via its errp argument. > After reading you commit 28b07e7, I am aware of this;) Thanks very much for your detailed explanation:) I was always curious about why there are 2 function(.init() & .realize()) for initializing device, now I guess I get it: it has just .init() at first, because of error report issue, the .realize() is added to replace it, in order to pass the error above. Do I understand it right ? If I understand it right, I see there are still devices initialization using .init() of DeviceClass & PCIDeviceClass, maybe I can help to convert some of these. > Commit 28b07e7 can serve as example of how to convert a device to > realize(). There are many more. The ones I did usually have "Convert > to realize()" in the commit message's first line. > > Regarding six error paths: I counted six places that return -1 directly > or goto err or similar. Your patch adds a suitable error message to one > of them. The other five need one, too. You'll have to examine the > called functions to find out whether they report anything. If they do, > they need to be first converted to pass an Error to their callers > instead. > It seems I have a misunderstanding about the "6 error paths":-[ I thought "error path" is a "error *reporting path*", like: fprintf the msg to stderr is a path, while output it via QMP is also a path. Because I did`t touch the code about -chardev/-mon/-monitor/-qmp before, so I spent 2 days reading the code:) Now I am cleared, Thanks you very much for the clarifying, Markus, I think this mistake won`t happen to me later;) And actually, I was already thinking add suitable error msg to each error path:) > . >
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > On 11/27/2015 10:22 PM, Markus Armbruster wrote: >> Cao jin <caoj.fnst@cn.fujitsu.com> writes: >> >>> Hi, Markus >>> >>> On 11/24/2015 06:08 PM, Markus Armbruster wrote: > [...] >>> >>> and this will cover to output to the monitor, right? >> >> The device still implements the old PCIDeviceClass.init() instead of the >> new .realize(). All devices need to be converted to .realize(). You >> can help by converting this one. >> >> .init() reports errors with error_report() and returns 0 on success, -1 >> on failure. >> >> .realize() passes errors to its callers via its errp argument. >> > > After reading you commit 28b07e7, I am aware of this;) Thanks very > much for your detailed explanation:) I was always curious about why > there are 2 function(.init() & .realize()) for initializing device, > now I guess I get it: it has just .init() at first, because of error > report issue, the .realize() is added to replace it, in order to pass > the error above. Do I understand it right ? Yes! > If I understand it right, I see there are still devices initialization > using .init() of DeviceClass & PCIDeviceClass, maybe I can help to > convert some of these. That would be helpful. There are probably easy cases where you only have to convert one device model at a time, and more complex cases, where you have to convert supporting functions from error_report() to error_set(), or convert device classes, not just individual devices. Best try an easy one first. >> Commit 28b07e7 can serve as example of how to convert a device to >> realize(). There are many more. The ones I did usually have "Convert >> to realize()" in the commit message's first line. >> > >> Regarding six error paths: I counted six places that return -1 directly >> or goto err or similar. Your patch adds a suitable error message to one >> of them. The other five need one, too. You'll have to examine the >> called functions to find out whether they report anything. If they do, >> they need to be first converted to pass an Error to their callers >> instead. >> > > It seems I have a misunderstanding about the "6 error paths":-[ I > thought "error path" is a "error *reporting path*", like: fprintf the > msg to stderr is a path, while output it via QMP is also a > path. Because I did`t touch the code about -chardev/-mon/-monitor/-qmp > before, so I spent 2 days reading the code:) Welcome to the club :) > Now I am cleared, Thanks you very much for the clarifying, Markus, I > think this mistake won`t happen to me later;) And actually, I was > already thinking add suitable error msg to each error path:) Glad I could help you!
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index b3a6479..86b7970 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -23,6 +23,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "xio3130_downstream.h" +#include "qemu/error-report.h" #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ #define XIO3130_REVISION 0x1 @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d) pcie_chassis_create(s->chassis); rc = pcie_chassis_add_slot(s); if (rc < 0) { + error_report("Slot number is reduplicate(chassis = %d, slot = %d). " + "Slot #(default 0) is unique within a chassis, chassis #" + "(default 0) is unique in the system. Please change chassis # " + "or slot #.", s->chassis, s->slot); goto err_pcie_cap; } pcie_cap_arifwd_init(d);
detailed error info could show user about how to use it. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/pci-bridge/xio3130_downstream.c | 5 +++++ 1 file changed, 5 insertions(+) When I learned to use root/upstream/downstream port like this: qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx... downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.