Hi, Thank you for the review. On Thursday, February 29, 2024 8:29:07 PM IST Quentin Monnet wrote: > [...] > The error string should be updated, we're not trying to pin one object > file here but to mount the bpffs on a directory to pin several objects. Sorry, I forgot to change the error message here. I'll change it. > > + name, err_str); > > Formatting nit: "name" should be aligned with the argument from the line > above (the opening double quote). You can catch this by running > "./scripts/checkpatch.pl --strict" on your patch/commit. Got it. I ran the checkpatch script without --strict, so it didn't catch this. > > + } > > + > > + return err; > > + } > > This block above cannot be before the check on "block_mount", or we will > ignore the "--nomount" option if the user passes it. Oh, I understand this now. The block_mount check should be done before any attempt to mount the bpffs. > Perhaps it would be clearer to split the logics of mount_bpffs_for_pin() > into two subfunctions, one for directories, one for file paths. This way > we would avoid to call malloc() and dirname() when "name" is already a > directory, and it would be easier to follow the different cases. I agree. I am thinking of having two mount_bpffs_for_pin_* functions, one for dirs and one for files. These will handle the differences between dirs and files and they can both call a third (but static) mount_bpffs_for_pin where the code common to both scenarios will exist. The actual mounting and --nomount check can be done in this static function. > [...] > We use err_str to pass it to mnt_fs, but we cannot use it here (it is > not set by mkdir). We probably want "strerror(errno)" instead. Understood. I'll change this too. Thanks, Sahil
