Project

General

Profile

Actions

Bug #16115

closed

Want 64-bit libnwam

Added by Marcel Telka 6 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
lib - userland libraries
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

Description

OpenIndiana switched nwam-manager to 64-bit few weeks ago. For that the 64-bit libnwam was needed. Since libnwam lives in the illumos-gate let's upstream that work.


Related issues

Blocked by illumos gate - Bug #16118: Fix problems with libnwam communication between 32-bit and 64-bit applicationsClosedCarsten Grzemba

Actions
Actions #1

Updated by Marcel Telka 6 months ago

Testing

This is used by OpenIndiana users for almost two months now.

Actions #2

Updated by Electric Monk 6 months ago

  • Gerrit CR set to 3186
Actions #3

Updated by Carsten Grzemba 6 months ago

The work is still in progress. With the 64-bit libwnam the communication between nwamd and mwan-manager is broken. Until now I have only a fix for the doorfs communication but the message queue communication msgsnd -- msgrcv is still not working.

Actions #4

Updated by Marcel Telka 6 months ago

Carsten Grzemba wrote in #note-3:

The work is still in progress. With the 64-bit libwnam the communication between nwamd and mwan-manager is broken. Until now I have only a fix for the doorfs communication but the message queue communication msgsnd -- msgrcv is still not working.

Yes, sure. I expect the further work in this area to be done (and upstreamed) as a followup changes.

Actions #5

Updated by Joshua M. Clulow 6 months ago

I don't think we should Integrate a partially functional library. Let's get it completely working and tested as a unit before integration.

Actions #6

Updated by Marcel Telka 6 months ago

Joshua M. Clulow wrote in #note-5:

I don't think we should Integrate a partially functional library. Let's get it completely working and tested as a unit before integration.

This is not a new library. Just addition of 64-bit build for already integrated one (and built currently as 32-bit only).

Without this if you build illumos-gate on OpenIndiana and then update your machine to the built bits you'll get broken system because nwam-manager will be unable to start at all due to missing 64-bit libnwam. With this integrated your nwam-manager will start but it will just not work properly.

This work was done by Hans, while the adaptation of libnwam for 64-bit is being done by Carsten, so I believe it would be cleaner to have two separate changesets.

So I propose this: wait with this one until Carsten fixes libnwam to be able to fully work as 64-bit library. Once his work is integrated I'll RTI this one. Carsten, could you please create new ticket for your work please so I can set it as a blocker of this one?

Thank you.

Actions #7

Updated by Joshua M. Clulow 6 months ago

I understand it's a 64-bit version of an existing 32-bit library. It (the 64-bit version) is a library we don't ship at all right now, and it needs to work (completely) at the moment we start to build and ship it. If there are bugs in the code we can fix and test separately before adding the new library, that's fine; otherwise it cannot be two separate changes.

Actions #8

Updated by Carsten Grzemba 6 months ago

To fix the message queue problem the only solution I have so far is:

diff --git a/usr/src/lib/libnwam/common/libnwam_events.c b/usr/src/lib/libnwam/common/libnwam_events.c
index 3bb6adf4a4..99e01c113b 100644
--- a/usr/src/lib/libnwam/common/libnwam_events.c
+++ b/usr/src/lib/libnwam/common/libnwam_events.c
@@ -93,6 +93,7 @@ nwam_event_wait(nwam_event_t *eventp)
 {
        nwam_error_t err;
        nwam_event_t event;
+       nwam_event_t ep;

        assert(eventp != NULL);

@@ -118,6 +119,12 @@ nwam_event_wait(nwam_event_t *eventp)
                }
        }

+#if defined(_LP64)
+       ep = (void*)event+4;
+        ep->mesg_type = (int) event->nwe_type;
+       bcopy(ep, event, event->nwe_size);
+#endif
+
        /* Resize event down from maximum size */
        if ((*eventp = realloc(event, event->nwe_size)) == NULL)
                return (NWAM_NO_MEMORY);

but this looks a little dirty to me, because this works only if the nwamd is still running in 32-bit.

Could it be if the data struct nwam_event_t for the first data field nwe_type had the type long instead of int as like described in MSGSND (2) , all would work out of the box in 32-bit and 64-bit?

Actions #10

Updated by Gordon Ross 6 months ago

The pack approach seems fine. You could use the (more modern) `__packed` instead.

Actions #11

Updated by Carsten Grzemba 6 months ago

for the message queue problem seems to be the right fix:

diff --git a/usr/src/lib/libnwam/common/libnwam.h b/usr/src/lib/libnwam/common/libnwam.h
index 63c6271e46..68053dcae6 100644
--- a/usr/src/lib/libnwam/common/libnwam.h
+++ b/usr/src/lib/libnwam/common/libnwam.h
@ -970,7 +970,7 @ typedef enum {

typedef struct nwam_event *nwam_event_t;
struct nwam_event {
- int nwe_type;
+ long nwe_type;
uint32_t nwe_size;
union {

So I can put things together for create a CR now.

Actions #12

Updated by Joshua M. Clulow 6 months ago

  • Blocked by Bug #16118: Fix problems with libnwam communication between 32-bit and 64-bit applications added
Actions #13

Updated by Joshua M. Clulow 6 months ago

NB: Please don't use subtasks, just use "blocked by" related issues.

Actions #14

Updated by Marcel Telka 5 months ago

  • Status changed from In Progress to Pending RTI
Actions #15

Updated by Electric Monk 5 months ago

  • Status changed from Pending RTI to Closed
  • % Done changed from 0 to 100

git commit d7c02bfa752bc4da7e47dd39cf6baa20bec9a956

commit  d7c02bfa752bc4da7e47dd39cf6baa20bec9a956
Author: Hans Rosenfeld <rosenfeld@grumpf.hope-2000.org>
Date:   2024-01-24T15:56:23.000Z

    16115 Want 64-bit libnwam
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Gordon Ross <Gordon.W.Ross@gmail.com>
    Reviewed by: Marcel Telka <marcel@telka.sk>
    Approved by: Dan McDonald <danmcd@mnx.io>

Actions

Also available in: Atom PDF