Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes:
> Hi, Markus
>
> On 05/19/2017 03:20 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes:
>>
>>> The rocker device still implements the old PCIDeviceClass .init()
>>> instead of the new .realize(). All devices need to be converted to
>>> .realize().
>>>
>>> .init() reports errors with fprintf() and return 0 on success, negative
>>> number on failure. Meanwhile, when -device rocker fails, it first report
>>> a specific error, then a generic one, like this:
>>>
>>> $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>>> rocker: name too long; please shorten to at most 9 chars
>>> qemu-system-x86_64: -device rocker,name=qemu-rocker: Device
>>> initialization failed
>>>
>>> Now, convert it to .realize() that passes errors to its callers via its
>>> errp argument. Also avoid the superfluous second error message. After
>>> the patch, effect like this:
>>>
>>> $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>>> qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long;
>>> please shorten to at most 9 chars
>>>
>>> Cc: jasow...@redhat.com
>>> Cc: j...@resnulli.us
>>> Cc: f4...@amsat.org
>>> Cc: arm...@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
>>
>> The conversion to realize() looks good to me, therefore
>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>>
>> However, I spotted a few issues not related to this patch.
>>
>> 1. Unusual macro names
>>
>> #define ROCKER "rocker"
>>
>> #define to_rocker(obj) \
>> OBJECT_CHECK(Rocker, (obj), ROCKER)
>>
>> Please clean up to
>>
>> #define TYPE_ROCKER "rocker"
>>
>> #define ROCKER(obj) \
>> OBJECT_CHECK(Rocker, (obj), TYPE_ROCKER)
>>
>> in a separate patch.
>
> Thanks, will fix it in the next version.
>
>>
>> 2. Memory leaks on error and unplug
>>
>> Explained inline below.
>>
>>> ---
>>> hw/net/rocker/rocker.c | 27 +++++++++++----------------
>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>>> index a382a6f..b9a77f1 100644
>>> --- a/hw/net/rocker/rocker.c
>>> +++ b/hw/net/rocker/rocker.c
>>> @@ -1252,20 +1252,18 @@ rollback:
>>> return err;
>>> }
>>>
>>> -static int rocker_msix_init(Rocker *r)
>>> +static int rocker_msix_init(Rocker *r, Error **errp)
>>> {
>>> PCIDevice *dev = PCI_DEVICE(r);
>>> int err;
>>> - Error *local_err = NULL;
>>>
>>> err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>>> &r->msix_bar,
>>> ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>>> &r->msix_bar,
>>> ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
>>> - 0, &local_err);
>>> + 0, errp);
>>> if (err) {
>>> - error_report_err(local_err);
>>> return err;
>>> }
>>>
>>> @@ -1301,7 +1299,7 @@ static World *rocker_world_type_by_name(Rocker *r,
>>> const char *name)
>>> return NULL;
>>> }
>>>
>>> -static int pci_rocker_init(PCIDevice *dev)
>>> +static void pci_rocker_realize(PCIDevice *dev, Error **errp)
>>> {
>>> Rocker *r = to_rocker(dev);
>>> const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>>> @@ -1319,10 +1317,9 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>> r->world_dflt = rocker_world_type_by_name(r, r->world_name);
>>> if (!r->world_dflt) {
>>> - fprintf(stderr,
>>> - "rocker: requested world \"%s\" does not exist\n",
>>> + error_setg(errp,
>>> + "invalid argument requested world %s does not exist",
>>> r->world_name);
>>> - err = -EINVAL;
>>> goto err_world_type_by_name;
>>> }
>>>
>>> @@ -1342,7 +1339,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>> /* MSI-X init */
>>>
>>> - err = rocker_msix_init(r);
>>> + err = rocker_msix_init(r, errp);
>>> if (err) {
>>> goto err_msix_init;
>>> }
>>> @@ -1354,7 +1351,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>> }
>>>
>>> if (rocker_find(r->name)) {
>>> - err = -EEXIST;
>>> + error_setg(errp, "%s already exists", r->name);
>>> goto err_duplicate;
>>> }
>>>
>>> @@ -1368,10 +1365,9 @@ static int pci_rocker_init(PCIDevice *dev)
>>> #define ROCKER_IFNAMSIZ 16
>>> #define MAX_ROCKER_NAME_LEN (ROCKER_IFNAMSIZ - 1 - 3 - 3)
>>> if (strlen(r->name) > MAX_ROCKER_NAME_LEN) {
>>> - fprintf(stderr,
>>> - "rocker: name too long; please shorten to at most %d
>>> chars\n",
>>> + error_setg(errp,
>>> + "name too long; please shorten to at most %d chars",
>>> MAX_ROCKER_NAME_LEN);
>>> - err = -EINVAL;
>>> goto err_name_too_long;
>>> }
>>>
>>> @@ -1429,7 +1425,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>> QLIST_INSERT_HEAD(&rockers, r, next);
>>>
>>> - return 0;
>>> + return;
>>>
>>> err_name_too_long:
>>> err_duplicate:
>> rocker_msix_uninit(r);
>> err_msix_init:
>> object_unparent(OBJECT(&r->msix_bar));
>> object_unparent(OBJECT(&r->mmio));
>> err_world_type_by_name:
>> for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>> if (r->worlds[i]) {
>>> @@ -1443,7 +1439,6 @@ err_world_type_by_name:
>>> world_free(r->worlds[i]);
>>> }
>>> }
>>> - return err;
>>> }
>>>
>>
>> Does this leak r->world_name and r->name?
>
> I think it was leaked neither r->world_name nor r->name, but msix and
> worlds related.
You're right: the two are properties, so the property machinery should
free them.
>> If yes, can you delay their allocation until after the last thing that
>> can fail? That way, you don't have to free them on failure. Simplifies
>> the error paths. Keeping them as simple as practical is desireable,
>> because they're hard to test.
>
> If delay allocation of r->worlds[] until after the last, when r->name and
> r->world_name failed, passing the error message to errp is a good idea. I
> think that's what 'error paths' means, then it is really not need to free
> the memory in the goto label. Because the r->worlds[] has not yet been
> allocated. Is that right? If yes, it's really a perfect solution.
>
> But, this ignores the fact that r->name and r->world_name are depend on
> r->worlds, so r->worlds's allocation must be before them. in this case,
> the previous solution will be lose its meaning.
Delaying allocation until after all error checks is not always
practical. You decide.
[...]
