qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()


From: Cao jin
Subject: Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
Date: Wed, 23 Dec 2015 21:08:35 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

Hi Stefano,
    first of all, thanks for your quick response:)

On 12/23/2015 08:03 PM, Stefano Stabellini wrote:
On Wed, 23 Dec 2015, Cao jin wrote:
Signed-off-by: Cao jin <address@hidden>
---

Since the callchain is pretty deep & error path is very much too, so I made the
patch based on the principal: catch/report the most necessary error msg with
smallest modification.(So you can see I don`t change some functions to void,
despite they coule be)

Thanks Cao.

For consistency with the other functions, I think it would be better to
change all functions to return void or none.


Ok, I`ll select one style may with the smallest modification;)

Also it might be nice to split the patch in a series.


Yup, and the patches should be independent from each other?

The patch as is fails to build:

qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized 
in this func


really weird...last patch you remind me that it cannot compile, make me find that my computer didn`t install xen-devel package, then I installed it right away. But this time, it really can compile on my computer....anyway, I will check it out later.


  hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++-----------------
  hw/xen/xen-host-pci-device.h |  5 +--
  hw/xen/xen_pt.c              | 67 +++++++++++++++++++------------------
  hw/xen/xen_pt.h              |  5 +--
  hw/xen/xen_pt_config_init.c  | 47 +++++++++++++-------------
  hw/xen/xen_pt_graphics.c     |  6 ++--
  6 files changed, 118 insertions(+), 91 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..1ab6d97 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice 
*d,
          /* The output is truncated, or some other error was encountered */
          return -ENODEV;
      }
+
      return 0;
  }

I would prefer to keep stylistic changes separate, especially the ones
in functions which would be otherwise left unmodified. Maybe you could
move them to a separate patch?


I can do that.


[...]
+
      if (i != PCI_NUM_REGIONS) {
          /* Invalid format or input to short */
-        rc = -ENODEV;
+        error_setg(errp, "Invalid format or input to short");

                                                      ^too short

How about printing all the string in buf? like:
"Invalid format or input to short: %s", buf

for all the other comments below: will fix them up:)

      }

  out:
      close(fd);
-    return rc;
  }

[...]


--
Yours Sincerely,

Cao Jin





reply via email to

[Prev in Thread] Current Thread [Next in Thread]