PengZheng commented on code in PR #476:
URL: https://github.com/apache/celix/pull/476#discussion_r1111421749
##########
bundles/deployment_admin/src/deployment_admin.c:
##########
@@ -581,10 +582,11 @@ celix_status_t
deploymentAdmin_updateDeploymentPackageBundles(deployment_admin_p
deploymentPackage_getBundle(source, info->symbolicName,
&updateBundle);
if (updateBundle != NULL) {
//printf("Update bundle from: %s\n", bundlePath);
+ celix_bundleContext_updateBundle(admin->context,
celix_bundle_getId(updateBundle), bundlePath);
bundle_update(updateBundle, bundlePath);
Review Comment:
```suggestion
```
It should be removed.
##########
bundles/http_admin/http_admin/src/activator.c:
##########
@@ -52,7 +44,28 @@ typedef struct http_admin_activator {
static int http_admin_start(http_admin_activator_t *act,
celix_bundle_context_t *ctx) {
celix_bundle_t *bundle = celix_bundleContext_getBundle(ctx);
- char* root = celix_bundle_getEntry(bundle, "root");
+ char* storeRoot = celix_bundle_getDataFile(bundle, "");
+ if (storeRoot == NULL) {
+ celix_bundleContext_log(ctx, CELIX_LOG_LEVEL_ERROR, "Cannot get bundle
store root for the http admin bundle.");
+ return CELIX_BUNDLE_EXCEPTION;
+ }
+
+ char* httpRoot = NULL;
+ int rc = asprintf(&httpRoot, "%s/root", storeRoot);
+ if (rc < 0) {
+ celix_bundleContext_log(ctx, CELIX_LOG_LEVEL_ERROR, "Cannot create
http root directory for the http admin bundle.");
+ free(storeRoot);
+ return CELIX_BUNDLE_EXCEPTION;
Review Comment:
Why not return more specific error like CELIX_ENOMEM?
##########
bundles/http_admin/http_admin/src/activator.c:
##########
@@ -72,22 +85,22 @@ static int http_admin_start(http_admin_activator_t *act,
celix_bundle_context_t
act->useWebsockets = prop_use_websockets;
const char *svr_opts[] = {
- "document_root", root,
+ "document_root", httpRoot,
"listening_ports", prop_port,
"websocket_timeout_ms", prop_timeout,
- "websocket_root", root,
+ "websocket_root", httpRoot,
"num_threads", prop_num_threads,
NULL
};
//Try the 'LISTENING_PORT' property first, if failing continue with the
port range functionality
- act->httpManager = httpAdmin_create(ctx, root, svr_opts);
+ act->httpManager = httpAdmin_create(ctx, httpRoot, svr_opts);
for(long port = prop_port_min; act->httpManager == NULL && port <=
prop_port_max; port++) {
char *port_str;
asprintf(&port_str, "%li", port);
svr_opts[3] = port_str;
- act->httpManager = httpAdmin_create(ctx, root, svr_opts);
+ act->httpManager = httpAdmin_create(ctx, httpRoot, svr_opts);
Review Comment:
I found several issues here:
1. If `httpAdmin_create` finally fails here, an error code should be
returned instead.
2. The above applies to `websocketAdmin_create`.
3. `httpAdmin_create` should take the responsibility of releasing `httpRoot`
in error condition, which is currently not done.
4. In `http_admin_stop`, various resources is NOT released in reverse order
as they are acquired.
Our new error injector may help testing this.
##########
bundles/shell/shell/src/install_command.c:
##########
@@ -31,7 +31,7 @@ bool installCommand_execute(void *handle, const char
*const_line, FILE *outStrea
char *line = celix_utils_strdup(const_line);
// ignore the command
- sub = strtok_r(line, delims, &save_ptr);
Review Comment:
Our shell command's interface is fundamentally flawed: it will split a
single command line argument containing space into two arguments. Bash and any
other shells I know do a perfect job of separating command line into (argc,
argv), which IMHO should be preserved by our interface and protocol
serialization. However, it's too late to change the interface now.
Of course, this is off the topic of the current PR.
##########
bundles/http_admin/http_admin/src/http_admin.c:
##########
@@ -498,14 +491,12 @@ void http_admin_startBundle(void *data, const
celix_bundle_t *bundle) {
}
if(status == CELIX_SUCCESS && manifest != NULL) {
- const char *aliases = NULL;
- const char *revision_root = NULL;
- long bnd_id;
-
- aliases = manifest_getValue(manifest, "X-Web-Resource");
- bnd_id = celix_bundle_getId(bundle);
- bundleRevision_getRoot(revision, &revision_root);
- createAliasesSymlink(aliases, admin->root, revision_root, bnd_id,
admin->aliasList);
+
+ long bnd_id = celix_bundle_getId(bundle);
+ const char* aliases = manifest_getValue(manifest, "X-Web-Resource");
Review Comment:
`celix_bundle_getManifestValue` should be used instead, so that we can
remove `manifest_getValue`
/`bundleArchive_getCurrentRevision`/`bundleRevision_getManifest`.
##########
bundles/http_admin/http_admin/src/activator.c:
##########
@@ -52,7 +44,28 @@ typedef struct http_admin_activator {
static int http_admin_start(http_admin_activator_t *act,
celix_bundle_context_t *ctx) {
celix_bundle_t *bundle = celix_bundleContext_getBundle(ctx);
- char* root = celix_bundle_getEntry(bundle, "root");
+ char* storeRoot = celix_bundle_getDataFile(bundle, "");
+ if (storeRoot == NULL) {
+ celix_bundleContext_log(ctx, CELIX_LOG_LEVEL_ERROR, "Cannot get bundle
store root for the http admin bundle.");
+ return CELIX_BUNDLE_EXCEPTION;
+ }
+
+ char* httpRoot = NULL;
+ int rc = asprintf(&httpRoot, "%s/root", storeRoot);
+ if (rc < 0) {
+ celix_bundleContext_log(ctx, CELIX_LOG_LEVEL_ERROR, "Cannot create
http root directory for the http admin bundle.");
+ free(storeRoot);
+ return CELIX_BUNDLE_EXCEPTION;
+ }
+ free(storeRoot);
+
+ celix_status_t status = celix_utils_createDirectory(httpRoot, false, NULL);
+ if (status != CELIX_SUCCESS) {
+ celix_bundleContext_log(ctx, CELIX_LOG_LEVEL_ERROR, "Cannot create
http root directory for the http admin bundle.");
+ free(httpRoot);
+ return CELIX_BUNDLE_EXCEPTION;
Review Comment:
Is more specific error better?
##########
bundles/http_admin/http_admin/src/activator.c:
##########
@@ -72,22 +85,22 @@ static int http_admin_start(http_admin_activator_t *act,
celix_bundle_context_t
act->useWebsockets = prop_use_websockets;
const char *svr_opts[] = {
- "document_root", root,
+ "document_root", httpRoot,
"listening_ports", prop_port,
"websocket_timeout_ms", prop_timeout,
- "websocket_root", root,
+ "websocket_root", httpRoot,
"num_threads", prop_num_threads,
NULL
};
//Try the 'LISTENING_PORT' property first, if failing continue with the
port range functionality
- act->httpManager = httpAdmin_create(ctx, root, svr_opts);
+ act->httpManager = httpAdmin_create(ctx, httpRoot, svr_opts);
for(long port = prop_port_min; act->httpManager == NULL && port <=
prop_port_max; port++) {
char *port_str;
asprintf(&port_str, "%li", port);
svr_opts[3] = port_str;
- act->httpManager = httpAdmin_create(ctx, root, svr_opts);
+ act->httpManager = httpAdmin_create(ctx, httpRoot, svr_opts);
Review Comment:
Fixing the above may belong to a separate PR.
If so, an issue is needed to keep a record.
##########
bundles/shell/shell/src/start_command.c:
##########
@@ -23,42 +23,37 @@
#include "celix_api.h"
#include "std_commands.h"
+#include "celix_convert_utils.h"
-bool startCommand_execute(void *handle, const char *const_command, FILE
*outStream, FILE *errStream) {
+bool startCommand_execute(void *handle, const char *constCommandLine, FILE
*outStream, FILE *errStream) {
Review Comment:
If `(argc, argv)` are passed instead of `constCommandLinke`, we can avoid
all the hassle of strdup/strtok.
It will also make integrating with 3rd party library such as
https://github.com/argtable/argtable3 easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
