Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The other two PRs have been closed, is this one still needed? sonic-gnmi: sonic-net/sonic-gnmi#387 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a new SONiC YANG RPC and translib transformer callback to support the gNOI File Remove workflow (coordinated with sonic-gnmi and sonic-host-services PRs).
Changes:
- Introduces a new YANG module
sonic-file-mgmtdefining an RPCremovewithremote_fileinput. - Adds a YANG annotation module wiring the RPC to a translib RPC callback (
rpc_file_remove_cb). - Implements the translib RPC callback that forwards the request to host services via D-Bus (
HostQuery("file.remove", ...)).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| translib/transformer/xfmr_sonic_file_mgmt.go | Adds RPC callback implementation for file remove via HostQuery/D-Bus. |
| models/yang/sonic/sonic-file-mgmt.yang | Defines the new sonic-file-mgmt RPC schema (remove, remote_file). |
| models/yang/annotations/sonic-file-mgmt-annot.yang | Binds the YANG RPC to the translib callback using sonic-ext:rpc-callback. |
| var operand struct { | ||
| RemoteFile string `json:"remote_file"` | ||
| } |
There was a problem hiding this comment.
RPC input unmarshalling doesn't match the JSON structure used by existing RPC callbacks (e.g., xfmr_showtech.go expects a top-level ":input" wrapper). With the current struct tags, operand.RemoteFile will remain empty for RFC7951-style payloads, causing the HostQuery to be invoked with an empty path. Update the operand struct to unmarshal the module-qualified input container and extract remote_file from there.
| result, _ := json.Marshal(&output) | ||
|
|
||
| host_output := HostQuery("file.remove", operand.RemoteFile) | ||
| if host_output.Err != nil { | ||
| glog.Errorf("Error: File Remove host Query failed: err=%v", host_output.Err) | ||
| return nil, host_output.Err |
There was a problem hiding this comment.
remote_file is not validated after parsing. If the field is missing/empty (including due to input unmarshalling issues), the code will call HostQuery("file.remove", ""), which can lead to unintended behavior. Reject empty remote_file with tlerr.InvalidArgs (and ideally include which field is missing).
| if host_output.Body[0].(int32) != 0 { | ||
| err = tlerr.New(host_output.Body[1].(string)) |
There was a problem hiding this comment.
The code assumes host_output.Body has at least two elements and that they are (int32, string). Since HostQuery returns a raw []interface{} from D-Bus, these direct index accesses and type assertions can panic if the host service returns a different signature or an empty body. Add length checks and safe type assertions (with an internal error if the response shape is unexpected).
| if host_output.Body[0].(int32) != 0 { | |
| err = tlerr.New(host_output.Body[1].(string)) | |
| if len(host_output.Body) < 2 { | |
| err = tlerr.New("Internal error: unexpected response from file.remove host service") | |
| glog.Errorf("Error: File Remove host Query returned invalid body length=%d", len(host_output.Body)) | |
| return nil, err | |
| } | |
| status, ok := host_output.Body[0].(int32) | |
| if !ok { | |
| err = tlerr.New("Internal error: unexpected response from file.remove host service") | |
| glog.Errorf("Error: File Remove host Query returned invalid status type=%T", host_output.Body[0]) | |
| return nil, err | |
| } | |
| msg, ok := host_output.Body[1].(string) | |
| if !ok { | |
| err = tlerr.New("Internal error: unexpected response from file.remove host service") | |
| glog.Errorf("Error: File Remove host Query returned invalid message type=%T", host_output.Body[1]) | |
| return nil, err | |
| } | |
| if status != 0 { | |
| err = tlerr.New(msg) |
|
|
||
| rpc remove { | ||
| description | ||
| "Remove removes the specified file from the target"; |
There was a problem hiding this comment.
The rpc description text "Remove removes the specified file..." is grammatically awkward/redundant. Consider rewording to something like "Remove deletes the specified file from the target" to improve clarity in generated docs/clients.
| "Remove removes the specified file from the target"; | |
| "Remove deletes the specified file from the target"; |
Add support for gNOI File Remove.
Other PRs: