diff mbox

[qemu,3/3] rocker: allow user to specify rocker world by property

Message ID 1455876403-8575-4-git-send-email-jiri@resnulli.us (mailing list archive)
State New, archived
Headers show

Commit Message

Jiri Pirko Feb. 19, 2016, 10:06 a.m. UTC 
From: Jiri Pirko <jiri@mellanox.com>

Add property to specify rocker world. All ports will be assigned to this
world.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 hw/net/rocker/rocker.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Comments

Stefan Hajnoczi Feb. 22, 2016, 5:51 p.m. UTC | #1 
On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>      /* allocate worlds */
>  
>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
> +
> +    if (!r->world_name) {
> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
> +    }
> +
> +    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",
> +                r->world_name);
> +        return -EINVAL;
> +    }

world_name is leaked here.  Please use goto to run the appropriate
cleanup code instead of returning directly.
Jiri Pirko Feb. 22, 2016, 6:06 p.m. UTC | #2 
Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
>On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
>> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>>      /* allocate worlds */
>>  
>>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
>> +
>> +    if (!r->world_name) {
>> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>> +    }
>> +
>> +    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",
>> +                r->world_name);
>> +        return -EINVAL;
>> +    }
>
>world_name is leaked here.  Please use goto to run the appropriate
>cleanup code instead of returning directly.

I did the same what is done with "r->name = g_strdup(ROCKER)"

I assumed since this is a property, the caller core will take care of
that.
Stefan Hajnoczi Feb. 25, 2016, 11:31 a.m. UTC | #3 
On Mon, Feb 22, 2016 at 07:06:34PM +0100, Jiri Pirko wrote:
> Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
> >On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
> >> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
> >>      /* allocate worlds */
> >>  
> >>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
> >> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
> >> +
> >> +    if (!r->world_name) {
> >> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
> >> +    }
> >> +
> >> +    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",
> >> +                r->world_name);
> >> +        return -EINVAL;
> >> +    }
> >
> >world_name is leaked here.  Please use goto to run the appropriate
> >cleanup code instead of returning directly.
> 
> I did the same what is done with "r->name = g_strdup(ROCKER)"
> 
> I assumed since this is a property, the caller core will take care of
> that.

You are right, the string properties will be freed by qdev property
release.

The return statement added by this patch still leaks
r->worlds[ROCKER_WORLD_TYPE_OF_DPA] so goto err_world_alloc is needed.

(pci_rocker_uninit() is not called if pci_rocker_init() fails.)
Jiri Pirko Feb. 25, 2016, 11:48 a.m. UTC | #4 
Thu, Feb 25, 2016 at 12:31:58PM CET, stefanha@gmail.com wrote:
>On Mon, Feb 22, 2016 at 07:06:34PM +0100, Jiri Pirko wrote:
>> Mon, Feb 22, 2016 at 06:51:50PM CET, stefanha@gmail.com wrote:
>> >On Fri, Feb 19, 2016 at 11:06:43AM +0100, Jiri Pirko wrote:
>> >> @@ -1297,7 +1310,18 @@ static int pci_rocker_init(PCIDevice *dev)
>> >>      /* allocate worlds */
>> >>  
>> >>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>> >> -    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
>> >> +
>> >> +    if (!r->world_name) {
>> >> +        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>> >> +    }
>> >> +
>> >> +    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",
>> >> +                r->world_name);
>> >> +        return -EINVAL;
>> >> +    }
>> >
>> >world_name is leaked here.  Please use goto to run the appropriate
>> >cleanup code instead of returning directly.
>> 
>> I did the same what is done with "r->name = g_strdup(ROCKER)"
>> 
>> I assumed since this is a property, the caller core will take care of
>> that.
>
>You are right, the string properties will be freed by qdev property
>release.
>
>The return statement added by this patch still leaks
>r->worlds[ROCKER_WORLD_TYPE_OF_DPA] so goto err_world_alloc is needed.
>
>(pci_rocker_uninit() is not called if pci_rocker_init() fails.)

Will fix and send v2. Thanks.
diff mbox

Patch

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index a1d921d..d0bdfcb 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -43,6 +43,7 @@  struct rocker {
 
     /* switch configuration */
     char *name;                  /* switch name */
+    char *world_name;            /* world name */
     uint32_t fp_ports;           /* front-panel port count */
     NICPeers *fp_ports_peers;
     MACAddr fp_start_macaddr;    /* front-panel port 0 mac addr */
@@ -1286,6 +1287,18 @@  static void rocker_msix_uninit(Rocker *r)
     rocker_msix_vectors_unuse(r, ROCKER_MSIX_VEC_COUNT(r->fp_ports));
 }
 
+static World *rocker_world_type_by_name(Rocker *r, const char *name)
+{
+    int i;
+
+    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
+        if (strcmp(name, world_name(r->worlds[i])) == 0) {
+            return r->worlds[i];
+	}
+    }
+    return NULL;
+}
+
 static int pci_rocker_init(PCIDevice *dev)
 {
     Rocker *r = to_rocker(dev);
@@ -1297,7 +1310,18 @@  static int pci_rocker_init(PCIDevice *dev)
     /* allocate worlds */
 
     r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
-    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
+
+    if (!r->world_name) {
+        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
+    }
+
+    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",
+                r->world_name);
+        return -EINVAL;
+    }
 
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (!r->worlds[i]) {
@@ -1509,6 +1533,7 @@  static void rocker_reset(DeviceState *dev)
 
 static Property rocker_properties[] = {
     DEFINE_PROP_STRING("name", Rocker, name),
+    DEFINE_PROP_STRING("world", Rocker, world_name),
     DEFINE_PROP_MACADDR("fp_start_macaddr", Rocker,
                         fp_start_macaddr),
     DEFINE_PROP_UINT64("switch_id", Rocker,