Re: [libvirt] [PATCH v5 08/13] qemu: Add zPCI address definition check

2018-09-16 Thread Yi Min Zhao 



在 2018/9/13 下午7:47, Andrea Bolognani 写道:

Just remove the full stop, like

   virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
  "%s",
  _("This QEMU binary doesn't support zPCI"));

AhGot it.

--
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 08/13] qemu: Add zPCI address definition check

2018-09-13 Thread Andrea Bolognani 
On Thu, 2018-09-13 at 18:08 +0800, Yi Min Zhao wrote:
> > [...]
> > > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > +   "%s",
> > > +   _("This QEMU binary doesn't support zPCI."));
> > 
> > No full stop at the end of the error message, please.
> 
> Could you please explain more?

Just remove the full stop, like

  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 "%s",
 _("This QEMU binary doesn't support zPCI"));

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 08/13] qemu: Add zPCI address definition check

2018-09-13 Thread Yi Min Zhao 



在 2018/9/11 下午8:44, Andrea Bolognani 写道:

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:

We should ensure that the Qemu should support zPCI when zPCI address is

s/Qemu/QEMU/

Sure.


[...]

+static int
+qemuDomainZPCIAddressDefValidate(virDomainDeviceDef *dev,
+ virQEMUCapsPtr qemuCaps)

The second argument is not aligned properly.

I'd also change the name: qemuDomainDeviceDefValidateZPCIAddress()
perhaps? It's quite a mouthful, but also more accurate.

ok.


[...]

+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s",
+   _("This QEMU binary doesn't support zPCI."));

No full stop at the end of the error message, please.

Could you please explain more?


[...]

@@ -5741,6 +5762,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef 
*dev,
  def->emulator)))
  return -1;
  
+ret = qemuDomainZPCIAddressDefValidate((virDomainDeviceDef *)dev, qemuCaps);

This cast is kinds gross. I realize virDomainDeviceGetInfo()
requires the pointer to be non-const, but from a semantics point
of view qemuDomainZPCIAddressDefValidate() should be okay with
being passed a const pointer - it's only validating the device,
not changing it, after all.

Please move the cast into the function instead of requiring the
caller to perform it.

We should also have a generic qemuDomainDeviceDefValidateAddress()
wrapper that calls the zPCI-specific one only for PCI devices, and
call that one from here.


Got it.

--
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 08/13] qemu: Add zPCI address definition check

2018-09-11 Thread Andrea Bolognani 
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> We should ensure that the Qemu should support zPCI when zPCI address is

s/Qemu/QEMU/

[...]
> +static int
> +qemuDomainZPCIAddressDefValidate(virDomainDeviceDef *dev,
> + virQEMUCapsPtr qemuCaps)

The second argument is not aligned properly.

I'd also change the name: qemuDomainDeviceDefValidateZPCIAddress()
perhaps? It's quite a mouthful, but also more accurate.

[...]
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   "%s",
> +   _("This QEMU binary doesn't support zPCI."));

No full stop at the end of the error message, please.

[...]
> @@ -5741,6 +5762,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef 
> *dev,
>  def->emulator)))
>  return -1;
>  
> +ret = qemuDomainZPCIAddressDefValidate((virDomainDeviceDef *)dev, 
> qemuCaps);

This cast is kinds gross. I realize virDomainDeviceGetInfo()
requires the pointer to be non-const, but from a semantics point
of view qemuDomainZPCIAddressDefValidate() should be okay with
being passed a const pointer - it's only validating the device,
not changing it, after all.

Please move the cast into the function instead of requiring the
caller to perform it.

We should also have a generic qemuDomainDeviceDefValidateAddress()
wrapper that calls the zPCI-specific one only for PCI devices, and
call that one from here.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list