Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2016-01-11 Thread Cao jin 



On 01/11/2016 10:32 PM, Markus Armbruster wrote:

Cao jin  writes:

[...]




Ok...will change the error msg to strerror(errno)


There's also error_setg_file_open(), not sure it fits here, but check it
out.



Thanks for reminding. Gerd Hoffmann has whole 
patchset(http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00245.html) 
which covers this, so maybe this doesn`t need to be updated.


--
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2016-01-11 Thread Markus Armbruster 
Cao jin  writes:

> On 12/19/2015 05:18 AM, Markus Armbruster wrote:
>> One short remark in addition to Eduardo's review.
>>
>> Eduardo Habkost  writes:
>>
> [...]

   config_fd = open(path, O_RDWR);
   if (config_fd < 0) {
 +error_setg(errp, "No such device");
   return -ENODEV;
   }
>>
>> Can we come up with nicer error messages?
>>
>
> Ok...will change the error msg to strerror(errno)

There's also error_setg_file_open(), not sure it fits here, but check it
out.

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2015-12-20 Thread Cao jin 



On 12/19/2015 05:18 AM, Markus Armbruster wrote:

One short remark in addition to Eduardo's review.

Eduardo Habkost  writes:


[...]


  config_fd = open(path, O_RDWR);
  if (config_fd < 0) {
+error_setg(errp, "No such device");
  return -ENODEV;
  }


Can we come up with nicer error messages?



Ok...will change the error msg to strerror(errno)


[...]


.



--
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2015-12-20 Thread Cao jin 



On 12/19/2015 02:37 AM, Eduardo Habkost wrote:

On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:

Signed-off-by: Cao jin 
---
  hw/pci-host/piix.c | 16 +---
  1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..e3840f0 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
  {0xa8, 4},  /* SNB: base of GTT stolen memory */
  };

-static int host_pci_config_read(int pos, int len, uint32_t val)
+static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)


You don't need the return value anymore, if you report errors
through the errp parameter. The function can be void, now.


Ok, will modify that in next version, as many people mentioned in the 
other patch...





  {
  char path[PATH_MAX];
  int config_fd;
@@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
uint32_t val)
  int ret = 0;

  if (rc >= size || rc < 0) {
+error_setg(errp, "No such device");
  return -ENODEV;
  }

  config_fd = open(path, O_RDWR);
  if (config_fd < 0) {
+error_setg(errp, "No such device");
  return -ENODEV;
  }

  if (lseek(config_fd, pos, SEEK_SET) != pos) {
+error_setg(errp, "lseek err: %s", strerror(errno));


What about error_setg_errno()?


Ok, I am not conscious that there exist error_setg_errno() :p




  ret = -errno;
  goto out;
  }
@@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, 
uint32_t val)
  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
  if (rc != len) {
  ret = -errno;
+error_setg(errp, "read err: %s", strerror(errno));


Same here.

OK.



  }
  out:
  close(config_fd);
  return ret;
  }

-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
  {
  uint32_t val = 0;
  int rc, i, num;
@@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
  for (i = 0; i < num; i++) {
  pos = igd_host_bridge_infos[i].offset;
  len = igd_host_bridge_infos[i].len;
-rc = host_pci_config_read(pos, len, val);
+rc = host_pci_config_read(pos, len, val, errp);
  if (rc) {


The usual pattern for error checking and propagation is:

 Error *err;
 host_pci_config_read(pos, len, val, );
 if (err) {
 error_propagate(errp, local_err);
 return;
 }



Yup. I see


-return -ENODEV;
+return;
  }
  pci_default_write_config(pci_dev, pos, val, len);
  }
-
-return 0;
  }

  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
*klass, void *data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

-k->init = igd_pt_i440fx_initfn;
+k->realize = igd_pt_i440fx_realize;
  dc->desc = "IGD Passthrough Host bridge";
  }

--
2.1.0







--
Yours Sincerely,

Cao Jin

[Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2015-12-18 Thread Cao jin 
Signed-off-by: Cao jin 
---
 hw/pci-host/piix.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..e3840f0 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
 {0xa8, 4},  /* SNB: base of GTT stolen memory */
 };
 
-static int host_pci_config_read(int pos, int len, uint32_t val)
+static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)
 {
 char path[PATH_MAX];
 int config_fd;
@@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
uint32_t val)
 int ret = 0;
 
 if (rc >= size || rc < 0) {
+error_setg(errp, "No such device");
 return -ENODEV;
 }
 
 config_fd = open(path, O_RDWR);
 if (config_fd < 0) {
+error_setg(errp, "No such device");
 return -ENODEV;
 }
 
 if (lseek(config_fd, pos, SEEK_SET) != pos) {
+error_setg(errp, "lseek err: %s", strerror(errno));
 ret = -errno;
 goto out;
 }
@@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, 
uint32_t val)
 } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 if (rc != len) {
 ret = -errno;
+error_setg(errp, "read err: %s", strerror(errno));
 }
 out:
 close(config_fd);
 return ret;
 }
 
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
 {
 uint32_t val = 0;
 int rc, i, num;
@@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
 for (i = 0; i < num; i++) {
 pos = igd_host_bridge_infos[i].offset;
 len = igd_host_bridge_infos[i].len;
-rc = host_pci_config_read(pos, len, val);
+rc = host_pci_config_read(pos, len, val, errp);
 if (rc) {
-return -ENODEV;
+return;
 }
 pci_default_write_config(pci_dev, pos, val, len);
 }
-
-return 0;
 }
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = igd_pt_i440fx_initfn;
+k->realize = igd_pt_i440fx_realize;
 dc->desc = "IGD Passthrough Host bridge";
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2015-12-18 Thread Eduardo Habkost 
On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
> Signed-off-by: Cao jin 
> ---
>  hw/pci-host/piix.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..e3840f0 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>  {0xa8, 4},  /* SNB: base of GTT stolen memory */
>  };
>  
> -static int host_pci_config_read(int pos, int len, uint32_t val)
> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)

You don't need the return value anymore, if you report errors
through the errp parameter. The function can be void, now.

>  {
>  char path[PATH_MAX];
>  int config_fd;
> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
> uint32_t val)
>  int ret = 0;
>  
>  if (rc >= size || rc < 0) {
> +error_setg(errp, "No such device");
>  return -ENODEV;
>  }
>  
>  config_fd = open(path, O_RDWR);
>  if (config_fd < 0) {
> +error_setg(errp, "No such device");
>  return -ENODEV;
>  }
>  
>  if (lseek(config_fd, pos, SEEK_SET) != pos) {
> +error_setg(errp, "lseek err: %s", strerror(errno));

What about error_setg_errno()?

>  ret = -errno;
>  goto out;
>  }
> @@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, 
> uint32_t val)
>  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  if (rc != len) {
>  ret = -errno;
> +error_setg(errp, "read err: %s", strerror(errno));

Same here.

>  }
>  out:
>  close(config_fd);
>  return ret;
>  }
>  
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
>  {
>  uint32_t val = 0;
>  int rc, i, num;
> @@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice 
> *pci_dev)
>  for (i = 0; i < num; i++) {
>  pos = igd_host_bridge_infos[i].offset;
>  len = igd_host_bridge_infos[i].len;
> -rc = host_pci_config_read(pos, len, val);
> +rc = host_pci_config_read(pos, len, val, errp);
>  if (rc) {

The usual pattern for error checking and propagation is:

Error *err;
host_pci_config_read(pos, len, val, );
if (err) {
error_propagate(errp, local_err);
return;
}

> -return -ENODEV;
> +return;
>  }
>  pci_default_write_config(pci_dev, pos, val, len);
>  }
> -
> -return 0;
>  }
>  
>  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->init = igd_pt_i440fx_initfn;
> +k->realize = igd_pt_i440fx_realize;
>  dc->desc = "IGD Passthrough Host bridge";
>  }
>  
> -- 
> 2.1.0
> 
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

2015-12-18 Thread Markus Armbruster 
One short remark in addition to Eduardo's review.

Eduardo Habkost  writes:

> On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
>> Signed-off-by: Cao jin 
>> ---
>>  hw/pci-host/piix.c | 16 +---
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 715208b..e3840f0 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>  {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>  };
>>  
>> -static int host_pci_config_read(int pos, int len, uint32_t val)
>> +static int host_pci_config_read(int pos, int len, uint32_t val, Error 
>> **errp)
>
> You don't need the return value anymore, if you report errors
> through the errp parameter. The function can be void, now.
>
>>  {
>>  char path[PATH_MAX];
>>  int config_fd;
>> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, 
>> uint32_t val)
>>  int ret = 0;
>>  
>>  if (rc >= size || rc < 0) {
>> +error_setg(errp, "No such device");
>>  return -ENODEV;
>>  }
>>  
>>  config_fd = open(path, O_RDWR);
>>  if (config_fd < 0) {
>> +error_setg(errp, "No such device");
>>  return -ENODEV;
>>  }

Can we come up with nicer error messages?

[...]